>> 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.
>
> Done, above saop_hash_eval_array(). The non-Const route copies the
> array, and a Const array needs no copy, because it is part of the plan.
But we could copy it, couldn't we? I'm more looking for a reason like
performance. As in, if we unified the code, what would happen that we
don't want to happen? Because we would get rid of a bunch of if-else and
harder to understand code if also Const arrays went through array_expr.
>> 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
>
> This form is hashed. The walker never gets a CaseWhen node, so
> T_CaseWhen is not needed. I went through expression_tree_walker() and
> ran a test to confirm this.
Ah, this is because raw_expression_tree_walker_impl() skips T_WhenExpr
and directly recurses into its contents. You could add the case with a
comment to be on the safe side, if the code ever gets changed.
>> 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).
I thought that part below from v1 would be good to preserve in the
commit message:
"Since PG 14 (50e17ad281) a ScalarArrayOpExpr with "useOr" can be
evaluated with a hash table instead of a linear scan of the array, but
only when the array argument is a Const. In a generic plan, a
parameterised list -- "col = ANY($1)", "col IN ($1, $2, ..., $N)",
"col = ANY($1::int[])", "col = ANY(string_to_array($1, ','))" -- keeps
a Param / ArrayExpr / FuncExpr in the plan and falls back to the
O(rows * N) linear path; so does an array computed by a stable function,
in any plan. That is the shape emitted by JDBC setArray, psycopg and
asyncpg, and by any driver that expands an IN list into bind
parameters, and a prepared statement switches to its generic plan after
five executions when that plan looks cheaper. MySQL and MariaDB
binary-search their sorted in_vector for the same query and stay
sub-linear."
This gives all the detail around when it doesn't work today, which is
very useful. Incorporating this into the commit message of v4 would be good.
> Any further feedback or guidance would be much appreciated.
I think you can go ahead and open a commit fest entry.
--
David Geier
