From 323bd018cde846c44f0040ec3788a393ebd09c41 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Mon, 7 Oct 2024 18:04:04 +0300 Subject: [PATCH] Make sure BufferTag padding bytes are cleared in hash keys (#9292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prefetch-queue hash table uses a BufferTag struct as the hash key, and it's hashed using hash_bytes(). It's important that all the padding bytes in the key are cleared, because hash_bytes() will include them. I was getting compiler warnings like this on v14 and v15, when compiling with -Warray-bounds: In function ‘prfh_lookup_hash_internal’, inlined from ‘prfh_lookup’ at pg_install/v14/include/postgresql/server/lib/simplehash.h:821:9, inlined from ‘neon_read_at_lsnv’ at pgxn/neon/pagestore_smgr.c:2789:11, inlined from ‘neon_read_at_lsn’ at pgxn/neon/pagestore_smgr.c:2904:2: pg_install/v14/include/postgresql/server/storage/relfilenode.h:90:43: warning: array subscript ‘PrefetchRequest[0]’ is partly outside array bounds of ‘BufferTag[1]’ {aka ‘struct buftag[1]’} [-Warray-bounds] 89 | ((node1).relNode == (node2).relNode && \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 90 | (node1).dbNode == (node2).dbNode && \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~ 91 | (node1).spcNode == (node2).spcNode) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ pg_install/v14/include/postgresql/server/storage/buf_internals.h:116:9: note: in expansion of macro ‘RelFileNodeEquals’ 116 | RelFileNodeEquals((a).rnode, (b).rnode) && \ | ^~~~~~~~~~~~~~~~~ pgxn/neon/neon_pgversioncompat.h:25:31: note: in expansion of macro ‘BUFFERTAGS_EQUAL’ 25 | #define BufferTagsEqual(a, b) BUFFERTAGS_EQUAL(*(a), *(b)) | ^~~~~~~~~~~~~~~~ pgxn/neon/pagestore_smgr.c:220:34: note: in expansion of macro ‘BufferTagsEqual’ 220 | #define SH_EQUAL(tb, a, b) (BufferTagsEqual(&(a)->buftag, &(b)->buftag)) | ^~~~~~~~~~~~~~~ pg_install/v14/include/postgresql/server/lib/simplehash.h:280:77: note: in expansion of macro ‘SH_EQUAL’ 280 | #define SH_COMPARE_KEYS(tb, ahash, akey, b) (ahash == SH_GET_HASH(tb, b) && SH_EQUAL(tb, b->SH_KEY, akey)) | ^~~~~~~~ pg_install/v14/include/postgresql/server/lib/simplehash.h:799:21: note: in expansion of macro ‘SH_COMPARE_KEYS’ 799 | if (SH_COMPARE_KEYS(tb, hash, key, entry)) | ^~~~~~~~~~~~~~~ pgxn/neon/pagestore_smgr.c: In function ‘neon_read_at_lsn’: pgxn/neon/pagestore_smgr.c:2742:25: note: object ‘buftag’ of size 20 2742 | BufferTag buftag = {0}; | ^~~~~~ This commit silences those warnings, although it's not clear to me why the compiler complained like that in the first place. I found the issue with padding bytes while looking into those warnings, but that was coincidental, I don't think the padding bytes explain the warnings as such. In v16, the BUFFERTAGS_EQUAL macro was replaced with a static inline function, and that also silences the compiler warning. Not clear to me why. --- pgxn/neon/pagestore_smgr.c | 43 +++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/pgxn/neon/pagestore_smgr.c b/pgxn/neon/pagestore_smgr.c index 08b3b53f02..49ac16158e 100644 --- a/pgxn/neon/pagestore_smgr.c +++ b/pgxn/neon/pagestore_smgr.c @@ -803,15 +803,19 @@ prefetch_register_bufferv(BufferTag tag, neon_request_lsns *frlsns, bool is_prefetch) { uint64 min_ring_index; - PrefetchRequest req; + PrefetchRequest hashkey; #if USE_ASSERT_CHECKING bool any_hits = false; #endif /* We will never read further ahead than our buffer can store. */ nblocks = Max(1, Min(nblocks, readahead_buffer_size)); - /* use an intermediate PrefetchRequest struct to ensure correct alignment */ - req.buftag = tag; + /* + * Use an intermediate PrefetchRequest struct as the hash key to ensure + * correct alignment and that the padding bytes are cleared. + */ + memset(&hashkey.buftag, 0, sizeof(BufferTag)); + hashkey.buftag = tag; Retry: min_ring_index = UINT64_MAX; @@ -837,8 +841,8 @@ Retry: slot = NULL; entry = NULL; - req.buftag.blockNum = tag.blockNum + i; - entry = prfh_lookup(MyPState->prf_hash, (PrefetchRequest *) &req); + hashkey.buftag.blockNum = tag.blockNum + i; + entry = prfh_lookup(MyPState->prf_hash, &hashkey); if (entry != NULL) { @@ -849,7 +853,7 @@ Retry: Assert(slot->status != PRFS_UNUSED); Assert(MyPState->ring_last <= ring_index && ring_index < MyPState->ring_unused); - Assert(BUFFERTAGS_EQUAL(slot->buftag, req.buftag)); + Assert(BUFFERTAGS_EQUAL(slot->buftag, hashkey.buftag)); /* * If the caller specified a request LSN to use, only accept @@ -981,7 +985,7 @@ Retry: * We must update the slot data before insertion, because the hash * function reads the buffer tag from the slot. */ - slot->buftag = req.buftag; + slot->buftag = hashkey.buftag; slot->shard_no = get_shard_number(&tag); slot->my_ring_index = ring_index; @@ -2749,14 +2753,19 @@ neon_read_at_lsnv(NRelFileInfo rinfo, ForkNumber forkNum, BlockNumber base_block uint64 ring_index; PrfHashEntry *entry; PrefetchRequest *slot; - BufferTag buftag = {0}; + PrefetchRequest hashkey; Assert(PointerIsValid(request_lsns)); Assert(nblocks >= 1); - CopyNRelFileInfoToBufTag(buftag, rinfo); - buftag.forkNum = forkNum; - buftag.blockNum = base_blockno; + /* + * Use an intermediate PrefetchRequest struct as the hash key to ensure + * correct alignment and that the padding bytes are cleared. + */ + memset(&hashkey.buftag, 0, sizeof(BufferTag)); + CopyNRelFileInfoToBufTag(hashkey.buftag, rinfo); + hashkey.buftag.forkNum = forkNum; + hashkey.buftag.blockNum = base_blockno; /* * The redo process does not lock pages that it needs to replay but are @@ -2774,7 +2783,7 @@ neon_read_at_lsnv(NRelFileInfo rinfo, ForkNumber forkNum, BlockNumber base_block * weren't for the behaviour of the LwLsn cache that uses the highest * value of the LwLsn cache when the entry is not found. */ - prefetch_register_bufferv(buftag, request_lsns, nblocks, mask, false); + prefetch_register_bufferv(hashkey.buftag, request_lsns, nblocks, mask, false); for (int i = 0; i < nblocks; i++) { @@ -2795,8 +2804,8 @@ neon_read_at_lsnv(NRelFileInfo rinfo, ForkNumber forkNum, BlockNumber base_block * Try to find prefetched page in the list of received pages. */ Retry: - buftag.blockNum = blockno; - entry = prfh_lookup(MyPState->prf_hash, (PrefetchRequest *) &buftag); + hashkey.buftag.blockNum = blockno; + entry = prfh_lookup(MyPState->prf_hash, &hashkey); if (entry != NULL) { @@ -2833,7 +2842,7 @@ Retry: { if (entry == NULL) { - ring_index = prefetch_register_bufferv(buftag, reqlsns, 1, NULL, false); + ring_index = prefetch_register_bufferv(hashkey.buftag, reqlsns, 1, NULL, false); Assert(ring_index != UINT64_MAX); slot = GetPrfSlot(ring_index); } @@ -2858,8 +2867,8 @@ Retry: } while (!prefetch_wait_for(ring_index)); Assert(slot->status == PRFS_RECEIVED); - Assert(memcmp(&buftag, &slot->buftag, sizeof(BufferTag)) == 0); - Assert(buftag.blockNum == base_blockno + i); + Assert(memcmp(&hashkey.buftag, &slot->buftag, sizeof(BufferTag)) == 0); + Assert(hashkey.buftag.blockNum == base_blockno + i); resp = slot->response;