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