Dubious shortcut in ckpt_buforder_comparator()

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Dubious shortcut in ckpt_buforder_comparator()
Date: 2018-01-10 18:06:50
Message-ID: 18437.1515607610@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

While analyzing a recent crash report[1], 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] https://postgr.es/m/CAMd+QORjQGFPhRqXj8DSLn2_UgnTHtP=Kc7h2e5_hGz0GLaBMw@mail.gmail.com

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2018-01-10 18:19:46 Re: PATCH: Configurable file mode mask
Previous Message Stephen Frost 2018-01-10 17:55:12 Re: PATCH: Configurable file mode mask