Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit c24a908

Browse files
committed
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
1 parent 0126838 commit c24a908

File tree

1 file changed

+7
-5
lines changed

1 file changed

+7
-5
lines changed

src/backend/storage/buffer/bufmgr.c

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4020,8 +4020,8 @@ local_buffer_write_error_callback(void *arg)
40204020
static int
40214021
rnode_comparator(const void *p1, const void *p2)
40224022
{
4023-
RelFileNode n1 = *(RelFileNode *) p1;
4024-
RelFileNode n2 = *(RelFileNode *) p2;
4023+
RelFileNode n1 = *(const RelFileNode *) p1;
4024+
RelFileNode n2 = *(const RelFileNode *) p2;
40254025

40264026
if (n1.relNode < n2.relNode)
40274027
return -1;
@@ -4130,8 +4130,8 @@ buffertag_comparator(const void *a, const void *b)
41304130
static int
41314131
ckpt_buforder_comparator(const void *pa, const void *pb)
41324132
{
4133-
const CkptSortItem *a = (CkptSortItem *) pa;
4134-
const CkptSortItem *b = (CkptSortItem *) pb;
4133+
const CkptSortItem *a = (const CkptSortItem *) pa;
4134+
const CkptSortItem *b = (const CkptSortItem *) pb;
41354135

41364136
/* compare tablespace */
41374137
if (a->tsId < b->tsId)
@@ -4151,8 +4151,10 @@ ckpt_buforder_comparator(const void *pa, const void *pb)
41514151
/* compare block number */
41524152
else if (a->blockNum < b->blockNum)
41534153
return -1;
4154-
else /* should not be the same block ... */
4154+
else if (a->blockNum > b->blockNum)
41554155
return 1;
4156+
/* equal page IDs are unlikely, but not impossible */
4157+
return 0;
41564158
}
41574159

41584160
/*

0 commit comments

Comments
 (0)