Tighten up btree_gist's handling of truncated bounds.

Truncating an internal node's upper bound can cause it to compare
less than some values that in fact are included in the represented
leaf page.  So we need a hack to make sure it looks large enough
to include all values that could be on the page.  But there's no
equivalent issue for the lower bound.  The fact that the code did
fuzzy comparisons for the lower bound too seems to be the result of
fuzzy thinking.  Or maybe there was a desire to not assume too much
about what the datatype's comparison rule is; but we've already
fully bought into the premise that internal keys compare like bytea.

Hence, remove the useless check against the key's lower bound in
gbt_var_node_pf_match.  The comparable check in gbt_var_penalty may
also be useless, but I'm not quite sure.  In any case that seems
negligible from a performance standpoint, so I left it alone.

Also, in the strategy cases in gbt_var_consistent that only
require comparisons to the lower bound, there's no need to call
gbt_var_node_pf_match at all.  Refactor that logic by inventing
macros lower_is_below_query and upper_is_above_query to directly
express what we need to test.  I also took this opportunity to flip
all the tests around to be "indexkey OP query" rather than mostly
being the reverse: IMO this makes the code less confusing since the
tests now match the names of the strategies.

Also, in the name of consistency, make gbt_num_consistent look
like that too.  There's no functional change there, but this
should be more readable going forward.

Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/AH*AvQCYKhQGVvPWi1GiU4oY.8.1781609375063.Hmail.3020001251@tju.edu.cn

diff --git a/contrib/btree_gist/btree_utils_num.c b/contrib/btree_gist/btree_utils_num.c
index 79c6223f99cd4f261a355040ecdddee592152737..971485dc8eb1014049288351f94be9ff355cf0d2 100644 (file)
--- a/contrib/btree_gist/btree_utils_num.c
+++ b/contrib/btree_gist/btree_utils_num.c
@@ -269,36 +269,54 @@ gbt_num_consistent(const GBT_NUMKEY_R *key,
 {
        bool            retval;
 
+       /*
+        * On leaf pages we directly apply the check "key->lower OP query"; we
+        * need not consider key->upper since it will be equal to key->lower.
+        *
+        * On internal pages we mostly need to check "is lower bound below query?"
+        * and/or "is upper bound above query?", where we must allow equality in
+        * both cases.
+        */
+#define lower_is_below_query() \
+       tinfo->f_le(key->lower, query, flinfo)
+#define upper_is_above_query() \
+       tinfo->f_ge(key->upper, query, flinfo)
+
        switch (strategy)
        {
                case BTLessEqualStrategyNumber:
-                       retval = tinfo->f_ge(query, key->lower, flinfo);
+                       if (is_leaf)
+                               retval = tinfo->f_le(key->lower, query, flinfo);
+                       else
+                               retval = lower_is_below_query();
                        break;
                case BTLessStrategyNumber:
                        if (is_leaf)
-                               retval = tinfo->f_gt(query, key->lower, flinfo);
+                               retval = tinfo->f_lt(key->lower, query, flinfo);
                        else
-                               retval = tinfo->f_ge(query, key->lower, flinfo);
+                               retval = lower_is_below_query();
                        break;
                case BTEqualStrategyNumber:
                        if (is_leaf)
-                               retval = tinfo->f_eq(query, key->lower, flinfo);
+                               retval = tinfo->f_eq(key->lower, query, flinfo);
                        else
-                               retval = (tinfo->f_le(key->lower, query, flinfo) &&
-                                                 tinfo->f_le(query, key->upper, flinfo));
+                               retval = lower_is_below_query() && upper_is_above_query();
                        break;
                case BTGreaterStrategyNumber:
                        if (is_leaf)
-                               retval = tinfo->f_lt(query, key->upper, flinfo);
+                               retval = tinfo->f_gt(key->lower, query, flinfo);
                        else
-                               retval = tinfo->f_le(query, key->upper, flinfo);
+                               retval = upper_is_above_query();
                        break;
                case BTGreaterEqualStrategyNumber:
-                       retval = tinfo->f_le(query, key->upper, flinfo);
+                       if (is_leaf)
+                               retval = tinfo->f_ge(key->lower, query, flinfo);
+                       else
+                               retval = upper_is_above_query();
                        break;
                case BtreeGistNotEqualStrategyNumber:
                        if (is_leaf)
-                               retval = !(tinfo->f_eq(query, key->lower, flinfo));
+                               retval = !(tinfo->f_eq(key->lower, query, flinfo));
                        else
                        {
                                /*
@@ -307,14 +325,17 @@ gbt_num_consistent(const GBT_NUMKEY_R *key,
                                 * descending if the query equals both bounds.  In all other
                                 * cases, we must descend.
                                 */
-                               retval = !(tinfo->f_eq(query, key->lower, flinfo) &&
-                                                  tinfo->f_eq(query, key->upper, flinfo));
+                               retval = !(tinfo->f_eq(key->lower, query, flinfo) &&
+                                                  tinfo->f_eq(key->upper, query, flinfo));
                        }
                        break;
                default:
                        retval = false;
        }
 
+#undef lower_is_below_query
+#undef upper_is_above_query
+
        return retval;
 }
 

diff --git a/contrib/btree_gist/btree_utils_var.c b/contrib/btree_gist/btree_utils_var.c
index 6ab2ca263efabb88aaacc24e0b54f4655ebaec75..ae1018013557047830786cb4c417e123ac3c443e 100644 (file)
--- a/contrib/btree_gist/btree_utils_var.c
+++ b/contrib/btree_gist/btree_utils_var.c
@@ -213,16 +213,14 @@ gbt_bytea_pf_match(const bytea *pf, const bytea *query, const gbtree_vinfo *tinf
  * If the data type is truncatable, then a shortened upper bound must be
  * considered to include all values that match it up to its own length,
  * even though longer values would normally be considered larger.
- *
- * XXX isn't the check against node->lower useless?
- * A truncated lower bound would already be less than all included values.
+ * We don't need to check the lower bound though: shortening it just
+ * makes it even smaller.
  */
 static bool
 gbt_var_node_pf_match(const GBT_VARKEY_R *node, const bytea *query, const gbtree_vinfo *tinfo)
 {
        return (tinfo->trnc &&
-                       (gbt_bytea_pf_match(node->lower, query, tinfo) ||
-                        gbt_bytea_pf_match(node->upper, query, tinfo)));
+                       gbt_bytea_pf_match(node->upper, query, tinfo));
 }
 
 
@@ -592,57 +590,60 @@ gbt_var_consistent(const GBT_VARKEY_R *key,
                                   const gbtree_vinfo *tinfo,
                                   FmgrInfo *flinfo)
 {
-       bool            retval = false;
+       bool            retval;
 
        /*
-        * Remember that f_cmp is for internal pages, f_eq etc for leaf pages, and
-        * on internal pages we need to check gbt_var_node_pf_match too.
+        * On leaf pages we directly apply the check "key->lower OP query"; we
+        * need not consider key->upper since it will be equal to key->lower.
+        *
+        * On internal pages we mostly need to check "is lower bound below query?"
+        * and/or "is upper bound above query?", where we must allow equality in
+        * both cases.  When checking against the upper bound we have to allow a
+        * gbt_var_node_pf_match too.
         *
-        * The leaf-page tests use swapped operands (e.g., f_gt(query, lower)
-        * means "lower < query"), which is why they look reversed.
+        * Remember that f_cmp is for internal pages, f_eq etc for leaf pages.
         */
+#define lower_is_below_query() \
+       (tinfo->f_cmp(key->lower, query, collation, flinfo) <= 0)
+#define upper_is_above_query() \
+       (tinfo->f_cmp(key->upper, query, collation, flinfo) >= 0 || \
+        gbt_var_node_pf_match(key, query, tinfo))
+
        switch (strategy)
        {
                case BTLessEqualStrategyNumber:
                        if (is_leaf)
-                               retval = tinfo->f_ge(query, key->lower, collation, flinfo);
+                               retval = tinfo->f_le(key->lower, query, collation, flinfo);
                        else
-                               retval = tinfo->f_cmp(query, key->lower, collation, flinfo) >= 0
-                                       || gbt_var_node_pf_match(key, query, tinfo);
+                               retval = lower_is_below_query();
                        break;
                case BTLessStrategyNumber:
                        if (is_leaf)
-                               retval = tinfo->f_gt(query, key->lower, collation, flinfo);
+                               retval = tinfo->f_lt(key->lower, query, collation, flinfo);
                        else
-                               retval = tinfo->f_cmp(query, key->lower, collation, flinfo) >= 0
-                                       || gbt_var_node_pf_match(key, query, tinfo);
+                               retval = lower_is_below_query();
                        break;
                case BTEqualStrategyNumber:
                        if (is_leaf)
-                               retval = tinfo->f_eq(query, key->lower, collation, flinfo);
+                               retval = tinfo->f_eq(key->lower, query, collation, flinfo);
                        else
-                               retval =
-                                       (tinfo->f_cmp(key->lower, query, collation, flinfo) <= 0 &&
-                                        tinfo->f_cmp(query, key->upper, collation, flinfo) <= 0) ||
-                                       gbt_var_node_pf_match(key, query, tinfo);
+                               retval = lower_is_below_query() && upper_is_above_query();
                        break;
                case BTGreaterStrategyNumber:
                        if (is_leaf)
-                               retval = tinfo->f_lt(query, key->upper, collation, flinfo);
+                               retval = tinfo->f_gt(key->lower, query, collation, flinfo);
                        else
-                               retval = tinfo->f_cmp(query, key->upper, collation, flinfo) <= 0
-                                       || gbt_var_node_pf_match(key, query, tinfo);
+                               retval = upper_is_above_query();
                        break;
                case BTGreaterEqualStrategyNumber:
                        if (is_leaf)
-                               retval = tinfo->f_le(query, key->upper, collation, flinfo);
+                               retval = tinfo->f_ge(key->lower, query, collation, flinfo);
                        else
-                               retval = tinfo->f_cmp(query, key->upper, collation, flinfo) <= 0
-                                       || gbt_var_node_pf_match(key, query, tinfo);
+                               retval = upper_is_above_query();
                        break;
                case BtreeGistNotEqualStrategyNumber:
                        if (is_leaf)
-                               retval = !(tinfo->f_eq(query, key->lower, collation, flinfo));
+                               retval = !(tinfo->f_eq(key->lower, query, collation, flinfo));
                        else
                        {
                                /*
@@ -652,13 +653,16 @@ gbt_var_consistent(const GBT_VARKEY_R *key,
                                 * In all other cases, we must descend.
                                 */
                                retval = tinfo->trnc ||
-                                       !(tinfo->f_cmp(query, key->lower, collation, flinfo) == 0 &&
-                                         tinfo->f_cmp(query, key->upper, collation, flinfo) == 0);
+                                       !(tinfo->f_cmp(key->lower, query, collation, flinfo) == 0 &&
+                                         tinfo->f_cmp(key->upper, query, collation, flinfo) == 0);
                        }
                        break;
                default:
                        retval = false;
        }
 
+#undef lower_is_below_query
+#undef upper_is_above_query
+
        return retval;
 }