Re: Check lateral references within PHVs for memoize cache keys

On 6/18/24 08:47, Richard Guo wrote:
> On Mon, Mar 18, 2024 at 4:36 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>> Here is another rebase over master so it applies again. I also added a
>> commit message to help review. Nothing else has changed.
>
> AFAIU currently we do not add Memoize nodes on top of join relation
> paths. This is because the ParamPathInfos for join relation paths do
> not maintain ppi_clauses, as the set of relevant clauses varies
> depending on how the join is formed. In addition, joinrels do not
> maintain lateral_vars. So we do not have a way to extract cache keys
> from joinrels.
>
> (Besides, there are places where the code doesn't cope with Memoize path
> on top of a joinrel path, such as in get_param_path_clause_serials.)
>
> Therefore, when extracting lateral references within PlaceHolderVars,
> there is no need to consider those that are due to be evaluated at
> joinrels.
>
> Hence, here is v7 patch for that. In passing, this patch also includes
> a comment explaining that Memoize nodes are currently not added on top
> of join relation paths (maybe we should have a separate patch for this?).
Hi,
I have reviewed v7 of the patch. This improvement is good enough to be
applied, thought. Here is some notes:

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.

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
}

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?

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?

Generally, this patch looks good for me to be committed.

--
regards, Andrei Lepikhov