From cb1cc305bc349338f75a549c36041b4c91cf779f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 6 Jul 2011 14:53:16 -0400 Subject: [PATCH] Remove assumptions that not-equals operators cannot be in any opclass. get_op_btree_interpretation assumed this in order to save some duplication of code, but it's not true in general anymore because we added <> support to btree_gist. (We still assume it for btree opclasses, though.) Also, essentially the same logic was baked into predtest.c. Get rid of that duplication by generalizing get_op_btree_interpretation so that it can be used by predtest.c. Per bug report from Denis de Bernardy and investigation by Jeff Davis, though I didn't use Jeff's patch exactly as-is. Back-patch to 9.1; we do not support this usage before that. --- src/backend/optimizer/util/predtest.c | 191 +++++++++----------------- src/backend/parser/parse_expr.c | 26 ++-- src/backend/utils/cache/lsyscache.c | 97 ++++++++----- src/include/utils/lsyscache.h | 12 +- 4 files changed, 145 insertions(+), 181 deletions(-) diff --git a/src/backend/optimizer/util/predtest.c b/src/backend/optimizer/util/predtest.c index a7e83729b1..bbfbc7947b 100644 --- a/src/backend/optimizer/util/predtest.c +++ b/src/backend/optimizer/util/predtest.c @@ -1249,6 +1249,7 @@ list_member_strip(List *list, Expr *datum) * and in addition we use (6) to represent <>. <> is not a btree-indexable * operator, but we assume here that if an equality operator of a btree * opfamily has a negator operator, the negator behaves as <> for the opfamily. + * (This convention is also known to get_op_btree_interpretation().) * * The interpretation of: * @@ -1285,7 +1286,7 @@ list_member_strip(List *list, Expr *datum) #define BTEQ BTEqualStrategyNumber #define BTGE BTGreaterEqualStrategyNumber #define BTGT BTGreaterStrategyNumber -#define BTNE 6 +#define BTNE ROWCOMPARE_NE static const StrategyNumber BT_implic_table[6][6] = { /* @@ -1556,18 +1557,12 @@ get_btree_test_op(Oid pred_op, Oid clause_op, bool refute_it) OprProofCacheKey key; OprProofCacheEntry *cache_entry; bool cfound; - bool pred_op_negated; - Oid pred_op_negator, - clause_op_negator, - test_op = InvalidOid; - Oid opfamily_id; + Oid test_op = InvalidOid; bool found = false; - StrategyNumber pred_strategy, - clause_strategy, - test_strategy; - Oid clause_righttype; - CatCList *catlist; - int i; + List *pred_op_infos, + *clause_op_infos; + ListCell *lcp, + *lcc; /* * Find or make a cache entry for this pair of operators. @@ -1628,135 +1623,71 @@ get_btree_test_op(Oid pred_op, Oid clause_op, bool refute_it) * corresponding test operator. This should work for any logically * consistent opfamilies. */ - catlist = SearchSysCacheList1(AMOPOPID, ObjectIdGetDatum(pred_op)); + clause_op_infos = get_op_btree_interpretation(clause_op); + if (clause_op_infos) + pred_op_infos = get_op_btree_interpretation(pred_op); + else /* no point in looking */ + pred_op_infos = NIL; - /* - * If we couldn't find any opfamily containing the pred_op, perhaps it is - * a <> operator. See if it has a negator that is in an opfamily. - */ - pred_op_negated = false; - if (catlist->n_members == 0) + foreach(lcp, pred_op_infos) { - pred_op_negator = get_negator(pred_op); - if (OidIsValid(pred_op_negator)) - { - pred_op_negated = true; - ReleaseSysCacheList(catlist); - catlist = SearchSysCacheList1(AMOPOPID, - ObjectIdGetDatum(pred_op_negator)); - } - } + OpBtreeInterpretation *pred_op_info = lfirst(lcp); + Oid opfamily_id = pred_op_info->opfamily_id; - /* Also may need the clause_op's negator */ - clause_op_negator = get_negator(clause_op); + foreach(lcc, clause_op_infos) + { + OpBtreeInterpretation *clause_op_info = lfirst(lcc); + StrategyNumber pred_strategy, + clause_strategy, + test_strategy; - /* Now search the opfamilies */ - for (i = 0; i < catlist->n_members; i++) - { - HeapTuple pred_tuple = &catlist->members[i]->tuple; - Form_pg_amop pred_form = (Form_pg_amop) GETSTRUCT(pred_tuple); - HeapTuple clause_tuple; + /* Must find them in same opfamily */ + if (opfamily_id != clause_op_info->opfamily_id) + continue; + /* Lefttypes should match */ + Assert(clause_op_info->oplefttype == pred_op_info->oplefttype); - /* Must be btree */ - if (pred_form->amopmethod != BTREE_AM_OID) - continue; + pred_strategy = pred_op_info->strategy; + clause_strategy = clause_op_info->strategy; - /* Get the predicate operator's btree strategy number */ - opfamily_id = pred_form->amopfamily; - pred_strategy = (StrategyNumber) pred_form->amopstrategy; - Assert(pred_strategy >= 1 && pred_strategy <= 5); + /* + * Look up the "test" strategy number in the implication table + */ + if (refute_it) + test_strategy = BT_refute_table[clause_strategy - 1][pred_strategy - 1]; + else + test_strategy = BT_implic_table[clause_strategy - 1][pred_strategy - 1]; - if (pred_op_negated) - { - /* Only consider negators that are = */ - if (pred_strategy != BTEqualStrategyNumber) + if (test_strategy == 0) + { + /* Can't determine implication using this interpretation */ continue; - pred_strategy = BTNE; - } + } - /* - * From the same opfamily, find a strategy number for the clause_op, - * if possible - */ - clause_tuple = SearchSysCache3(AMOPOPID, - ObjectIdGetDatum(clause_op), - CharGetDatum(AMOP_SEARCH), - ObjectIdGetDatum(opfamily_id)); - if (HeapTupleIsValid(clause_tuple)) - { - Form_pg_amop clause_form = (Form_pg_amop) GETSTRUCT(clause_tuple); - - /* Get the restriction clause operator's strategy/datatype */ - clause_strategy = (StrategyNumber) clause_form->amopstrategy; - Assert(clause_strategy >= 1 && clause_strategy <= 5); - Assert(clause_form->amoplefttype == pred_form->amoplefttype); - clause_righttype = clause_form->amoprighttype; - ReleaseSysCache(clause_tuple); - } - else if (OidIsValid(clause_op_negator)) - { - clause_tuple = SearchSysCache3(AMOPOPID, - ObjectIdGetDatum(clause_op_negator), - CharGetDatum(AMOP_SEARCH), - ObjectIdGetDatum(opfamily_id)); - if (HeapTupleIsValid(clause_tuple)) + /* + * See if opfamily has an operator for the test strategy and the + * datatypes. + */ + if (test_strategy == BTNE) { - Form_pg_amop clause_form = (Form_pg_amop) GETSTRUCT(clause_tuple); - - /* Get the restriction clause operator's strategy/datatype */ - clause_strategy = (StrategyNumber) clause_form->amopstrategy; - Assert(clause_strategy >= 1 && clause_strategy <= 5); - Assert(clause_form->amoplefttype == pred_form->amoplefttype); - clause_righttype = clause_form->amoprighttype; - ReleaseSysCache(clause_tuple); - - /* Only consider negators that are = */ - if (clause_strategy != BTEqualStrategyNumber) - continue; - clause_strategy = BTNE; + test_op = get_opfamily_member(opfamily_id, + pred_op_info->oprighttype, + clause_op_info->oprighttype, + BTEqualStrategyNumber); + if (OidIsValid(test_op)) + test_op = get_negator(test_op); } else - continue; - } - else - continue; - - /* - * Look up the "test" strategy number in the implication table - */ - if (refute_it) - test_strategy = BT_refute_table[clause_strategy - 1][pred_strategy - 1]; - else - test_strategy = BT_implic_table[clause_strategy - 1][pred_strategy - 1]; + { + test_op = get_opfamily_member(opfamily_id, + pred_op_info->oprighttype, + clause_op_info->oprighttype, + test_strategy); + } - if (test_strategy == 0) - { - /* Can't determine implication using this interpretation */ - continue; - } + if (!OidIsValid(test_op)) + continue; - /* - * See if opfamily has an operator for the test strategy and the - * datatypes. - */ - if (test_strategy == BTNE) - { - test_op = get_opfamily_member(opfamily_id, - pred_form->amoprighttype, - clause_righttype, - BTEqualStrategyNumber); - if (OidIsValid(test_op)) - test_op = get_negator(test_op); - } - else - { - test_op = get_opfamily_member(opfamily_id, - pred_form->amoprighttype, - clause_righttype, - test_strategy); - } - if (OidIsValid(test_op)) - { /* * Last check: test_op must be immutable. * @@ -1772,9 +1703,13 @@ get_btree_test_op(Oid pred_op, Oid clause_op, bool refute_it) break; } } + + if (found) + break; } - ReleaseSysCacheList(catlist); + list_free_deep(pred_op_infos); + list_free_deep(clause_op_infos); if (!found) { diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 08f0439e7e..65d03adc49 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -2170,8 +2170,7 @@ make_row_comparison_op(ParseState *pstate, List *opname, List *opfamilies; ListCell *l, *r; - List **opfamily_lists; - List **opstrat_lists; + List **opinfo_lists; Bitmapset *strats; int nopers; int i; @@ -2241,8 +2240,7 @@ make_row_comparison_op(ParseState *pstate, List *opname, * containing the operators, and see which interpretations (strategy * numbers) exist for each operator. */ - opfamily_lists = (List **) palloc(nopers * sizeof(List *)); - opstrat_lists = (List **) palloc(nopers * sizeof(List *)); + opinfo_lists = (List **) palloc(nopers * sizeof(List *)); strats = NULL; i = 0; foreach(l, opexprs) @@ -2251,17 +2249,18 @@ make_row_comparison_op(ParseState *pstate, List *opname, Bitmapset *this_strats; ListCell *j; - get_op_btree_interpretation(opno, - &opfamily_lists[i], &opstrat_lists[i]); + opinfo_lists[i] = get_op_btree_interpretation(opno); /* - * convert strategy number list to a Bitmapset to make the + * convert strategy numbers into a Bitmapset to make the * intersection calculation easy. */ this_strats = NULL; - foreach(j, opstrat_lists[i]) + foreach(j, opinfo_lists[i]) { - this_strats = bms_add_member(this_strats, lfirst_int(j)); + OpBtreeInterpretation *opinfo = lfirst(j); + + this_strats = bms_add_member(this_strats, opinfo->strategy); } if (i == 0) strats = this_strats; @@ -2309,14 +2308,15 @@ make_row_comparison_op(ParseState *pstate, List *opname, for (i = 0; i < nopers; i++) { Oid opfamily = InvalidOid; + ListCell *j; - forboth(l, opfamily_lists[i], r, opstrat_lists[i]) + foreach(j, opinfo_lists[i]) { - int opstrat = lfirst_int(r); + OpBtreeInterpretation *opinfo = lfirst(j); - if (opstrat == rctype) + if (opinfo->strategy == rctype) { - opfamily = lfirst_oid(l); + opfamily = opinfo->opfamily_id; break; } } diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c index 28d18b0d32..a59bb1bc1a 100644 --- a/src/backend/utils/cache/lsyscache.c +++ b/src/backend/utils/cache/lsyscache.c @@ -622,52 +622,30 @@ get_op_hash_functions(Oid opno, /* * get_op_btree_interpretation * Given an operator's OID, find out which btree opfamilies it belongs to, - * and what strategy number it has within each one. The results are - * returned as an OID list and a parallel integer list. + * and what properties it has within each one. The results are returned + * as a palloc'd list of OpBtreeInterpretation structs. * * In addition to the normal btree operators, we consider a <> operator to be * a "member" of an opfamily if its negator is an equality operator of the * opfamily. ROWCOMPARE_NE is returned as the strategy number for this case. */ -void -get_op_btree_interpretation(Oid opno, List **opfamilies, List **opstrats) +List * +get_op_btree_interpretation(Oid opno) { + List *result = NIL; + OpBtreeInterpretation *thisresult; CatCList *catlist; - bool op_negated; int i; - *opfamilies = NIL; - *opstrats = NIL; - /* * Find all the pg_amop entries containing the operator. */ catlist = SearchSysCacheList1(AMOPOPID, ObjectIdGetDatum(opno)); - /* - * If we can't find any opfamily containing the op, perhaps it is a <> - * operator. See if it has a negator that is in an opfamily. - */ - op_negated = false; - if (catlist->n_members == 0) - { - Oid op_negator = get_negator(opno); - - if (OidIsValid(op_negator)) - { - op_negated = true; - ReleaseSysCacheList(catlist); - catlist = SearchSysCacheList1(AMOPOPID, - ObjectIdGetDatum(op_negator)); - } - } - - /* Now search the opfamilies */ for (i = 0; i < catlist->n_members; i++) { HeapTuple op_tuple = &catlist->members[i]->tuple; Form_pg_amop op_form = (Form_pg_amop) GETSTRUCT(op_tuple); - Oid opfamily_id; StrategyNumber op_strategy; /* must be btree */ @@ -675,23 +653,66 @@ get_op_btree_interpretation(Oid opno, List **opfamilies, List **opstrats) continue; /* Get the operator's btree strategy number */ - opfamily_id = op_form->amopfamily; op_strategy = (StrategyNumber) op_form->amopstrategy; Assert(op_strategy >= 1 && op_strategy <= 5); - if (op_negated) + thisresult = (OpBtreeInterpretation *) + palloc(sizeof(OpBtreeInterpretation)); + thisresult->opfamily_id = op_form->amopfamily; + thisresult->strategy = op_strategy; + thisresult->oplefttype = op_form->amoplefttype; + thisresult->oprighttype = op_form->amoprighttype; + result = lappend(result, thisresult); + } + + ReleaseSysCacheList(catlist); + + /* + * If we didn't find any btree opfamily containing the operator, perhaps + * it is a <> operator. See if it has a negator that is in an opfamily. + */ + if (result == NIL) + { + Oid op_negator = get_negator(opno); + + if (OidIsValid(op_negator)) { - /* Only consider negators that are = */ - if (op_strategy != BTEqualStrategyNumber) - continue; - op_strategy = ROWCOMPARE_NE; - } + catlist = SearchSysCacheList1(AMOPOPID, + ObjectIdGetDatum(op_negator)); - *opfamilies = lappend_oid(*opfamilies, opfamily_id); - *opstrats = lappend_int(*opstrats, op_strategy); + for (i = 0; i < catlist->n_members; i++) + { + HeapTuple op_tuple = &catlist->members[i]->tuple; + Form_pg_amop op_form = (Form_pg_amop) GETSTRUCT(op_tuple); + StrategyNumber op_strategy; + + /* must be btree */ + if (op_form->amopmethod != BTREE_AM_OID) + continue; + + /* Get the operator's btree strategy number */ + op_strategy = (StrategyNumber) op_form->amopstrategy; + Assert(op_strategy >= 1 && op_strategy <= 5); + + /* Only consider negators that are = */ + if (op_strategy != BTEqualStrategyNumber) + continue; + + /* OK, report it with "strategy" ROWCOMPARE_NE */ + thisresult = (OpBtreeInterpretation *) + palloc(sizeof(OpBtreeInterpretation)); + thisresult->opfamily_id = op_form->amopfamily; + thisresult->strategy = ROWCOMPARE_NE; + thisresult->oplefttype = op_form->amoplefttype; + thisresult->oprighttype = op_form->amoprighttype; + result = lappend(result, thisresult); + } + + ReleaseSysCacheList(catlist); + } } - ReleaseSysCacheList(catlist); + return result; } /* diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h index 0a419dcf65..f4490adc31 100644 --- a/src/include/utils/lsyscache.h +++ b/src/include/utils/lsyscache.h @@ -17,6 +17,15 @@ #include "access/htup.h" #include "nodes/pg_list.h" +/* Result list element for get_op_btree_interpretation */ +typedef struct OpBtreeInterpretation +{ + Oid opfamily_id; /* btree opfamily containing operator */ + int strategy; /* its strategy number */ + Oid oplefttype; /* declared left input datatype */ + Oid oprighttype; /* declared right input datatype */ +} OpBtreeInterpretation; + /* I/O function selector for get_type_io_data */ typedef enum IOFuncSelector { @@ -50,8 +59,7 @@ extern bool get_compatible_hash_operators(Oid opno, Oid *lhs_opno, Oid *rhs_opno); extern bool get_op_hash_functions(Oid opno, RegProcedure *lhs_procno, RegProcedure *rhs_procno); -extern void get_op_btree_interpretation(Oid opno, - List **opfamilies, List **opstrats); +extern List *get_op_btree_interpretation(Oid opno); extern bool equality_ops_are_compatible(Oid opno1, Oid opno2); extern Oid get_opfamily_proc(Oid opfamily, Oid lefttype, Oid righttype, int16 procnum); -- 2.40.0