public inbox for [email protected]  

help / color / mirror / Atom feed

From: Andrei Lepikhov <[email protected]>
To: Richard Guo <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Cc: David Rowley <[email protected]>
Cc: Tom Lane <[email protected]>
Subject: Re: Check lateral references within PHVs for memoize cache keys
Date: Fri, 28 Jun 2024 21:14:53 +0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAMbWs4-n-bc=cxXwn7oY0E8YojSwiVLUpz6+OqPre3_YJs1_NQ@mail.gmail.com>
References: <CAMbWs48jLxn0pAPZpJ50EThZ569Xrw+=4Ac3QvkpQvNszbeoNg@mail.gmail.com>
	<CAMbWs48TGnh8s=9-hJge9Et4Crf9q+_oU4FVekLoX4ommi4CVg@mail.gmail.com>
	<CAMbWs4_E2HcTJ0b2srM3hwnQbH5SogkZMM399WUG4gTQSa1FOQ@mail.gmail.com>
	<[email protected]>
	<CAMbWs49+Cjoy0S0xkCRDcHXGHvsYLOdvr9jq9OTONOBnsgzXOg@mail.gmail.com>
	<CAMbWs4_imG5C8rXt7xdU7zf6whUDc2rdDun+Vtrowcmxb41CzA@mail.gmail.com>
	<CAMbWs49Z6TrH4qJuudPx2toeJutPOV1CyvbL3GE1QNg_LaATEg@mail.gmail.com>
	<CAMbWs48A3Zd2=wZ9xCM07qHEeGBw-X2nodj2n_qnHNtBqiA73g@mail.gmail.com>
	<CAMbWs4-n-bc=cxXwn7oY0E8YojSwiVLUpz6+OqPre3_YJs1_NQ@mail.gmail.com>

On 6/18/24 08:47, Richard Guo wrote:
> On Mon, Mar 18, 2024 at 4:36 PM Richard Guo <[email protected]> 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

view thread (9+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Check lateral references within PHVs for memoize cache keys
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox