Commit 3afd75ea authored by Tom Lane's avatar Tom Lane

Remove dubious micro-optimization in ckpt_buforder_comparator().

It seems incorrect to assume that the list of CkptSortItems can never
contain duplicate page numbers: concurrent activity could result in some
page getting dropped from a low-numbered buffer and later loaded into a
high-numbered buffer while BufferSync is scanning the buffer pool.
If that happened, the comparator would give self-inconsistent results,
potentially confusing qsort().  Saving one comparison step is not worth
possibly getting the sort wrong.

So far as I can tell, nothing would actually go wrong given our current
implementation of qsort().  It might get a bit slower than expected
if there were a large number of duplicates of one value, but that's
surely a probability-epsilon case.  Still, the comment is wrong,
and if we ever switched to another sort implementation it might be
less forgiving.

In passing, avoid casting away const-ness of the argument pointers;
I've not seen any compiler complaints from that, but it seems likely
that some compilers would not like it.

Back-patch to 9.6 where this code came in, just in case I've underestimated
the possible consequences.

Discussion: https://postgr.es/m/18437.1515607610@sss.pgh.pa.us
parent 2fd58096
...@@ -4064,8 +4064,8 @@ local_buffer_write_error_callback(void *arg) ...@@ -4064,8 +4064,8 @@ local_buffer_write_error_callback(void *arg)
static int static int
rnode_comparator(const void *p1, const void *p2) rnode_comparator(const void *p1, const void *p2)
{ {
RelFileNode n1 = *(RelFileNode *) p1; RelFileNode n1 = *(const RelFileNode *) p1;
RelFileNode n2 = *(RelFileNode *) p2; RelFileNode n2 = *(const RelFileNode *) p2;
if (n1.relNode < n2.relNode) if (n1.relNode < n2.relNode)
return -1; return -1;
...@@ -4174,8 +4174,8 @@ buffertag_comparator(const void *a, const void *b) ...@@ -4174,8 +4174,8 @@ buffertag_comparator(const void *a, const void *b)
static int static int
ckpt_buforder_comparator(const void *pa, const void *pb) ckpt_buforder_comparator(const void *pa, const void *pb)
{ {
const CkptSortItem *a = (CkptSortItem *) pa; const CkptSortItem *a = (const CkptSortItem *) pa;
const CkptSortItem *b = (CkptSortItem *) pb; const CkptSortItem *b = (const CkptSortItem *) pb;
/* compare tablespace */ /* compare tablespace */
if (a->tsId < b->tsId) if (a->tsId < b->tsId)
...@@ -4195,8 +4195,10 @@ ckpt_buforder_comparator(const void *pa, const void *pb) ...@@ -4195,8 +4195,10 @@ ckpt_buforder_comparator(const void *pa, const void *pb)
/* compare block number */ /* compare block number */
else if (a->blockNum < b->blockNum) else if (a->blockNum < b->blockNum)
return -1; return -1;
else /* should not be the same block ... */ else if (a->blockNum > b->blockNum)
return 1; return 1;
/* equal page IDs are unlikely, but not impossible */
return 0;
} }
/* /*
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment