Dubious shortcut in ckpt_buforder_comparator()

Started by Tom Laneover 8 years ago3 messageshackers
Jump to latest
#1Tom Lane
tgl@sss.pgh.pa.us

While analyzing a recent crash report[1]/messages/by-id/CAMd+QORjQGFPhRqXj8DSLn2_UgnTHtP=Kc7h2e5_hGz0GLaBMw@mail.gmail.com, I noticed that bufmgr.c's
ckpt_buforder_comparator is coded to assume that no two CkptSortItems
could have equal page IDs; it therefore skips the final comparison
and will never return 0 (equal). I do not think that assumption is
correct. I do not see anything preventing the scenario where, while
we are scanning the buffer pool at the beginning of BufferSync(),
somebody writes out a dirty buffer we've already seen and recycles
it for another page, and then somebody else fetches that same page
back into a different buffer and dirties it, and finally we arrive
at that second buffer and conclude it should be written too.

If we do get two entries with equal page IDs into the CkptSortItem
list, ckpt_buforder_comparator will give self-inconsistent results:
when actually x = y, it will say both x > y and y > x depending on
which way you pass the arguments to it.

Now, I cannot find any reason to think this would have awful consequences
given our current implementation of qsort. But we might not always use
that code --- I remember some discussion of timsort, for instance ---
and another sorting implementation might be less happy about it.

So I think we should get rid of that micro-optimization, which is
probably useless anyway from a performance standpoint, and do the
comparison honestly. Any objections?

regards, tom lane

[1]: /messages/by-id/CAMd+QORjQGFPhRqXj8DSLn2_UgnTHtP=Kc7h2e5_hGz0GLaBMw@mail.gmail.com

#2Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#1)
Re: Dubious shortcut in ckpt_buforder_comparator()

Hi,

On 2018-01-10 13:06:50 -0500, Tom Lane wrote:

So I think we should get rid of that micro-optimization, which is
probably useless anyway from a performance standpoint, and do the
comparison honestly. Any objections?

No, absolutely none. You're going to change it?

Greetings,

Andres Freund

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#2)
Re: Dubious shortcut in ckpt_buforder_comparator()

Andres Freund <andres@anarazel.de> writes:

On 2018-01-10 13:06:50 -0500, Tom Lane wrote:

So I think we should get rid of that micro-optimization, which is
probably useless anyway from a performance standpoint, and do the
comparison honestly. Any objections?

No, absolutely none. You're going to change it?

Will do.

regards, tom lane