From: Tom Lane <tgl@sss.pgh.pa.us>
To: David Rowley <dgrowleyml@gmail.com>
cc: gourlaouen.mikael@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18627: Regression (15 -> 16) - Join removal not performed
 when join condition spans multiple tables
In-reply-to: 
 <CAApHDvpUiPCefrOL2id3LbF6MC0oiz8pJcDcUw3qZ=aVHjViHQ@mail.gmail.com>
References: <18627-44f950eb6a8416c2@postgresql.org>
 <CAApHDvpUiPCefrOL2id3LbF6MC0oiz8pJcDcUw3qZ=aVHjViHQ@mail.gmail.com>
Comments: In-reply-to David Rowley <dgrowleyml@gmail.com>
	message dated "Mon, 23 Sep 2024 01:45:30 +1200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2899425.1727024160.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sun, 22 Sep 2024 12:56:00 -0400
Message-ID: <2899426.1727024160@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2899426.1727024160%40sss.pgh.pa.us>
Precedence: bulk

David Rowley <dgrowleyml@gmail.com> writes:
> On Mon, 23 Sept 2024 at 00:55, PG Bug reporting form
> <noreply@postgresql.org> wrote:
> It looks like the first bad commit is acc5821e4d (Further fixes in
> qual nullingrel adjustment for outer join commutation)
> I doubt breaking this was an intended change of the nullable Var work.
> Tom can likely confirm.

Hmm ... I did not work through exactly why it broke at that particular
commit and not somewhen else, but what I conclude is that it's fairly
accidental that it ever worked at all.  join_is_removable is rejecting
removal of the syntactically-lower join because it thinks that
i.version_id is still needed "above" the join, even though we
previously removed the upper join condition:

(gdb) =

219                     if (!bms_is_subset(innerrel->attr_needed[attroff],=
 inputrelids))
(gdb) call bmsToString (inputrelids)
$5 =3D 0x2ecd968 "(b 1 2)"
(gdb) call bmsToString (joinrelids)
$6 =3D 0x2eac408 "(b 1 2 3)"
(gdb) call bmsToString (innerrel->attr_needed[attroff])
$7 =3D 0x2eac818 "(b 1 2 3)"

This happens because remove_rel_from_query only cleaned relid 4
(destination d) out of the attr_needed data; it didn't really
account for the fact that i.version_id is no longer needed anywhere
once that ON clause is gone.  That is, really at this point
attr_needed[attroff] ought to be empty, and it isn't.

Pre-v16, the case managed to work because we didn't track outer
joins (here, relid 3 for the syntactically-lower join) in
attr_needed, and so this test succeeded anyway.  That feels fairly
accidental though; it seems likely that there are related cases
that could be optimized and never have been.

The "clean and obviously correct" answer would be to recompute
all the attr_needed values from scratch after removing the upper
outer join.  That seems impractically expensive.  What I'm
thinking about is to test against joinrelids here instead of
inputrelids.  As the adjacent comment says, that would get the
wrong answer for "pushed down" conditions, but we could revert
to the pre-v16 hack of checking relevant join conditions in
the loop further down, ie put back this kluge:

        if (RINFO_IS_PUSHED_DOWN(restrictinfo, joinrelids))
        {
            /*
             * If such a clause actually references the inner rel then joi=
n
             * removal has to be disallowed.  We have to check this despit=
e
             * the previous attr_needed checks because of the possibility =
of
             * pushed-down clauses referencing the rel.
             */
            if (bms_is_member(innerrelid, restrictinfo->clause_relids))
                return false;
            continue;            /* else, ignore; not useful here */
        }

That's ugly for sure, and it would only revert to the status
quo ante.  But I'm not seeing a better way right now.

			regards, tom lane