Re: Converting SetOp to read its two inputs separately

On Thu, 19 Dec 2024 at 15:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> With David's recent fixes to allow telling BuildTupleHashTableExt
> what input slot type to expect, it's possible to remove the per-row
> slot type conversions I was doing before.  So here's an updated
> patchset with that done.
>
> The common_result_slot_type() function I wrote here perhaps
> should be made generally available, but I didn't do that yet.

I think it would be good to make this generic as it can be at least
used in nodeRecursiveunion.c and nodeAppend.c.  For the Append usage,
I wonder if it's worth making the function accept an array of
PlanStates rather than just two of them. Then, either add a helper
function that allocates and assigns the outer and inner side to a
stack-allocated two-element array, or just do that part in nodeSetOp.c
and nodeRecursiveunion.c and pass that to the array-accepting
function.

Review:

0001:

+ /* Left group is first, has no right matches */

Should the comma be an "or"?

0002:

Should this adjust the following nodeSetOp.c comment:

 * In SETOP_SORTED mode, each input has been sorted according to all the
 * grouping columns (ie, all the non-junk attributes).  The SetOp node

This implies that there can be junk columns.

0003:

Looks fine. The only thing that I paused to think about was your
decision to double disable hashing when !enable_hashagg and the
results are bigger than get_hash_memory_limit(). I think have you have
makes sense there.

0004:

Looks fine. I see no reason to delay you pushing this.

0005:

Looks good.

David