On Thu, Feb 11, 2021 at 12:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> James Coleman <jtc331@gmail.com> writes:
> > 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.
>
> Thanks for looking.
>
> > 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.)
>
> No objection to doing that, however ...
>
> > 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.
>
> I stared at this for awhile and eventually convinced myself that it
> implemented the same logic, but it still seems overly complex. We
> do not need either the firstTuple or lastTuple flags, and we could
> convert the nTuple adjustments into a normal for-loop with (IMO)
> much greater intelligibility. What do you think of the attached?
Yes, that looks even better. Not sure how I missed that I'd just
reimplemented a normal for-loop with firstTuple/lastTuple conditions,
but I guess that's the benefit of coming at it with fresh eyes and
without the history of how it got the way it was.
+1 on committing v2.
James
