Re: Converting SetOp to read its two inputs separately

On Fri, 20 Dec 2024 at 08:38, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> > On Thu, 19 Dec 2024 at 15:44, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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