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.
