From: | Richard Guo <guofenglinux(at)gmail(dot)com> |
---|---|
To: | Andrei Lepikhov <lepihov(at)gmail(dot)com> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, David Rowley <dgrowleyml(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | Re: Check lateral references within PHVs for memoize cache keys |
Date: | 2024-07-11 09:18:39 |
Message-ID: | CAMbWs4_CQR9ikyAhjH0CwzCdp+FcRAeoVsi5Oohd7s=c5RZABg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Jun 28, 2024 at 10:14 PM Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:
> I have reviewed v7 of the patch. This improvement is good enough to be
> applied, thought.
Thank you for reviewing this patch!
> Comment may be rewritten for clarity:
> "Determine if the clauses in param_info and innerrel's lateral_vars" -
> I'd replace lateral_vars with 'lateral references' to combine in one
> phrase PHV from rel and root->placeholder_list sources.
Makes sense. I ended up using 'innerrel's lateral vars' to include
both the lateral Vars/PHVs found in innerrel->lateral_vars and those
extracted from within PlaceHolderVars that are due to be evaluated at
innerrel.
> I wonder if we can add whole PHV expression instead of the Var (as
> discussed above) just under some condition:
> if (!bms_intersect(pull_varnos(root, (Node *) phinfo->ph_var->phexpr),
> innerrelids))
> {
> // Add whole PHV
> }
> else
> {
> // Add only pulled vars
> }
Good point. After considering it further, I think we should do this.
As David explained, this can be beneficial in cases where the whole
expression results in fewer distinct values to cache tuples for.
> I got the point about Memoize over join, but as a join still calls
> replace_nestloop_params to replace parameters in its clauses, why not to
> invent something similar to find Memoize keys inside specific JoinPath
> node? It is not the issue of this patch, though - but is it doable?
I don't think it's impossible to do, but I'm skeptical that there's an
easy way to identify all the cache keys for joinrels, without having
available ppi_clauses and lateral_vars.
> IMO, the code:
> if (bms_nonempty_difference(outerPlan->chgParam, node->keyparamids))
> cache_purge_all(node);
>
> is a good place to check an assertion: is it really the parent query
> parameters that make a difference between memoize keys and node list of
> parameters?
I don't think we have enough info available here to identify which
params within outerPlan->chgParam are from outer levels. Maybe we can
store root->outer_params in the MemoizeState node to help with this
assertion, but I'm not sure if it's worth the trouble.
Attached is an updated version of this patch.
Thanks
Richard
Attachment | Content-Type | Size |
---|---|---|
v8-0001-Check-lateral-references-within-PHVs-for-memoize-cache-keys.patch | application/octet-stream | 15.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Nisha Moond | 2024-07-11 09:31:21 | Re: Improve the connection failure error messages |
Previous Message | shveta malik | 2024-07-11 09:06:13 | Re: Conflict detection and logging in logical replication |