This is an automated email from the ASF dual-hosted git repository. maxyang pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/cloudberry.git
commit 68cba7eda1833d36b2e0f69aae6808aa20fc5332 Author: xuejing zhao <[email protected]> AuthorDate: Thu Dec 1 10:08:23 2022 +0800 (main only): fix wrong results caused by over-eager constraint exclusion (#14553) When checking relation exclusion by constraints, GPDB performs further checks that if the predicate is of format expr=constant, try to evaluate the clause by replacing every occurrence of expr with the constant. But, this does not work for all cases. the related issues are as follows: remove simple_equality_predicate_refuted to fix wrong results caused by over-eager constraint exclusion. --- src/backend/optimizer/util/clauses.c | 43 --------- src/backend/optimizer/util/predtest.c | 174 ---------------------------------- src/include/optimizer/clauses.h | 2 - src/test/regress/expected/gporca.out | 3 +- 4 files changed, 2 insertions(+), 220 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 1c851a5997..c79268a164 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -5570,49 +5570,6 @@ flatten_join_alias_var_optimizer(Query *query, int queryLevel) return queryNew; } -/** - * Returns true if the equality operator with the given opno - * values is a true equality operator (unlike some of the - * box/rectangle/etc. types which implement 'goofy' operators). - * - * This function currently only knows about built-in types, and is - * conservative with regard to which it returns true for (only - * ones which have been verified to work the right way). - */ -bool is_builtin_true_equality_between_same_type(int opno) -{ - switch (opno) - { - case BitEqualOperator: - case BooleanEqualOperator: - case BpcharEqualOperator: - case CashEqualOperator: - case CharEqualOperator: - case CidEqualOperator: - case DateEqualOperator: - case Float4EqualOperator: - case Float8EqualOperator: - case Int2EqualOperator: - case Int4EqualOperator: - case Int8EqualOperator: - case IntervalEqualOperator: - case NameEqualOperator: - case NumericEqualOperator: - case OidEqualOperator: - case TextEqualOperator: - case TIDEqualOperator: - case TimeEqualOperator: - case TimestampEqualOperator: - case TimestampTZEqualOperator: - case TimeTZEqualOperator: - case XidEqualOperator: - return true; - - default: - return false; - } -} - /** * Structs and Methods to support searching of matching subexpressions. */ diff --git a/src/backend/optimizer/util/predtest.c b/src/backend/optimizer/util/predtest.c index 010abda223..9b772e5985 100644 --- a/src/backend/optimizer/util/predtest.c +++ b/src/backend/optimizer/util/predtest.c @@ -35,8 +35,6 @@ #include "optimizer/paths.h" #include "optimizer/predtest_valueset.h" -static const bool kUseFnEvaluationForPredicates = true; - /* * Proof attempts involving large arrays in ScalarArrayOpExpr nodes are * likely to require O(N^2) time, and more often than not fail anyway. @@ -118,9 +116,6 @@ static bool operator_same_subexprs_lookup(Oid pred_op, Oid clause_op, static Oid get_btree_test_op(Oid pred_op, Oid clause_op, bool refute_it); static void InvalidateOprProofCacheCallBack(Datum arg, int cacheid, uint32 hashvalue); - -static bool simple_equality_predicate_refuted(Node *clause, Node *predicate); - /* * predicate_implied_by * Recursively checks whether the clauses in clause_list imply that the @@ -257,10 +252,6 @@ predicate_refuted_by(List *predicate_list, List *clause_list, /* And away we go ... */ if ( predicate_refuted_by_recurse(c, p, weak)) return true; - - if ( ! kUseFnEvaluationForPredicates ) - return false; - return simple_equality_predicate_refuted((Node*)clause_list, (Node*)predicate_list); } /*---------- @@ -1257,171 +1248,6 @@ predicate_refuted_by_simple_clause(Expr *predicate, Node *clause, return operator_predicate_proof(predicate, clause, true, weak); } -/** - * If n is a List, then return an AND tree of the nodes of the list - * Otherwise return n. - */ -static Node * -convertToExplicitAndsShallowly( Node *n) -{ - if ( IsA(n, List)) - { - ListCell *cell; - List *list = (List*)n; - Node *result = NULL; - - Assert(list_length(list) != 0 ); - - foreach( cell, list ) - { - Node *value = (Node*) lfirst(cell); - if ( result == NULL) - { - result = value; - } - else - { - result = (Node *) makeBoolExpr(AND_EXPR, list_make2(result, value), -1 /* parse location */); - } - } - return result; - } - else return n; -} - -/** - * Check to see if the predicate is expr=constant or constant=expr. In that case, try to evaluate the clause - * by replacing every occurrence of expr with the constant. If the clause can then be reduced to FALSE, we - * conclude that the expression is refuted - * - * Returns true only if evaluation is possible AND expression is refuted based on evaluation results - * - * MPP-18979: - * This mechanism cannot be used to prove implication. One example expression is - * "F(x)=1 and x=2", where F(x) is an immutable function that returns 1 for any input x. - * In this case, replacing x with 2 produces F(2)=1 and 2=2. Although evaluating the resulting - * expression gives TRUE, we cannot conclude that (x=2) is implied by the whole expression. - * - */ -static bool -simple_equality_predicate_refuted(Node *clause, Node *predicate) -{ - OpExpr *predicateExpr; - Node *leftPredicateOp, *rightPredicateOp; - Node *constExprInPredicate, *varExprInPredicate; - List *list; - - /* BEGIN inspecting the predicate: this only works for a simple equality predicate */ - if ( nodeTag(predicate) != T_List ) - return false; - - if ( clause == predicate ) - return false; /* don't both doing for self-refutation ... let normal behavior handle that */ - - list = (List *) predicate; - if ( list_length(list) != 1 ) - return false; - - predicate = linitial(list); - if ( ! is_opclause(predicate)) - return false; - - predicateExpr = (OpExpr*) predicate; - leftPredicateOp = get_leftop((Expr*)predicate); - rightPredicateOp = get_rightop((Expr*)predicate); - if (!leftPredicateOp || !rightPredicateOp) - return false; - - /* check if it's equality operation */ - if ( ! is_builtin_true_equality_between_same_type(predicateExpr->opno)) - return false; - - /* check if one operand is a constant */ - if ( IsA(rightPredicateOp, Const)) - { - varExprInPredicate = leftPredicateOp; - constExprInPredicate = rightPredicateOp; - } - else if ( IsA(leftPredicateOp, Const)) - { - constExprInPredicate = leftPredicateOp; - varExprInPredicate = rightPredicateOp; - } - else - { - return false; - } - - if ( IsA(varExprInPredicate, RelabelType)) - { - RelabelType *rt = (RelabelType*) varExprInPredicate; - varExprInPredicate = (Node*) rt->arg; - } - - if ( ! IsA(varExprInPredicate, Var)) - { - /* for now, this code is targeting predicates used in value partitions ... - * so don't apply it for other expressions. This check can probably - * simply be removed and some test cases built. */ - return false; - } - - /* DONE inspecting the predicate */ - - /* clause may have non-immutable functions...don't eval if that's the case: - * - * Note that since we are replacing elements of the clause that match - * varExprInPredicate, there is no need to also check varExprInPredicate - * for mutable functions (note that this is only relevant when the - * earlier check for varExprInPredicate being a Var is removed. - */ - if ( contain_mutable_functions(clause)) - return false; - - /* now do the evaluation */ - { - Node *newClause, *reducedExpression; - ReplaceExpressionMutatorReplacement replacement; - bool result = false; - MemoryContext old_context; - MemoryContext tmp_context; - - replacement.replaceThis = varExprInPredicate; - replacement.withThis = constExprInPredicate; - replacement.numReplacementsDone = 0; - - tmp_context = AllocSetContextCreate(CurrentMemoryContext, - "Predtest", - ALLOCSET_DEFAULT_MINSIZE, - ALLOCSET_DEFAULT_INITSIZE, - ALLOCSET_DEFAULT_MAXSIZE); - - old_context = MemoryContextSwitchTo(tmp_context); - - newClause = replace_expression_mutator(clause, &replacement); - - if ( replacement.numReplacementsDone > 0) - { - newClause = convertToExplicitAndsShallowly(newClause); - reducedExpression = eval_const_expressions(NULL, newClause); - - if ( IsA(reducedExpression, Const )) - { - Const *c = (Const *) reducedExpression; - if ( c->consttype == BOOLOID && - ! c->constisnull ) - { - result = (DatumGetBool(c->constvalue) == false); - } - } - } - - MemoryContextSwitchTo(old_context); - MemoryContextDelete(tmp_context); - return result; - } -} - /* * If clause asserts the non-truth of a subclause, return that subclause; * otherwise return NULL. diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h index 85d90a5bb6..9bd16403a5 100644 --- a/src/include/optimizer/clauses.h +++ b/src/include/optimizer/clauses.h @@ -73,8 +73,6 @@ extern Query *inline_set_returning_function(PlannerInfo *root, extern Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, Oid result_collation); -extern bool is_builtin_true_equality_between_same_type(int opno); - extern bool subexpression_match(Expr *expr1, Expr *expr2); // resolve the join alias varno/varattno information to its base varno/varattno information diff --git a/src/test/regress/expected/gporca.out b/src/test/regress/expected/gporca.out index 9d33dc28b0..a9c968211c 100644 --- a/src/test/regress/expected/gporca.out +++ b/src/test/regress/expected/gporca.out @@ -11555,7 +11555,8 @@ INSERT INTO tc0 VALUES (NULL); SELECT * from tc0 where a IS NULL; a --- -(0 rows) + +(1 row) CREATE TABLE tc1 (a int check (a between 1 and 2 or a != 3 and a > 5)); NOTICE: Table doesn't have 'DISTRIBUTED BY' clause -- Using column named 'a' as the Cloudberry Database data distribution key for this table. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
