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
