Re: Check lateral references within PHVs for memoize cache keys

On Fri, 30 Dec 2022 at 16:00, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
> On Fri, Dec 9, 2022 at 5:16 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>>
>> Actually we do have checked PHVs for lateral references, earlier in
>> create_lateral_join_info. But that time we only marked lateral_relids
>> and direct_lateral_relids, without remembering the lateral expressions.
>> So I'm wondering whether we can fix that by fetching Vars (or PHVs) of
>> lateral references within PlaceHolderVars and remembering them in the
>> baserel's lateral_vars.
>>
>> Attach a draft patch to show my thoughts.

I'm surprised to see that it's only Memoize that ever makes use of
lateral_vars. I'd need a bit more time to process your patch, but one
additional thought I had was that I wonder if the following code is
still needed in nodeMemoize.c

if (bms_nonempty_difference(outerPlan->chgParam, node->keyparamids))
cache_purge_all(node);

Ideally, that would be an Assert failure, but possibly we should
probably still call cache_purge_all(node) after Assert(false) so that
at least we'd not start returning wrong results if we've happened to
miss other cache keys. I thought maybe something like:

if (bms_nonempty_difference(outerPlan->chgParam, node->keyparamids))
{
/*
* Really the planner should have added all the possible parameters to
* the cache keys, so let's Assert fail here so we get the memo to fix
* that can fix that. On production builds, we'd better purge the
* cache to account for the changed parameter value.
*/
Assert(false);

cache_purge_all(node);
}

I've not run the tests to ensure we don't get an Assert failure with
that, however.

All that cache_purge_all code added in 411137a42 likely was an
incorrect fix for what you've raised here, but it's maybe a good
failsafe to keep around even if we think we've now found all possible
parameters that can invalidate the memorized results.

David