Re: BUG #16846: "retrieved too many tuples in a bounded sort"

James Coleman <jtc331(at)gmail(dot)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?

regards, tom lane