On Thu, Feb 4, 2021 at 10:07 PM Neil Chen <carpenter.nail.cz@gmail.com> wrote:
>
>
>
> On Fri, Feb 5, 2021 at 8:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>>
>> I poked at this until I found a small modification of the existing
>> regression tests that would reach the problematic path with lastTuple
>> true. That confirmed that there's a problem, because it gave a flat-out
>> wrong answer. I'm not really familiar enough with this code to be sure
>> if this is a complete fix, but it fixes the cases we have and it doesn't
>> break anything else in our regression tests. So, in view of the fact
>> that we have 13.2 release deadline on Monday, I went ahead and pushed it.
>> (I did rewrite your comment.)
>>
>
> That's great, I will also continue to try to check if it is a complete fix.
> Thank you.
I didn't see this thread until now (unfortunately I'm not able to
consistently keep up with mailing list traffic, though I'm happy to be
tagged on any incremental sort issue so I see that discussion). I
reviewed the patch, and I believe it's correct.
I did have to stare a bit at nodeIncrementalSort.c for a while though
to realize that the test case works because the full sort state was
bounded (so 5 tuples is enough to trigger the case even though a
cursory reading of the code and the bug description would imply that
64 tuples are needed to trigger it). So I have a mild preference for
noting that in the test case comment, and I also lean towards having
an EXPLAIN on the test case query to make sure it remains a valid test
case in the future (i.e., making sure other changes don't change plan
such that this case no longer has regression coverage.)
We can simplify the code a bit so that lastTuple is only set to true
when necessary, rather than setting it only to unset it in this case.
Attached is a patch to do all of the above, though I'm hardly
interested in making this a hill to die on.
James
