On 25.11.25 06:46, Bertrand Drouvot wrote:
@@ -389,7 +389,7 @@ hash_xlog_split_allocate_page(XLogReaderState *record)
/* extract low and high masks. */
memcpy(&lowmask, data, sizeof(uint32));
- highmask = (uint32 *) ((char *) data + sizeof(uint32));
+ highmask = (uint32 *) (data + sizeof(uint32));
I wonder about these, too. I like knowing what the code does without
having to check the type of `data`. But then later on we do a `data +=
sizeof(uint32) * 2`, so you have to check the type anyway, so... I
don't know.
I think that even with the cast in place, it's good to check the type of data.
Not for the line that follows (i.e: "data += sizeof(uint32) * 2") but to check
that the cast makes sense and does not hide "wrong" pointer manipulation.
So I think that with or without the cast one would need to check. But that feels
more natural to check when there is no cast (as we don't assume that someone
said "I know what I'm doing"). So I'm in favor of removing the cast, thoughts?
I think this whole thing could be simplified by overlaying a uint32 over
"data" and just accessing the array fields normally. See attached patch.
From 01d999144692ce3af845882bbe32754f260281fb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Fri, 28 Nov 2025 09:05:48 +0100
Subject: [PATCH] Simplify hash_xlog_split_allocate_page()
---
src/backend/access/hash/hash_xlog.c | 28 ++++++++++------------------
1 file changed, 10 insertions(+), 18 deletions(-)
diff --git a/src/backend/access/hash/hash_xlog.c
b/src/backend/access/hash/hash_xlog.c
index 2a0145f3c9b..fa6b80d3a83 100644
--- a/src/backend/access/hash/hash_xlog.c
+++ b/src/backend/access/hash/hash_xlog.c
@@ -314,8 +314,6 @@ hash_xlog_split_allocate_page(XLogReaderState *record)
Buffer oldbuf;
Buffer newbuf;
Buffer metabuf;
- Size datalen PG_USED_FOR_ASSERTS_ONLY;
- char *data;
XLogRedoAction action;
/*
@@ -375,6 +373,8 @@ hash_xlog_split_allocate_page(XLogReaderState *record)
{
Page page;
HashMetaPage metap;
+ Size datalen;
+ char *data;
page = BufferGetPage(metabuf);
metap = HashPageGetMeta(page);
@@ -384,31 +384,23 @@ hash_xlog_split_allocate_page(XLogReaderState *record)
if (xlrec->flags & XLH_SPLIT_META_UPDATE_MASKS)
{
- uint32 lowmask;
- uint32 *highmask;
-
- /* extract low and high masks. */
- memcpy(&lowmask, data, sizeof(uint32));
- highmask = (uint32 *) ((char *) data + sizeof(uint32));
+ uint32 *mask_data = (uint32 *) data;
+ uint32 lowmask = mask_data[0];
+ uint32 highmask = mask_data[1];
/* update metapage */
metap->hashm_lowmask = lowmask;
- metap->hashm_highmask = *highmask;
-
- data += sizeof(uint32) * 2;
+ metap->hashm_highmask = highmask;
}
if (xlrec->flags & XLH_SPLIT_META_UPDATE_SPLITPOINT)
{
- uint32 ovflpoint;
- uint32 *ovflpages;
-
- /* extract information of overflow pages. */
- memcpy(&ovflpoint, data, sizeof(uint32));
- ovflpages = (uint32 *) ((char *) data + sizeof(uint32));
+ uint32 *ovfl_data = (uint32 *) data;
+ uint32 ovflpoint = ovfl_data[0];
+ uint32 ovflpages = ovfl_data[1];
/* update metapage */
- metap->hashm_spares[ovflpoint] = *ovflpages;
+ metap->hashm_spares[ovflpoint] = ovflpages;
metap->hashm_ovflpoint = ovflpoint;
}
--
2.52.0
