On Thu, 19 Dec 2024 at 15:44, Tom Lane <t...@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
