Robert Haas <robertmhaas@gmail.com> writes:
> So I tried out the logic described in this email and, with a few
> modifications, it seemed to work. Updated patch attached, any review
> appreciated.
Applied with revisions.
>> 3. TGL: "Speaking of sorting, it's not entirely clear to me how the
>> patch ensures that all the child plans produce the necessary sort keys
>> as output columns, and especially not how it ensures that they all get
>> produced in the *same* output columns. This might accidentally manage
>> to work because of the "throwaway" call to make_sort_from_pathkeys(),
>> but at the very least that's a misleading comment." I'm not sure what
>> needs to be done about this; I'm going to look at this further.
> I spent some time looking at this complaint, and I'm still not sure
> what needs to be done about it. Experimentation reveals that the
> neither the throwaway call to make_sort_from_pathkeys() nor any other
> call to that function is creating the relevant target list entries.
Yeah, this was in fact pretty broken. make_sort_from_pathkeys *does*
create the relevant tlist entries, but they were getting lost again
because they were attached to a Result node that went into the bit
bucket along with the "throwaway" Sort node. Much worse, it was hit or
miss whether the tlist entries would be present in the child plan nodes
--- as coded, this would be the case only for children that were
explicitly sorted to meet the merge's pathkey requirements. If they
weren't there, any sorting on resjunk values would misbehave. I was
able to develop a test case that exposed this by using expression
indexes, so that went into the regression tests.
I fixed this by refactoring make_sort_from_pathkeys so that the tlist
fixup logic was separately accessible.
There were assorted other bugs too. I think it's all good now, but
we'll find out when beta starts ...
regards, tom lane