Обсуждение: pgsql: Fix planner's use of Result Cache with unique joins

Fix planner's use of Result Cache with unique joins

When the planner considered using a Result Cache node to cache results
from the inner side of a Nested Loop Join, it failed to consider that the
inner path's parameterization may not be the entire join condition.  If
the join was marked as inner_unique then we may accidentally put the cache
in singlerow mode.  This meant that entries would be marked as complete
after caching the first row.  That was wrong as if only part of the join
condition was parameterized then the uniqueness of the unique join was not
guaranteed at the Result Cache's level.  The uniqueness is only guaranteed
after Nested Loop applies the join filter.  If subsequent rows were found,
this would lead to:

ERROR: cache entry already complete

This could have been fixed by only putting the cache in singlerow mode if
the entire join condition was parameterized.  However, Nested Loop will
only read its inner side so far as the first matching row when the join is
unique, so that might mean we never get an opportunity to mark cache
entries as complete.  Since non-complete cache entries are useless for
subsequent lookups, we just don't bother considering a Result Cache path
in this case.

In passing, remove the XXX comment that claimed the above ERROR might be
better suited to be an Assert.  After there being an actual case which
triggered it, it seems better to keep it an ERROR.

Reported-by: David Christensen
Discussion: https://postgr.es/m/CAOxo6X+dy-V58iEPFgst8ahPKEU+38NZzUuc+a7wDBZd4TrHMQ@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/9e215378d7fbb7d4615be917917c52f246cc6c61

Modified Files
--------------
src/backend/executor/nodeResultCache.c |  2 +-
src/backend/optimizer/path/joinpath.c  | 31 +++++++++++++++++++++++++++++++
2 files changed, 32 insertions(+), 1 deletion(-)

David Rowley <drowley@postgresql.org> writes:
> Fix planner's use of Result Cache with unique joins

Coverity is not happy with this:

/srv/coverity/git/pgsql-git/postgresql/src/backend/optimizer/path/joinpath.c: 532 in get_resultcache_path()
526          * determining if the join is unique.
527          *
528          * XXX this could be enabled if the remaining join quals were made part of
529          * the inner scan's filter instead of the join filter.  Maybe it's worth
530          * considering doing that?
531          */
>>>     CID 1484914:  Null pointer dereferences  (FORWARD_NULL)
>>>     Dereferencing null pointer "inner_path->param_info".
532         if (extra->inner_unique &&
533             list_length(inner_path->param_info->ppi_clauses) <
534             list_length(extra->restrictlist))
535             return NULL;
536

This complaint is triggered because elsewhere in the same function,
you're careful to check for inner_path->param_info being null before
trying to dereference it:

    if ((inner_path->param_info == NULL ||
         inner_path->param_info->ppi_clauses == NIL) &&
        innerrel->lateral_vars == NIL)
        return NULL;

Coverity reasons that it's probably a bug that you didn't do the same
here; and I think it's right.

            regards, tom lane

On Mon, 24 May 2021 at 02:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Coverity is not happy with this:

> This complaint is triggered because elsewhere in the same function,
> you're careful to check for inner_path->param_info being null before
> trying to dereference it:
>
>     if ((inner_path->param_info == NULL ||
>          inner_path->param_info->ppi_clauses == NIL) &&
>         innerrel->lateral_vars == NIL)
>         return NULL;
>
> Coverity reasons that it's probably a bug that you didn't do the same
> here; and I think it's right.

Yeah, I think that should be fixed. Thanks for reporting.

I propose the attached.  I think if the restrictlist is empty then we
needn't bother trying Result Cache.

David