On 28.11.25 10:06, Bertrand Drouvot wrote:
Hi,
On Fri, Nov 28, 2025 at 09:11:16AM +0100, Peter Eisentraut wrote:
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.
Indeed, that's a nice simplification.
- data += sizeof(uint32) * 2;
Is it safe? I mean could XLH_SPLIT_META_UPDATE_MASKS and
XLH_SPLIT_META_UPDATE_SPLITPOINT
be set simultaneously?
Yes, that's what was probably intended. But apparently not exercised in
the tests.
So maybe more like this patch.
From 4d1ade22d4fa45daa5e6e3bfc2c701f9536d0a95 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Fri, 28 Nov 2025 14:18:37 +0100
Subject: [PATCH v2] Simplify hash_xlog_split_allocate_page()
---
src/backend/access/hash/hash_xlog.c | 30 ++++++++++++-----------------
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/src/backend/access/hash/hash_xlog.c
b/src/backend/access/hash/hash_xlog.c
index 2a0145f3c9b..440220dca56 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,41 +373,37 @@ hash_xlog_split_allocate_page(XLogReaderState *record)
{
Page page;
HashMetaPage metap;
+ Size datalen;
+ char *data;
+ uint32 *uidata;
+ int uidatacount;
page = BufferGetPage(metabuf);
metap = HashPageGetMeta(page);
metap->hashm_maxbucket = xlrec->new_bucket;
data = XLogRecGetBlockData(record, 2, &datalen);
+ uidata = (uint32 *) data;
+ uidatacount = 0;
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 lowmask = uidata[uidatacount++];
+ uint32 highmask = uidata[uidatacount++];
/* 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 ovflpoint = uidata[uidatacount++];
+ uint32 ovflpages = uidata[uidatacount++];
/* update metapage */
- metap->hashm_spares[ovflpoint] = *ovflpages;
metap->hashm_ovflpoint = ovflpoint;
+ metap->hashm_spares[ovflpoint] = ovflpages;
}
MarkBufferDirty(metabuf);
--
2.52.0
