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
