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? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
