From: Tom Lane <tgl@sss.pgh.pa.us>
To: David Rowley <dgrowleyml@gmail.com>
cc: Nathan Bossart <nathandbossart@gmail.com>,
        Paul Ramsey <pramsey@cleverelephant.ca>,
        PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Pg18 Recursive Crash
In-reply-to: <CAApHDvo11HBcb624eTAkXMBkN3=fji_DP+BqNYGYfNNo8nOxaQ@mail.gmail.com>
References: <99F064C1-B3EB-4BE7-97D2-D2A0AA487A71@cleverelephant.ca> <Z2BtL7ZXEjKkHtNX@nathan> <CAApHDvo11HBcb624eTAkXMBkN3=fji_DP+BqNYGYfNNo8nOxaQ@mail.gmail.com>
Comments: In-reply-to David Rowley <dgrowleyml@gmail.com>
	message dated "Wed, 18 Dec 2024 10:58:37 +1300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2493000.1734473089.1@sss.pgh.pa.us>
Date: Tue, 17 Dec 2024 17:04:49 -0500
Message-ID: <2493001.1734473089@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/2493001.1734473089%40sss.pgh.pa.us>
Precedence: bulk

David Rowley <dgrowleyml@gmail.com> writes:
> The slightly annoying thing here is that the attached patch passes the
> TupleTableSlotOps as NULL in nodeSetOp.c. Per nodeAppend.c line 186,
> Append does not go to much effort to setting a fixed
> TupleTableSlotOps. Really it could loop over all the child plans and
> check if those have fixed slot types of the same type and then fix its
> own resulting slot. For nodeSetOps.c use case, since the planner
> (currently) injects the flag into the target list, it'll always
> project and use a virtual slot type.  It's maybe worth coming back and
> adjusting nodeAppend.c so it works a bit harder to fix its slot type.
> I think that's likely for another patch, however.  Tom is also
> currently working on nodeSetOps.c to change how all this works so it
> no longer uses the flags method to determine the outer and inner
> sides.

Yeah, I see no point in putting effort into improving the current
nodeSetOp implementation.  There might be a reason to change
nodeAppend as you suggest for other use-cases though.

> I plan to push the attached patch soon.

I'll presumably need to rebase my nodeSetOp patch when this goes
in.  I'll take a look then at whether the new code can be improved
with this additional feature.

			regards, tom lane