Re: Assert !bms_overlap(joinrel->relids, required_outer) - Mailing list pgsql-hackers

From Richard Guo
Subject Re: Assert !bms_overlap(joinrel->relids, required_outer)
Date
Msg-id CAMbWs4-+Gs0HJ9ouBUb=qwHsGCXxG+92eJzLOpCkedvgtOWQ=Q@mail.gmail.com
Whole thread Raw
In response to Re: Assert !bms_overlap(joinrel->relids, required_outer)  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: Assert !bms_overlap(joinrel->relids, required_outer)
List pgsql-hackers

On Fri, Jun 30, 2023 at 12:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Richard Guo <guofenglinux@gmail.com> writes:
> On Wed, Jun 28, 2023 at 10:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Those cases will go through calc_non_nestloop_required_outer
>> which has
>> /* neither path can require rels from the other */
>> Assert(!bms_overlap(outer_paramrels, inner_path->parent->relids));
>> Assert(!bms_overlap(inner_paramrels, outer_path->parent->relids));

> Looking at these two assertions it occurred to me that shouldn't we
> check against top_parent_relids for an otherrel since paths are
> parameterized by top-level parents?  We do that in try_nestloop_path.

Yeah, while looking at this I was wondering why try_mergejoin_path and
try_hashjoin_path don't do the same "Paths are parameterized by
top-level parents, so run parameterization tests on the parent relids"
dance that try_nestloop_path does.  This omission is consistent with
that, but it's not obvious why it'd be okay to skip it for
non-nestloop joins.  I guess we'd have noticed by now if it wasn't
okay, but ...

I think it just makes these two assertions meaningless to skip it for
non-nestloop joins if the input paths are for otherrels, because paths
would never be parameterized by the member relations.  So these two
assertions would always be true for otherrel paths.  I think this is why
we have not noticed any problem by now.

However, this is not what we want.  What we want is to verify that
neither input path for the joinrel can require rels from the other, even
for otherrel paths.  So I think the current code is not right for that.
We need to check against top_parent_relids for otherrel paths, and that
would make these assertions meaningful.

Thanks
Richard

pgsql-hackers by date:

Previous
From: Kyotaro Horiguchi
Date:
Subject: Re: vacuumdb/clusterdb/reindexdb: allow specifying objects to process in all databases
Next
From: Andres Freund
Date:
Subject: Re: ReadRecentBuffer() doesn't scale well