| From: | Ayoub Kazar <ayoub(dot)kazar(at)data-bene(dot)io> |
|---|---|
| To: | David Geier <geidav(dot)pg(at)gmail(dot)com>, best xmg <bestxmg(at)gmail(dot)com> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: Hash a ScalarArrayOpExpr whose array is fixed for one execution |
| Date: | 2026-10-30 17:56:51 |
| Message-ID: | 349c7d07-2d83-40c6-84a7-c92ee3d9bb87@data-bene.io |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hello Linden, David,
On 29/10/2026 10:46, David Geier wrote:
>>> 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.
+1 to simplifying the code, but we should be careful about hot-path
overhead. In master, Const arrays are pre-validated at plan time,
so ExecEvalHashedScalarArrayOp runs without checking for NULL, empty, or
short arrays on every tuple (aren't those for the whole execution? so
why check every time?). In v4, having this in the inner loop for every
row is not insignificant.
>
>>> 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.
AI suggests also NamedArtExpr and CollateExpr, i think they are safe as
well to allow.
Other findings:
While it is true that val <> ALL(array) == NOT(val = ANY (array)) (in
Logic), however for user defined types and operators that are not
strict, comparing NULL with a value can return false for both = and <> ...
So this makes the equivalence in that case not always true. (Attached is
a reprod of the bug)
>> Open question
>> ------------
>> cost_qual_eval charges the hashed cost for an opaque stable array using
>> estimate_array_length()'s default guess, which slightly over-estimates
>> startup for an array that turns out short at run time. I have left that
>> for a follow-up; happy to fold in a fix if reviewers prefer.
> What are your ideas for improving on that? You cannot know the runtime
> array length at planning time.
We should be cautious about charging an O(1) per-tuple cost during
generic planning when the array length is unknown, given that the
executor can unilaterally decide to abandon hashing and execute an O(N)
linear scan instead.
Here's a case:
CREATE TABLE large_bench (id bigint, cat int, val text);
INSERT INTO large_bench SELECT g, (g / 100) % 11, repeat('x', 100) FROM
generate_series(1, 5000000) g;
CREATE INDEX idx_large_bench_cat ON large_bench(cat);
ANALYZE large_bench;
PREPARE p_query(int[]) AS SELECT * FROM large_bench WHERE cat = ANY($1);
SET plan_cache_mode = force_generic_plan;
Optimizer Costs:
Master: Bitmap Index Scan (187,282) < Seq Scan (203,410)
Patched v4: Seq Scan (165,910) < Bitmap Index Scan (187,282) (because of
the cheap cost per tuple)
test executing with |ARRAY[1, 2]|:
Cold Cache:
Patched Seq Scan: 1,519 ms (reads 168k buffer pages, filters 4.09M rows)
Unpatched Index Scan: 129 ms (reads 24k buffer pages)
Impact: 11.7x slowdown
Warm Cache (5-run average):
Patched Seq Scan: 233.8 ms (touches all 90,910 table blocks)
Unpatched Index Scan: 84.1 ms (touches only 22,730 blocks)
Impact: 2.8x slowdown and 4x more buffer I/O
I think costing both cases (array length known or unknown) as unhashed
linear scans isn't a bad idea maybe, i admit i haven't thought a lot
about it yet.
Regards,
Ayoub
| Attachment | Content-Type | Size |
|---|---|---|
| non-strict-reprod.sql | application/sql | 1.2 KB |
| From | Date | Subject | |
|---|---|---|---|
| Previous Message | Nathan Bossart | 2026-10-30 17:55:39 | Re: Call CalculateShmemSize() only once |