Message-ID: <a19ac195-0dd9-43f1-94a8-8ab39e48dab7@gmail.com>
Date: Fri, 28 Jun 2024 21:14:53 +0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Andrei Lepikhov <lepihov@gmail.com>
Subject: Re: Check lateral references within PHVs for memoize cache keys
To: Richard Guo <guofenglinux@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>,
 David Rowley <dgrowleyml@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
References: <CAMbWs48jLxn0pAPZpJ50EThZ569Xrw+=4Ac3QvkpQvNszbeoNg@mail.gmail.com>
 <CAMbWs48TGnh8s=9-hJge9Et4Crf9q+_oU4FVekLoX4ommi4CVg@mail.gmail.com>
 <CAMbWs4_E2HcTJ0b2srM3hwnQbH5SogkZMM399WUG4gTQSa1FOQ@mail.gmail.com>
 <1724688.1688837323@sss.pgh.pa.us>
 <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>
Content-Language: en-US
In-Reply-To: <CAMbWs4-n-bc=cxXwn7oY0E8YojSwiVLUpz6+OqPre3_YJs1_NQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/a19ac195-0dd9-43f1-94a8-8ab39e48dab7%40gmail.com>
Precedence: bulk

On 6/18/24 08:47, Richard Guo wrote:
> On Mon, Mar 18, 2024 at 4:36 PM Richard Guo <guofenglinux@gmail.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