postgres.git / summary / log / commit / refs

commit    3afd75eaac8aaccf5aeebc52548c396b84d85516
Author:   Tom Lane <tgl@sss.pgh.pa.us>
Date:     Wed Jan 10 20:50:54 2018 +0000

    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 4e44336332b..01eabe57063 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4064,8 +4064,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;
@@ -4174,8 +4174,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)
@@ -4195,8 +4195,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;
 }
 
 /*


[parent: 2fd58096f027]