On 6/7/2023 03:06, Alena Rybakina wrote:
I corrected this constant in the patch.
The patch don't apply cleanly: it contains some trailing spaces.
Also, quick glance into the code shows some weak points;
1. transformBoolExprOr should have input type BoolExpr.
2. You can avoid the switch operator at the beginning of the function,
because you only need one option.
3. Stale comments: RestrictIinfos definitely not exists at this point.
4. I don't know, you really need to copy the expr or not, but it is
better to do as late, as possible.
5. You assume, that leftop is non-constant and rightop - constant. Why?
6.I doubt about equivalence operator. Someone can invent a custom '='
operator with another semantics, than usual. May be better to check
mergejoinability?
7. I don't know how to confidently identify constant expressions at this
level. So, I guess, You can only merge here expressions like
"F(X)=Const", not an 'F(X)=ConstExpression'.
See delta.diff with mentioned changes in attachment.
--
regards,
Andrey Lepikhov
Postgres Professional
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index c9193d826f..26648b0876 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -107,41 +107,22 @@ typedef struct OrClauseGroupEntry
static int const_transform_or_limit = 500;
static Node *
-transformBoolExprOr(ParseState *pstate, Expr *expr_orig)
+transformBoolExprOr(ParseState *pstate, BoolExpr *expr_orig)
{
List *or_list = NIL;
List *groups_list = NIL;
ListCell *lc_eargs;
Node *result;
- BoolExpr *expr = (BoolExpr *)copyObject(expr_orig);
- const char *opname;
+ BoolExpr *expr;
bool change_apply = false;
- bool or_statement = false;
-
- Assert(IsA(expr, BoolExpr));
/* If this is not expression "Or", then will do it the old way. */
- switch (expr->boolop)
- {
- case AND_EXPR:
- opname = "AND";
- break;
- case OR_EXPR:
- opname = "OR";
- or_statement = true;
- break;
- case NOT_EXPR:
- opname = "NOT";
- break;
- default:
- elog(ERROR, "unrecognized boolop: %d", (int)
expr->boolop);
- opname = NULL; /* keep compiler quiet */
- break;
- }
-
- if (!or_statement || list_length(expr->args) < const_transform_or_limit)
+ if (expr_orig->boolop != OR_EXPR ||
+ list_length(expr_orig->args) < const_transform_or_limit)
return transformBoolExpr(pstate, (BoolExpr *)expr_orig);
+ expr = (BoolExpr *)copyObject(expr_orig);
+
/*
* NOTE:
* It is an OR-clause. So, rinfo->orclause is a BoolExpr node,
contains
@@ -176,15 +157,13 @@ transformBoolExprOr(ParseState *pstate, Expr *expr_orig)
if (!arg)
break;
- allow_transformation = (
- or_statement &&
- arg->type == T_A_Expr && (arg)->kind ==
AEXPR_OP &&
+ allow_transformation = (arg->type == T_A_Expr && (arg)->kind ==
AEXPR_OP &&
list_length((arg)->name) >=1 && strcmp(strVal(linitial((arg)->name)), "=") == 0
);
bare_orarg = transformExprRecurse(pstate, (Node *)arg);
- bare_orarg = coerce_to_boolean(pstate, bare_orarg, opname);
+ bare_orarg = coerce_to_boolean(pstate, bare_orarg, "OR");
/*
* The next step: transform all the inputs, and detect whether
any contain
@@ -357,14 +336,10 @@ transformBoolExprOr(ParseState *pstate, Expr *expr_orig)
}
}
- if (!change_apply)
- {
- or_statement = false;
- }
list_free_deep(groups_list);
}
- if (or_statement)
+ if (change_apply)
{
/* One more trick: assemble correct clause */
expr = list_length(or_list) > 1 ? makeBoolExpr(OR_EXPR,
list_copy(or_list), -1) : linitial(or_list);
@@ -376,9 +351,8 @@ transformBoolExprOr(ParseState *pstate, Expr *expr_orig)
* transformBoolExpr function
*/
else
- {
result = transformBoolExpr(pstate, (BoolExpr *)expr_orig);
- }
+
list_free(or_list);
return result;
@@ -496,7 +470,7 @@ transformExprRecurse(ParseState *pstate, Node *expr)
}
case T_BoolExpr:
- result = (Node *)transformBoolExprOr(pstate, (Expr
*)expr);
+ result = (Node *)transformBoolExprOr(pstate, (BoolExpr
*)expr);
break;
case T_FuncCall:
