Re: Hash a ScalarArrayOpExpr whose array is fixed for one execution

> Attached is v3. I have reviewed every line of it myself, changed it
> where needed, and tested it. Please review v3 instead of v2.
>
> My answers to your questions in my last mail are still valid. The
> performance numbers there are also still valid.
>
> 0001 only refactors, with no behavior change. 0002 is the feature and
> its tests. Both apply cleanly to today's master.

Thanks. That helped reviewing 0002.

> Some code in 0002 may be hard to understand, because it handles rare
> cases. For your reference, here is a brief explanation. I hope it
> makes your review easier. Line numbers are in the v3-0002 patch file.
>
> 1. Line 113, use_hashed_step in ExecInitExprRec(): the hashed step
> reads a non-Const array only once. That is wrong in a PL/pgSQL
> simple expression, which is reused with new parameter values, and
> in JSON ON ERROR or ON EMPTY, where errors must be caught. So these
> cases use the plain step.
>
> 2. Line 129, negfuncid in ExecInitExprRec(): a hashed NOT IN looks
> values up with "=" (negfuncid). The plain step must use the
> operator itself, "<>".
>
> 3. Line 379, ExecEvalHashedScalarArrayOp(): on master, this step only
> gets a Const array that the planner has checked: not NULL, and with
> at least 9 elements. A non-Const array is only known at run time,
> so it can be NULL, empty or short. These lines handle that the
> same way as ExecEvalScalarArrayOp(), in the same order, so
> NULL = ANY ('{}') still gives false.
>
> 4. Line 578, saop_array_may_vary_walker(): only arrays built from
> Consts, external Params and the node types listed in the switch are
> hashed. Anything else is rejected, including rare nodes that change
> from row to row: merge_action(), OLD/NEW in RETURNING, and
> set-returning functions. Vars of any level are rejected too,
> because outer references are not yet Params here.

Thanks for the explanations. They were useful. All of them should be
turned into comments in the code.

A comment saying why we cannot unify the Const and non-Const case in
execExprInterp.c would be useful. One could think that it might be a
good idea to now always go the non-Const route for code simplicity /
consistency.

Doesn't saop_array_may_vary_walker() misses T_CaseWhen? T_CaseExpr's
args is a list of CaseWhen nodes. When recursing it doesn't match
against T_CaseWhen and returns true while CaseWhen doesn't vary. I think
that would only occur in this form:

x = ANY(CASE WHEN $2 THEN $1 ELSE '{...}' END

And two nit comments:

- Use castNode() instead of C casts
- Use a wrapper around saop_array_may_vary_walker(), e.g.
saop_array_may_vary()

Beyond that, I would make the commit message less technical. This level
of detail should be code comments. The commit message should high-level
outline the change and its limitations. You could put there e.g. where
the optimization doesn't apply today and where it still doesn't apply
wit the commit's changes (soft errors, standalone expressions).

The rest LGTM.

--
David Geier