Subject: An incorrect check in get_memoize_path

From: Richard Guo <[email-masked]>
Date: Mon, 7 Apr 2025 16:50:20 +0900

While reviewing another patch [1], I noticed an incorrect check in
get_memoize_path.

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

This check is to ensure that, for unique joins, all the restriction
clauses are parameterized to enable the use of Memoize nodes.
However, ppi_clauses includes join clauses available from all outer
rels, not just the current outer rel, while extra->restrictlist only
includes the restriction clauses for the current join.  This means the
check could pass even if a restriction clause isn't parameterized, as
long as another join clause, which doesn't belong to the current join,
is included in ppi_clauses.  It's not very hard to construct a query
that exposes this issue.

create table t (a int, b int);

explain (costs off)
select * from t t0 left join
  (t t1 left join t t2 on t1.a = t2.a
   join lateral
     (select distinct on (a, b) a, b, t0.a+t1.a+t2.a as x from t t3 offset 0) t3
         on t1.a = t3.a and coalesce(t2.b) = t3.b)
  on t0.b = t3.b;
                       QUERY PLAN
---------------------------------------------------------
 Nested Loop Left Join
   ->  Seq Scan on t t0
   ->  Nested Loop
         Join Filter: (COALESCE(t2.b) = t3.b)
         ->  Hash Left Join
               Hash Cond: (t1.a = t2.a)
               ->  Seq Scan on t t1
               ->  Hash
                     ->  Seq Scan on t t2
         ->  Subquery Scan on t3
               Filter: ((t0.b = t3.b) AND (t1.a = t3.a))
               ->  Unique
                     ->  Sort
                           Sort Key: t3_1.a, t3_1.b
                           ->  Seq Scan on t t3_1
(15 rows)

Consider the join to t3.  It is a unique join, and not all of its
restriction clauses are parameterized.  Despite this, the check still
passes.

Interestingly, although the check passes, no Memoize nodes are added
on top of t3's paths because paraminfo_get_equal_hashops insists that
all ppi_clauses must satisfy clause_sides_match_join (though I
actually question whether this is necessary, but that's a separate
issue).  However, this does not justify the check being correct.

Hence, I propose the attached patch for the fix.

BTW, there is a XXX comment there saying that maybe we can make the
remaining join quals part of the inner scan's filter instead of the
join filter.  I don't think this is possible in all cases.  In the
above query, 'coalesce(t2.b) = t3.b' cannot be made part of t3's scan
filter, according to join_clause_is_movable_into.  So I removed that
comment in the patch while we're here.

Any thoughts?

[1] https://postgr.es/m/[email-masked]

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Tue, 8 Apr 2025 15:32:00 +0900

On Mon, Apr 7, 2025 at 9:54 PM Andrei Lepikhov <[email-masked]> wrote:
> On 4/7/25 09:50, Richard Guo wrote:

> At the same time I think term 'Incorrect' is not good unless you show an
> example where data returned is not consistent to the expected.
> I think this inequality check has worked in couple with the
> get_equal_hashops.

Do you mean this check is designed to work in conjunction with the
clause_sides_match_join check in paraminfo_get_equal_hashops?  I would
consider this a very poor design.  The purpose of this check is simply
to verify whether all restriction clauses are parameterized.  I don't
understand why it needs to depend on paraminfo_get_equal_hashops in
such an unexpected way to function correctly.  I can't see any
advantage to this design, other than that it's prone to bugs.
Moreover, in the case where not all restriction clauses are
parameterized, why waste CPU cycles running all the code after the
check only for paraminfo_get_equal_hashops to catch it later?
Additionally, I couldn't find any comments explaining this unusual
behavior, either in the check itself or in paraminfo_get_equal_hashops.

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Mon, 14 Apr 2025 15:49:59 +0900

> comment in the patch while we're here.

Here is an updated patch with a commit message.  Regarding
backporting, I'm inclined not to, given the lack of field reports.
Any objections to pushing it?

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Wed, 16 Apr 2025 11:11:57 +0900

On Mon, Apr 14, 2025 at 8:08 PM wenhui qiu <[email-masked]> wrote:
>   No objections.It's a pity that the postgresql18 version has been code-frozen

v18 is now in feature freeze, but not code freeze, so bug fixes are
still allowed.  I've pushed this patch after adding the "Reviewed-by"
tags.

Thanks
Richard