On Fri, 20 Dec 2024 at 08:38, Tom Lane <t...@sss.pgh.pa.us> wrote: > > David Rowley <dgrowle...@gmail.com> writes: > > On Thu, 19 Dec 2024 at 15:44, Tom Lane <t...@sss.pgh.pa.us> wrote: > >> 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. > > OK, done, and I added an 0006 patch that uses that infrastructure > in the obvious places.
That looks good. Thanks for adjusting the other node types too. > I also addressed your remarks about comments. Otherwise I'm feeling > like this is about ready to push. I think so too. One minor detail... I think the only thing I'd like to see is the moving of the enable_hashagg checks to increment the disabled_nodes count in create_setop_path() instead of where it's being called. I understand there's only 1 caller of that function that passes SETOP_HASHED, but it does seem nicer to put that logic where it belongs. With how you have it now, if we were ever to grow any more places that built SETOP_HASHED SetOpPaths, they'd also need to adjust disabled_nodes manually and that seems easy to forget. Also, looking around for references to "disabled_nodes", it looks like all other places where we fiddle with the value of disabled_nodes are in costsize.c. I understand we do check enable_hashagg in other places, but those all seem to be so we avoid generating some Path rather than to determine the disabled_node value. David
