Обсуждение: [PATCH] Use DatumGetUInt32() for dsm_attach() in test_shm_mq_main()

On Thu, Jun 26, 2025 at 12:51:10PM -0700, Jianghua Yang wrote:
> The argument passed to `dsm_attach()` is expected to be a `uint32`, but the
> code currently uses `DatumGetInt32()` to extract it from the `Datum`
> argument. This can lead to incorrect behavior when the high bit is set, as
> 'unable to map dynamic shared memory segment'.

I'm not sure this actually causes any problems in practice because
dsm_attach() treats its argument as unsigned.  In any case, I've never seen
this test fail like that, and presumably the high bit is sometimes set
because the handle is generated with a PRNG.

Nevertheless, I see no point in using the wrong macro.  I'll plan on
committing/back-patching this shortly.

-- 
nathan

On Thu, Jun 26, 2025 at 01:46:10PM -0700, Jianghua Yang wrote:
> Just to follow up - in our production system (pg_cron extension),
> we´ve encountered real issues caused by passing a `Datum` to
> `dsm_attach()` using `DatumGetInt32()` instead of `DatumGetUInt32()`.
> 
> Here's a sample of the errors observed in our logs:
> 
> 
> ERROR: unable to map dynamic shared memory segment
> WARNING: one or more background workers failed to start
> 
> 
> These errors trace back to failures in `dsm_attach()`, where the
> segment handle value was incorrectly interpreted due to sign extension
> from `int32`.

I think there might be something else going on.  I added a debug log in
test_shm_mq, and it looks like it regularly uses handles with the high bit
set.  I also wrote a test program and consulted the C standard, which seem
to confirm that passing a signed integer to a function with an unsigned
parameter leaves the high bit set.

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Thu, Jun 26, 2025 at 01:46:10PM -0700, Jianghua Yang wrote:
>> These errors trace back to failures in `dsm_attach()`, where the
>> segment handle value was incorrectly interpreted due to sign extension
>> from `int32`.

> I think there might be something else going on.

I agree with Nathan: the patch you proposed is purely cosmetic.
I don't object to it, but you need to dig deeper because this
will not resolve any actual behavioral problem.

            regards, tom lane

Committed.

-- 
nathan