Re: Check lateral references within PHVs for memoize cache keys

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