From: Tom Lane Date: Fri, 3 Jul 2026 19:25:19 +0000 (-0400) Subject: Tighten up btree_gist's handling of truncated bounds. X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=fea9c1884b2009a94287989e961d6493e22bf656;p=thirdparty%2Fpostgresql.git 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 Reviewed-by: Ayush Tiwari 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 79c6223f99c..971485dc8eb 100644 --- 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 6ab2ca263ef..ae101801355 100644 --- 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; }