> On Apr 15, 2026, at 04:38, Andrew Dunstan <[email protected]> wrote:
>
>
> On 2026-04-14 Tu 6:40 AM, Jakub Wartak wrote:
>> Hi Andres, Andrew, Chao
>>
>> thanks for putting so much effort into enhancing the the previous
>> implementation
>> of this code. Earlier I was not aware of potential problems involved.
>>
>> On Fri, Apr 10, 2026 at 9:41 AM Chao Li <[email protected]> wrote:
>>>
>>>
>>>> On Apr 9, 2026, at 18:59, Andrew Dunstan <[email protected]> wrote:
>>>>
>>>>
>>>> On 2026-04-08 We 1:01 PM, Andres Freund wrote:
>>>>> Hi,
>>>>>
>>>>> Attached is a very rough first draft for how I think this needs to look
>>>>> like.
>>>>>
>>>>> Basically, SIGNAL_INFO always will pass both the signal number and
>>>>> extended
>>>>> information along to the signal handler. The extended information is a
>>>>> postgres specific struct. If the platform can't provide the extended
>>>>> information, the values are instead set to some default value indicating
>>>>> that
>>>>> the information is not known.
>>>>>
>>>>> With that die() (and also StatementCancelHandler, ...) can just set
>>>>> whatever
>>>>> globals it wants, without pqsignal.c needing to know about it.
>>>>>
>>>>> It also allows us to extend the amount of information in the future. E.g.
>>>>> I'd
>>>>> like to log the reason for a segfault (could e.g. be an OOM kill or an
>>>>> umapped
>>>>> region) to stderr.
>>>>>
>>>>> The annoying thing about it is needing to change nearly all the existing
>>>>> references to SIG_IGN/SIG_DFL, to avoid warnings due to mismatched types.
>>>> I agree that's annoying. The only way around it I found was via some
>>>> casting to/from void* that I suspect you would find a cure worse than the
>>>> disease.
>>>> I reworked your patch slightly. This version fixes the translatability
>>>> issue you raised earlier, makes the TAP test from the original commit more
>>>> robust, and tries to resolve your XXX issue by moving the assignment of
>>>> ProcDieSenderPid/Uid inside the "if (!proc_exit_inprogress)" block.
>>>>
>>> I reviewed this version. Besides the compile warning and uid 0 issues, I
>>> got a few more comments, so I try to put them all together as below.
>>>
>> TL;DR; win/mingw is really unhappy with the state of rework patch right now.
>> I've tried to enhance and fix it, and now it's green for default CI run and
>> also for mingw too. Attached 0002-fixup-win32 patch does not incorporate
>> Chao's
>> all findings so far.
>>
>> [..]
>>> 2 - uid 0 problem
>>> ```
>>> +typedef struct pg_signal_info
>>> +{
>>> + pid_t pid; /* pid of sending process
>>> or 0 if unknown */
>>> + uid_t uid; /* uid of sending process
>>> or 0 if unknown */
>>> +} pg_signal_info;
>>> ```
>>>
>>> I think we can mention that “uid” is only meaningful when pid is set.
>> [..]
>>> 5
>>> ```
>>> + pg_info.uid = 0;
>>> ```
>>>
>>> If you take comment 2, then when unavailable, we can just don’t assign
>>> anything to pg_info.uid. Or "(uid_t)-1", maybe.
>>>
>>
>> I. I've started from this and I vaguley remembered that I had terrible
>> experience
>> when trying to chose the proper types for those field types, but I couldn't
>> remind myself why, so I've gave a try of the current rework patch and got
>> this
>> on Windows Server 2022, vs2019, cirrus-ci said:
>>
>> [08:40:22.848] c:\cirrus\src\include\c.h(1451): error C2061: syntax
>> error: identifier 'pid_t'
>> [08:40:22.848] c:\cirrus\src\include\c.h(1452): error C2061: syntax
>> error: identifier 'uid'
>> [08:40:22.848] c:\cirrus\src\include\c.h(1452): error C2059: syntax error:
>> ';'
>> [08:40:22.848] c:\cirrus\src\include\c.h(1453): error C2059: syntax error:
>> '}'
>>
>> for c.h:
>> 1449 typedef struct pg_signal_info
>> 1450 {
>> 1451 pid_t pid; /* pid of
>> sending process or 0 if unknown */
>> 1452 uid_t uid; /* uid of
>> sending process or 0 if unknown */
>> 1453 } pg_signal_info;
>>
>> so maybe we should just move that typedef with SIGNAL_ARGS to after
>> "include of port.h" (line ~1471) in that c.h, because only then we'll have
>> access to:
>> src/include/port/win32_port.h:typedef int pid_t;
>> src/include/port/win32_port.h:typedef int uid_t;
>> but the problem is that port.h itself requires SIGNAL_ARGS to be defined
>> and that seems to be like chicken and egg problem. I thought that just
>> using native "ints" could be the way to go, but the problem is that now
>> that uid_t can be bigger than pid_t as Linux kernel headers show this:
>>
>> x86_64-linux-gnu/bits/types.h:#define __S32_TYPE int
>> x86_64-linux-gnu/bits/types.h:#define __U32_TYPE unsigned int
>> x86_64-linux-gnu/bits/types.h:__STD_TYPE __UID_T_TYPE __uid_t; /*
>> Type of user identifications. */
>> x86_64-linux-gnu/bits/types.h:__STD_TYPE __PID_T_TYPE __pid_t; /*
>> Type of process identifications. */
>> x86_64-linux-gnu/bits/typesizes.h:#define __UID_T_TYPE __U32_TYPE
>> x86_64-linux-gnu/bits/typesizes.h:#define __PID_T_TYPE __S32_TYPE
>>
>> so maybe do that typedef struct pg_signal_info with 2x uint32_t and that's
>> good enough? (it covers the ranges necessary and makes it platform
>> compatible)
>>
>> II. While we are tthis so with above uid_t of up to being u32, possibly
>> `volatile int ProcDieSenderUid/Pid` should be also bigger like uint32_t?
>> My fixup-patch does not incude it, because I don't know really.
>>
>> III. As Chao said I've used:
>>
>> +#define PG_SIG_DFL (pqsigfunc) (pg_funcptr_t) SIG_DFL
>> +#define PG_SIG_IGN (pqsigfunc) (pg_funcptr_t) SIG_IGN
>>
>> IV. Then just got lots of warnings for use_wrapper/wrapp_handler and so on
>>
>> [09:27:33.602] ../src/port/pqsignal.c(206): error C2065:
>> 'use_wrapper': undeclared identifier
>> [09:27:33.602] ../src/port/pqsignal.c(206): warning C4113: 'pqsigfunc'
>> differs in parameter lists from 'void (__cdecl *)(int)'
>>
>> that's for:
>> 204 #else
>> 205 /* Forward to Windows native signal system. */
>> 206 if (signal(signo, use_wrapper ? wrapper_handler :
>> func) == SIG_ERR)
>> 207 Assert(false); /* probably
>> indicates coding error */
>>
>> In the end, I've ended up using wrapper_handler for the windows path there
>> as signal() requires function with single param.
>>
>> IV. Got some further issues, and VC complained that siginfo_t is used for
>> USE_SIGACTION - isn't it impossible on win32? Anyway, that makes some sense,
>> USE_SIGINFO is not defined and we use 'siginfo_t', so something like fixes
>> it:
>>
>> @@ -90,7 +90,7 @@ static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
>> *
>> * This wrapper also handles restoring the value of errno.
>> */
>> -#ifdef USE_SIGACTION
>> +#if defined(USE_SIGACTION) && defined(USE_SIGINFO)
>> static void
>> wrapper_handler(int postgres_signal_arg, siginfo_t *info, void *context)
>> #else
>>
>> V. Later I've stumbled on series of other problems related to
>> src/backend/port/win32/signal.c (it also uses SIG_DFL but not PG_SIG_DFL and
>> stil somewhat references pgsigfunc, so those changes seemed to impact it).
>>
>> Also there was:
>> [10:37:02.824] ../src/backend/port/win32/signal.c(154): error C2198:
>> 'sig': too few arguments for call
>>
>> so I've fixed with adding nodata struct there and:
>> @@ -151,7 +154,7 @@ pgwin32_dispatch_queued_signals(void)
>> block_mask |= sigmask(i);
>>
>> sigprocmask(SIG_BLOCK,
>> &block_mask, &save_mask);
>> - sig(i);
>> + sig(i, &nodata);
>> sigprocmask(SIG_SETMASK,
>> &save_mask, NULL);
>>
>> VI. Possibly we could rename USE_SIGACTION define because at least to me it
>> is confusing to me to reason and communicate about in terms of win32 context
>> on win32 do we do have it or not? (it can be both ways):
>> * win32 C API doesnt have it
>> * PG does have sigaction win32 wrapper with override macro
>> #define sigaction.. pqsigaction..
>> * however src/include/libpq/pqsignal.h says "sa_sigaction not yet
>> implemented"
>> for it (so with USE_SIGACTION are we talking about sa_sigaction field
>> memeber
>> that it's not used or about sigaction function?)
>>
>> VII. FWIW, I've also removed superflous "else"
>> #ifdef USE_SIGINFO
>> if (!(is_ign || is_dfl))
>> {
>> act.sa_sigaction = wrapper_handler;
>> act.sa_flags |= SA_SIGINFO;
>> }
>> - else
>> #else
>>
>>
>
>
> I'm not 100% sure that else shouldn't be there. Maybe have another look?
>
> Attached is a consolidation that includes your other fixes, plus:
>
> . uid comment -- "only meaningful when pid is not 0"
> . const pg_signal_info *pg_siginfo in SIGNAL_ARGS
> . 2 spaces after period in syncrep.c errdetail
>
>
> cheers
>
>
> andrew
>
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
> <v2-0001-Rework-signal-handler-infrastructure-to-pass-send.patch>
V2 LGTM.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
