diff options
| author | Tom Lane <tgl@sss.pgh.pa.us> | 2018-01-10 15:50:54 -0500 |
|---|---|---|
| committer | Tom Lane <tgl@sss.pgh.pa.us> | 2018-01-10 15:50:54 -0500 |
| commit | 3afd75eaac8aaccf5aeebc52548c396b84d85516 (patch) | |
| tree | da2ccb0a821a7b6b06efdb2b1648c3d346180c9d | |
| parent | 2fd58096f02777c38edb392f78cb5b4ebd90e9d2 (diff) | |
| download | postgresql-3afd75eaac8aaccf5aeebc52548c396b84d85516.tar.gz postgresql-3afd75eaac8aaccf5aeebc52548c396b84d85516.zip | |
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
| -rw-r--r-- | src/backend/storage/buffer/bufmgr.c | 12 |
1 files 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; } /* |