Re: Postgres 16.1 - Bug: cache entry already complete

On Wed, Jan 3, 2024 at 9:48 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:

> My concern with that is that the Unique Join optimisation will cause
> execution to skip to the next outer row and that will leave us no
> means of marking the Memoize cache entry as complete. In
> singlerow==false Memoize nodes, we only mark the cache as complete
> when we read the inner node to completion. The unique join
> optimisation means we often don't do that due to skipping to the next
> outer row on finding the first inner match.
>
> Basically, what I'm saying is that Memoize is going to result in many
> more cache misses due to incomplete cache entries. Maybe we should
> have get_memoize_path() return NULL for this case so that we don't
> Memoize when there's a Join Filter and extra->inner_unique is set to
> true.

I agree that we should just avoid making a memoize path in this case.
And I think we already have this logic in get_memoize_path():

if (extra->inner_unique &&
(inner_path->param_info == NULL ||
list_length(inner_path->param_info->ppi_clauses) <
list_length(extra->restrictlist)))
return NULL;

But why does the code above fail to capture the problematic query? It
turns out that inner_path->param_info->ppi_clauses contains cloned
clauses, i.e. multiple versions of the same clause in order to cope with
outer join identity 3. And this causes the above code to give incorrect
conclusions about the length of 'ppi_clauses'. And this also explains
why this issue only occurs in v16.

As a clause's serial number is unique within the current PlannerInfo
context, and multiple clones of the same clause have the same serial
number, it seems to me that it's more correct to calculate the length of
ppi_clauses by:

bms_num_members(inner_path->param_info->ppi_serials)

So I think maybe we can fix this issue with the attached patch.

BTW, the SingleRow in explain seems quite handy. I wonder if we can
keep it.

Thanks
Richard