On Tue, Apr 14, 2026 at 10:38 PM 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?
So in 0001 we had pqsignal() like below:
```
#ifdef USE_SIGINFO
if (!(is_ign || is_dfl))
{
act.sa_sigaction = wrapper_handler;
act.sa_flags |= SA_SIGINFO;
}
else // <--- this is the problematic line that I wanted to remove
#else
else
act.sa_handler = wrapper_handler;
#endif
#ifdef SA_NOCLDSTOP
if (signo == SIGCHLD)
act.sa_flags |= SA_NOCLDSTOP;
#endif
```
(in latest v2-0001 it seems to be refactored and looks more readable to me)
IMHO it was superflous for the following reasons:
a) previous releases did not have such condition (flow went unconditially to
`if (signo == SIGCHLD)` and not based on function spec. in a lot of places we
call `pqsignal(SIGCHLD, PG_SIG_DFL)` but in PostmasterMain() we call
`pqsignal(SIGCHLD, handle_pm_child_exit_signal);` so we should set
SA_NOCLDSTOP in both cases not just one.
b) if the platform wouldn't have SA_NOCLDSTOP, so stil with USE_SIGACTION (it
would have to be basically non POSIX.1-1990, but not WIN32, so impossible?),
that would shift the code flow to later lines (sigaction() itself) so
that would be a bug
I would say I would even propose removal of #ifdef SA_NOCLDSTOP and assume it's
always there, as personally I hate such macro complexity.
BTW tiny nitpick to myself:
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -206,7 +206,7 @@ pqsignal(int signo, pqsigfunc func)
* Forward to Windows native signal system, we need to send this though
* wrapper handler as it it needs to take single argument only.
*/
- if(is_ign)
+ if (is_ign)
> 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
v2-0001 LGTM.
-J.
