From c24a908d04806c34c3e5dda42d39bb5ca1fc5ffb Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 10 Jan 2018 15:50:54 -0500 Subject: [PATCH] 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 --- src/backend/storage/buffer/bufmgr.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 9cc2470de8..0e4a8ac800 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -4020,8 +4020,8 @@ local_buffer_write_error_callback(void *arg) static int rnode_comparator(const void *p1, const void *p2) { - RelFileNode n1 = *(RelFileNode *) p1; - RelFileNode n2 = *(RelFileNode *) p2; + RelFileNode n1 = *(const RelFileNode *) p1; + RelFileNode n2 = *(const RelFileNode *) p2; if (n1.relNode < n2.relNode) return -1; @@ -4130,8 +4130,8 @@ buffertag_comparator(const void *a, const void *b) static int ckpt_buforder_comparator(const void *pa, const void *pb) { - const CkptSortItem *a = (CkptSortItem *) pa; - const CkptSortItem *b = (CkptSortItem *) pb; + const CkptSortItem *a = (const CkptSortItem *) pa; + const CkptSortItem *b = (const CkptSortItem *) pb; /* compare tablespace */ if (a->tsId < b->tsId) @@ -4151,8 +4151,10 @@ ckpt_buforder_comparator(const void *pa, const void *pb) /* compare block number */ else if (a->blockNum < b->blockNum) return -1; - else /* should not be the same block ... */ + else if (a->blockNum > b->blockNum) return 1; + /* equal page IDs are unlikely, but not impossible */ + return 0; } /* -- 2.40.0