Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr - Mailing list pgsql-hackers

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> Eval_const_expressions() doesn't know about ScalarArrayOpExpr.
> ...
> That seems like an oversight. I guess that scenario doesn't happen very 
> often in practice, but there's no reason not to do it when it does. 
> Patch attached.

Yeah, I think it's a lack-of-round-tuits situation.  Your patch reminds
me of a more ambitious attempt I made awhile back to reduce the amount
of duplicative boilerplate in eval_const_expressions.  I think I wrote
it during last year's beta period, stashed it because I didn't feel like
arguing for applying it right then, and then forgot about it.  Blowing
the dust off, it's attached.  It fixes ArrayRef and RowExpr as well as
ScalarArrayOpExpr, with a net growth of only 20 lines (largely comments).

> On a side-note, I find it a bit awkward that ScalarArrayOpExpr uses a 
> 2-element List to hold the scalar and array arguments. Wouldn't it be 
> much more straightforward to have explicit "Expr *scalararg" and "Expr 
> *arrayarg" fields?

I think it's modeled on OpExpr, which also uses a list though you could
argue for separate leftarg and rightarg fields instead.

            regards, tom lane

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index a1dafc8..95489f4 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** static List *find_nonnullable_vars_walke
*** 115,120 ****
--- 115,123 ----
  static bool is_strict_saop(ScalarArrayOpExpr *expr, bool falseOK);
  static Node *eval_const_expressions_mutator(Node *node,
                                 eval_const_expressions_context *context);
+ static bool contain_non_const_walker(Node *node, void *context);
+ static bool ece_function_is_safe(Oid funcid,
+                      eval_const_expressions_context *context);
  static List *simplify_or_arguments(List *args,
                        eval_const_expressions_context *context,
                        bool *haveNull, bool *forceTrue);
*************** estimate_expression_value(PlannerInfo *r
*** 2443,2448 ****
--- 2446,2482 ----
      return eval_const_expressions_mutator(node, &context);
  }

+ /*
+  * The generic case in eval_const_expressions_mutator is to recurse using
+  * expression_tree_mutator, which will copy the given node unchanged but
+  * const-simplify its arguments (if any) as far as possible.  If the node
+  * itself does immutable processing, and each of its arguments were reduced
+  * to a Const, we can then reduce it to a Const using evaluate_expr.  (Some
+  * node types need more complicated logic; for example, a CASE expression
+  * might be reducible to a constant even if not all its subtrees are.)
+  */
+ #define ece_generic_processing(node) \
+     expression_tree_mutator((Node *) (node), eval_const_expressions_mutator, \
+                             (void *) context)
+
+ /*
+  * Check whether all arguments of the given node were reduced to Consts.
+  * By going directly to expression_tree_walker, contain_non_const_walker
+  * is not applied to the node itself, only to its children.
+  */
+ #define ece_all_arguments_const(node) \
+     (!expression_tree_walker((Node *) (node), contain_non_const_walker, NULL))
+
+ /* Generic macro for applying evaluate_expr */
+ #define ece_evaluate_expr(node) \
+     ((Node *) evaluate_expr((Expr *) (node), \
+                             exprType((Node *) (node)), \
+                             exprTypmod((Node *) (node)), \
+                             exprCollation((Node *) (node))))
+
+ /*
+  * Recursive guts of eval_const_expressions/estimate_expression_value
+  */
  static Node *
  eval_const_expressions_mutator(Node *node,
                                 eval_const_expressions_context *context)
*************** eval_const_expressions_mutator(Node *nod
*** 2758,2763 ****
--- 2792,2816 ----
                  newexpr->location = expr->location;
                  return (Node *) newexpr;
              }
+         case T_ScalarArrayOpExpr:
+             {
+                 ScalarArrayOpExpr *saop;
+
+                 /* Copy the node and const-simplify its arguments */
+                 saop = (ScalarArrayOpExpr *) ece_generic_processing(node);
+
+                 /* Make sure we know underlying function */
+                 set_sa_opfuncid(saop);
+
+                 /*
+                  * If all arguments are Consts, and it's a safe function, we
+                  * can fold to a constant
+                  */
+                 if (ece_all_arguments_const(saop) &&
+                     ece_function_is_safe(saop->opfuncid, context))
+                     return ece_evaluate_expr(saop);
+                 return (Node *) saop;
+             }
          case T_BoolExpr:
              {
                  BoolExpr   *expr = (BoolExpr *) node;
*************** eval_const_expressions_mutator(Node *nod
*** 2982,3022 ****
              }
          case T_ArrayCoerceExpr:
              {
!                 ArrayCoerceExpr *expr = (ArrayCoerceExpr *) node;
!                 Expr       *arg;
!                 ArrayCoerceExpr *newexpr;
!
!                 /*
!                  * Reduce constants in the ArrayCoerceExpr's argument, then
!                  * build a new ArrayCoerceExpr.
!                  */
!                 arg = (Expr *) eval_const_expressions_mutator((Node *) expr->arg,
!                                                               context);

!                 newexpr = makeNode(ArrayCoerceExpr);
!                 newexpr->arg = arg;
!                 newexpr->elemfuncid = expr->elemfuncid;
!                 newexpr->resulttype = expr->resulttype;
!                 newexpr->resulttypmod = expr->resulttypmod;
!                 newexpr->resultcollid = expr->resultcollid;
!                 newexpr->isExplicit = expr->isExplicit;
!                 newexpr->coerceformat = expr->coerceformat;
!                 newexpr->location = expr->location;

                  /*
!                  * If constant argument and it's a binary-coercible or
!                  * immutable conversion, we can simplify it to a constant.
                   */
!                 if (arg && IsA(arg, Const) &&
!                     (!OidIsValid(newexpr->elemfuncid) ||
!                 func_volatile(newexpr->elemfuncid) == PROVOLATILE_IMMUTABLE))
!                     return (Node *) evaluate_expr((Expr *) newexpr,
!                                                   newexpr->resulttype,
!                                                   newexpr->resulttypmod,
!                                                   newexpr->resultcollid);
!
!                 /* Else we must return the partially-simplified node */
!                 return (Node *) newexpr;
              }
          case T_CollateExpr:
              {
--- 3035,3054 ----
              }
          case T_ArrayCoerceExpr:
              {
!                 ArrayCoerceExpr *acexpr;

!                 /* Copy the node and const-simplify its arguments */
!                 acexpr = (ArrayCoerceExpr *) ece_generic_processing(node);

                  /*
!                  * If all arguments are Consts, and it's a binary-coercible or
!                  * safe conversion function, we can fold to a constant
                   */
!                 if (ece_all_arguments_const(acexpr) &&
!                     (!OidIsValid(acexpr->elemfuncid) ||
!                      ece_function_is_safe(acexpr->elemfuncid, context)))
!                     return ece_evaluate_expr(acexpr);
!                 return (Node *) acexpr;
              }
          case T_CollateExpr:
              {
*************** eval_const_expressions_mutator(Node *nod
*** 3208,3248 ****
                  else
                      return copyObject(node);
              }
          case T_ArrayExpr:
              {
!                 ArrayExpr  *arrayexpr = (ArrayExpr *) node;
!                 ArrayExpr  *newarray;
!                 bool        all_const = true;
!                 List       *newelems;
!                 ListCell   *element;
!
!                 newelems = NIL;
!                 foreach(element, arrayexpr->elements)
!                 {
!                     Node       *e;
!
!                     e = eval_const_expressions_mutator((Node *) lfirst(element),
!                                                        context);
!                     if (!IsA(e, Const))
!                         all_const = false;
!                     newelems = lappend(newelems, e);
!                 }
!
!                 newarray = makeNode(ArrayExpr);
!                 newarray->array_typeid = arrayexpr->array_typeid;
!                 newarray->array_collid = arrayexpr->array_collid;
!                 newarray->element_typeid = arrayexpr->element_typeid;
!                 newarray->elements = newelems;
!                 newarray->multidims = arrayexpr->multidims;
!                 newarray->location = arrayexpr->location;
!
!                 if (all_const)
!                     return (Node *) evaluate_expr((Expr *) newarray,
!                                                   newarray->array_typeid,
!                                                   exprTypmod(node),
!                                                   newarray->array_collid);

!                 return (Node *) newarray;
              }
          case T_CoalesceExpr:
              {
--- 3240,3262 ----
                  else
                      return copyObject(node);
              }
+         case T_ArrayRef:
          case T_ArrayExpr:
+         case T_RowExpr:
+         case T_BooleanTest:
              {
!                 /*
!                  * Generic handling for node types whose own processing is
!                  * known to be immutable, and for which we need no smarts
!                  * beyond "simplify if all inputs are constants".
!                  */

!                 /* Copy the node and const-simplify its arguments */
!                 node = ece_generic_processing(node);
!                 /* If all arguments are Consts, we can fold to a constant */
!                 if (ece_all_arguments_const(node))
!                     return ece_evaluate_expr(node);
!                 return node;
              }
          case T_CoalesceExpr:
              {
*************** eval_const_expressions_mutator(Node *nod
*** 3319,3325 ****
                   * simple Var.  (This case won't be generated directly by the
                   * parser, because ParseComplexProjection short-circuits it.
                   * But it can arise while simplifying functions.)  Also, we
!                  * can optimize field selection from a RowExpr construct.
                   *
                   * However, replacing a whole-row Var in this way has a
                   * pitfall: if we've already built the rel targetlist for the
--- 3333,3340 ----
                   * simple Var.  (This case won't be generated directly by the
                   * parser, because ParseComplexProjection short-circuits it.
                   * But it can arise while simplifying functions.)  Also, we
!                  * can optimize field selection from a RowExpr construct, or
!                  * of course from a constant.
                   *
                   * However, replacing a whole-row Var in this way has a
                   * pitfall: if we've already built the rel targetlist for the
*************** eval_const_expressions_mutator(Node *nod
*** 3334,3339 ****
--- 3349,3356 ----
                   * We must also check that the declared type of the field is
                   * still the same as when the FieldSelect was created --- this
                   * can change if someone did ALTER COLUMN TYPE on the rowtype.
+                  * If it isn't, we skip the optimization; the case will
+                  * probably fail at runtime, but that's not our problem here.
                   */
                  FieldSelect *fselect = (FieldSelect *) node;
                  FieldSelect *newfselect;
*************** eval_const_expressions_mutator(Node *nod
*** 3384,3389 ****
--- 3401,3417 ----
                  newfselect->resulttype = fselect->resulttype;
                  newfselect->resulttypmod = fselect->resulttypmod;
                  newfselect->resultcollid = fselect->resultcollid;
+                 if (arg && IsA(arg, Const))
+                 {
+                     Const       *con = (Const *) arg;
+
+                     if (rowtype_field_matches(con->consttype,
+                                               newfselect->fieldnum,
+                                               newfselect->resulttype,
+                                               newfselect->resulttypmod,
+                                               newfselect->resultcollid))
+                         return ece_evaluate_expr(newfselect);
+                 }
                  return (Node *) newfselect;
              }
          case T_NullTest:
*************** eval_const_expressions_mutator(Node *nod
*** 3477,3535 ****
                  newntest->location = ntest->location;
                  return (Node *) newntest;
              }
-         case T_BooleanTest:
-             {
-                 BooleanTest *btest = (BooleanTest *) node;
-                 BooleanTest *newbtest;
-                 Node       *arg;
-
-                 arg = eval_const_expressions_mutator((Node *) btest->arg,
-                                                      context);
-                 if (arg && IsA(arg, Const))
-                 {
-                     Const       *carg = (Const *) arg;
-                     bool        result;
-
-                     switch (btest->booltesttype)
-                     {
-                         case IS_TRUE:
-                             result = (!carg->constisnull &&
-                                       DatumGetBool(carg->constvalue));
-                             break;
-                         case IS_NOT_TRUE:
-                             result = (carg->constisnull ||
-                                       !DatumGetBool(carg->constvalue));
-                             break;
-                         case IS_FALSE:
-                             result = (!carg->constisnull &&
-                                       !DatumGetBool(carg->constvalue));
-                             break;
-                         case IS_NOT_FALSE:
-                             result = (carg->constisnull ||
-                                       DatumGetBool(carg->constvalue));
-                             break;
-                         case IS_UNKNOWN:
-                             result = carg->constisnull;
-                             break;
-                         case IS_NOT_UNKNOWN:
-                             result = !carg->constisnull;
-                             break;
-                         default:
-                             elog(ERROR, "unrecognized booltesttype: %d",
-                                  (int) btest->booltesttype);
-                             result = false;        /* keep compiler quiet */
-                             break;
-                     }
-
-                     return makeBoolConst(result, false);
-                 }
-
-                 newbtest = makeNode(BooleanTest);
-                 newbtest->arg = (Expr *) arg;
-                 newbtest->booltesttype = btest->booltesttype;
-                 newbtest->location = btest->location;
-                 return (Node *) newbtest;
-             }
          case T_PlaceHolderVar:

              /*
--- 3505,3510 ----
*************** eval_const_expressions_mutator(Node *nod
*** 3552,3565 ****
      }

      /*
!      * For any node type not handled above, we recurse using
!      * expression_tree_mutator, which will copy the node unchanged but try to
!      * simplify its arguments (if any) using this routine. For example: we
!      * cannot eliminate an ArrayRef node, but we might be able to simplify
!      * constant expressions in its subscripts.
       */
!     return expression_tree_mutator(node, eval_const_expressions_mutator,
!                                    (void *) context);
  }

  /*
--- 3527,3583 ----
      }

      /*
!      * For any node type not handled above, copy the node unchanged but
!      * const-simplify its subexpressions.  This is the correct thing for node
!      * types whose behavior might change between planning and execution, such
!      * as CoerceToDomain.  It's also a safe default for new node types not
!      * known to this routine.
       */
!     return ece_generic_processing(node);
! }
!
! /*
!  * Subroutine for eval_const_expressions: check for non-Const nodes.
!  *
!  * We can abort recursion immediately on finding a non-Const node.  This is
!  * critical for performance, else eval_const_expressions_mutator would take
!  * O(N^2) time on non-simplifiable trees.  However, we do need to descend
!  * into List nodes since expression_tree_walker sometimes invokes the walker
!  * function directly on List subtrees.
!  */
! static bool
! contain_non_const_walker(Node *node, void *context)
! {
!     if (node == NULL)
!         return false;
!     if (IsA(node, Const))
!         return false;
!     if (IsA(node, List))
!         return expression_tree_walker(node, contain_non_const_walker, context);
!     /* Otherwise, abort the tree traversal and return true */
!     return true;
! }
!
! /*
!  * Subroutine for eval_const_expressions: check if a function is OK to evaluate
!  */
! static bool
! ece_function_is_safe(Oid funcid, eval_const_expressions_context *context)
! {
!     char        provolatile = func_volatile(funcid);
!
!     /*
!      * Ordinarily we are only allowed to simplify immutable functions. But for
!      * purposes of estimation, we consider it okay to simplify functions that
!      * are merely stable; the risk that the result might change from planning
!      * time to execution time is worth taking in preference to not being able
!      * to estimate the value at all.
!      */
!     if (provolatile == PROVOLATILE_IMMUTABLE)
!         return true;
!     if (context->estimate && provolatile == PROVOLATILE_STABLE)
!         return true;
!     return false;
  }

  /*
diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out
index 43b36f6..a4bac8e 100644
*** a/src/test/regress/expected/rowtypes.out
--- b/src/test/regress/expected/rowtypes.out
*************** ERROR:  cannot compare dissimilar column
*** 307,316 ****
  explain (costs off)
  select * from int8_tbl i8
  where i8 in (row(123,456)::int8_tbl, '(4567890123456789,123)');
!                                                    QUERY PLAN
! -----------------------------------------------------------------------------------------------------------------
   Seq Scan on int8_tbl i8
!    Filter: (i8.* = ANY (ARRAY[ROW('123'::bigint, '456'::bigint)::int8_tbl, '(4567890123456789,123)'::int8_tbl]))
  (2 rows)

  select * from int8_tbl i8
--- 307,316 ----
  explain (costs off)
  select * from int8_tbl i8
  where i8 in (row(123,456)::int8_tbl, '(4567890123456789,123)');
!                                   QUERY PLAN
! -------------------------------------------------------------------------------
   Seq Scan on int8_tbl i8
!    Filter: (i8.* = ANY ('{"(123,456)","(4567890123456789,123)"}'::int8_tbl[]))
  (2 rows)

  select * from int8_tbl i8

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers