Re: Check lateral references within PHVs for memoize cache keys - Mailing list pgsql-hackers

From Richard Guo
Subject Re: Check lateral references within PHVs for memoize cache keys
Date
Msg-id CAMbWs4_CQR9ikyAhjH0CwzCdp+FcRAeoVsi5Oohd7s=c5RZABg@mail.gmail.com
Whole thread Raw
In response to Re: Check lateral references within PHVs for memoize cache keys  (Andrei Lepikhov <lepihov@gmail.com>)
Responses Re: Check lateral references within PHVs for memoize cache keys
Re: Check lateral references within PHVs for memoize cache keys
List pgsql-hackers
On Fri, Jun 28, 2024 at 10:14 PM Andrei Lepikhov <lepihov@gmail.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

pgsql-hackers by date:

Previous
From: shveta malik
Date:
Subject: Re: Conflict detection and logging in logical replication
Next
From: Nisha Moond
Date:
Subject: Re: Improve the connection failure error messages