Horton

Thread

Commits

  1. Remove redefinitions of SIG_* macros in win32_port.h.

  2. Convert libpgport's pqsignal() to a void function.

  3. Avoid calling pqsignal() with invalid signals on Windows frontends.

  4. Avoid symbol collisions between pqsignal.c and legacy-pqsignal.c.

  5. Check that MyProcPid == getpid() in backend signal handlers.

  6. Further refactoring of c.h and nearby files.

  7. Here's the latest win32 signals code, this time in the form of a patch

  8. Add C version of initdb, from Andrew Dunstan.

  9. Allow Win32 to compile under MinGW. Major changes are:

  1. convert libpgport's pqsignal() to a void function

    Nathan Bossart <nathandbossart@gmail.com> — 2025-01-15T02:45:12Z

    (moving to a new thread)

On Mon, Jan 13, 2025 at 10:04:31AM -0600, Nathan Bossart wrote:
> On Sat, Jan 11, 2025 at 02:04:13PM -0500, Tom Lane wrote:
>> BTW, this decouples legacy-pqsignal.c from pqsignal.c enough
>> that we could now do what's contemplated in the comments from
>> 3b00fdba9: simplify that version by making it return void,
>> or perhaps better just a true/false success report.
>> I've not touched that point here, though.
> 
> I think it should just return void since AFAICT nobody checks the return
> value.  But it would probably be a good idea to add some sort of error
> checking within the function.  I've attached a draft patch that adds some
> new assertions.  I originally tried to use elog() where it was available,
> but besides making the code even more unreadable, I think it's unnecessary
> (since AFAICT any problems are likely coding errors).

With commit 9a45a89 in place, this is now possible.  I've attached a new
version of the patch with a more fleshed-out commit message and a small
comment fix.

For background, the protections added by commit 3b00fdb introduced race
conditions to pqsignal() that could lead to bogus return values.  That's of
little consequence since nobody seems to look at the return values, but it
would have been nice to convert it to a void function to avoid any
possibility of a bogus return value.  Unfortunately, doing so would have
required also modifying legacy-pqsignal.c's version of the function, which
would've required an SONAME bump, which didn't seem worth it.  Or so I
thought...

Thanks to commit 9a45a89, legacy-pqsignal.c now has its own dedicated
extern for pqsignal(), which decouples it enough that we can follow through
with changing libpqport's pqsignal() to a void function.

Thoughts?

-- 
nathan

  2. Re: convert libpgport's pqsignal() to a void function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-15T03:02:46Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> Thanks to commit 9a45a89, legacy-pqsignal.c now has its own dedicated
> extern for pqsignal(), which decouples it enough that we can follow through
> with changing libpqport's pqsignal() to a void function.

> Thoughts?

LGTM, although I don't know enough about Windows to know if the
"== SIG_ERR" test in that path is correct.

			regards, tom lane

  3. Re: convert libpgport's pqsignal() to a void function

    Nathan Bossart <nathandbossart@gmail.com> — 2025-01-15T03:57:57Z

    On Tue, Jan 14, 2025 at 10:02:46PM -0500, Tom Lane wrote:
> LGTM, although I don't know enough about Windows to know if the
> "== SIG_ERR" test in that path is correct.

It's apparently not [0].  :(

My guess is that this has something to do with redefining SIG_ERR in
win32_port.h.  We might be able to use push_macro/pop_macro to keep the old
value around, but at the moment I'm leaning towards just removing the
assertion in that path.

[0] https://cirrus-ci.com/task/6237809813487616

-- 
nathan

  4. Re: convert libpgport's pqsignal() to a void function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-15T04:08:05Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> On Tue, Jan 14, 2025 at 10:02:46PM -0500, Tom Lane wrote:
>> LGTM, although I don't know enough about Windows to know if the
>> "== SIG_ERR" test in that path is correct.

> It's apparently not [0].  :(

Bleah.

> My guess is that this has something to do with redefining SIG_ERR in
> win32_port.h.  We might be able to use push_macro/pop_macro to keep the old
> value around, but at the moment I'm leaning towards just removing the
> assertion in that path.

I wonder why we redefine those values?  But I tend to agree that just
removing the test is sufficient for now.  Given the lack of failure
checks in the existing code, and the lack of trouble reports
suggesting any problem, it's hard to muster enthusiasm for spending
a lot of effort on this.

			regards, tom lane

  5. Re: convert libpgport's pqsignal() to a void function

    Nathan Bossart <nathandbossart@gmail.com> — 2025-01-15T19:15:26Z

    On Tue, Jan 14, 2025 at 11:08:05PM -0500, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> My guess is that this has something to do with redefining SIG_ERR in
>> win32_port.h.  We might be able to use push_macro/pop_macro to keep the old
>> value around, but at the moment I'm leaning towards just removing the
>> assertion in that path.
> 
> I wonder why we redefine those values?

I wondered the same.  Those redefines have been there since commit 5049196,
but I haven't been able to find any real discussion in the archives about
it.  Maybe I will bug Magnus about it sometime, in case he happens to
remember the reason.

> But I tend to agree that just
> removing the test is sufficient for now.  Given the lack of failure
> checks in the existing code, and the lack of trouble reports
> suggesting any problem, it's hard to muster enthusiasm for spending
> a lot of effort on this.

Assuming cfbot likes this new version of the patch, I'll commit it shortly.
Thanks for reviewing.

-- 
nathan

  6. Re: convert libpgport's pqsignal() to a void function

    Thomas Munro <thomas.munro@gmail.com> — 2025-01-15T19:21:08Z

    On Thu, Jan 16, 2025 at 8:15 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> On Tue, Jan 14, 2025 at 11:08:05PM -0500, Tom Lane wrote:
> > Nathan Bossart <nathandbossart@gmail.com> writes:
> >> My guess is that this has something to do with redefining SIG_ERR in
> >> win32_port.h.  We might be able to use push_macro/pop_macro to keep the old
> >> value around, but at the moment I'm leaning towards just removing the
> >> assertion in that path.
> >
> > I wonder why we redefine those values?
>
> I wondered the same.  Those redefines have been there since commit 5049196,
> but I haven't been able to find any real discussion in the archives about
> it.  Maybe I will bug Magnus about it sometime, in case he happens to
> remember the reason.

My guess would be: perhaps some ancient version of MinGW didn't define
them?  They're defined by MinGW and native signal.h now and they have
the same values, so we should remove them I think.

Assertion failed: 0, file ../src/port/pqsignal.c, line 147

Could be due to calling native signal() with a signal number other
than the 6 values required to work by the C standard?

  7. Re: convert libpgport's pqsignal() to a void function

    Nathan Bossart <nathandbossart@gmail.com> — 2025-01-15T19:47:18Z

    On Thu, Jan 16, 2025 at 08:21:08AM +1300, Thomas Munro wrote:
> On Thu, Jan 16, 2025 at 8:15 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> On Tue, Jan 14, 2025 at 11:08:05PM -0500, Tom Lane wrote:
>> > I wonder why we redefine those values?
>>
>> I wondered the same.  Those redefines have been there since commit 5049196,
>> but I haven't been able to find any real discussion in the archives about
>> it.  Maybe I will bug Magnus about it sometime, in case he happens to
>> remember the reason.
> 
> My guess would be: perhaps some ancient version of MinGW didn't define
> them?  They're defined by MinGW and native signal.h now and they have
> the same values, so we should remove them I think.

Okay.  If nothing else, it'd be interesting to see what the buildfarm
thinks.

> Assertion failed: 0, file ../src/port/pqsignal.c, line 147
> 
> Could be due to calling native signal() with a signal number other
> than the 6 values required to work by the C standard?

Looking closer, that probably makes more sense than my SIG_ERR redefinition
theory.  If that assertion is getting hit, that means signal() _is_
returning SIG_ERR (either the system one or our redefined version), and it
looks like it's pretty common to use -1 for SIG_ERR.  That'd only affect
Windows frontend programs, but it still sounds scary.  I'll try getting
more details about the error with some custom cfbot runs.

-- 
nathan

  8. Re: convert libpgport's pqsignal() to a void function

    Nathan Bossart <nathandbossart@gmail.com> — 2025-01-15T20:56:44Z

    On Wed, Jan 15, 2025 at 01:47:18PM -0600, Nathan Bossart wrote:
> On Thu, Jan 16, 2025 at 08:21:08AM +1300, Thomas Munro wrote:
>> Could be due to calling native signal() with a signal number other
>> than the 6 values required to work by the C standard?
> 
> Looking closer, that probably makes more sense than my SIG_ERR redefinition
> theory.  If that assertion is getting hit, that means signal() _is_
> returning SIG_ERR (either the system one or our redefined version), and it
> looks like it's pretty common to use -1 for SIG_ERR.  That'd only affect
> Windows frontend programs, but it still sounds scary.  I'll try getting
> more details about the error with some custom cfbot runs.

I think this is what's happening.  cfbot's test failures are caused by
initdb's setup_signals().  The call to pqsignal(SIGHUP, trapsig) seems to
fail because SIGHUP isn't a real signal number, just something that's made
up in win32_port.h.  This SIGHUP definition was added by commit 12c9423 in
May 2003, then the pqsignal(SIGHUP, ...) call was added in initdb by commit
279598b in November 2003, but it might not have been broken at that time
because it doesn't look like initdb.c included the SIGHUP #define.  In any
case, I think this has been broken since at least commit ed9b360 (November
2017), if not earlier.

Perhaps we should surround all those extra signal #defines in win32_port.h
with an #ifndef FRONTEND.  initdb seems to be good about avoiding
pqsignal() calls if the signal doesn't exist, but I wouldn't be surprised
if there are other frontend programs that are not so cautious.  I'll give
it a try on cfbot and see what breaks...

-- 
nathan

  9. Re: convert libpgport's pqsignal() to a void function

    Thomas Munro <thomas.munro@gmail.com> — 2025-01-15T22:07:41Z

    On Thu, Jan 16, 2025 at 8:47 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> On Thu, Jan 16, 2025 at 08:21:08AM +1300, Thomas Munro wrote:
> > Could be due to calling native signal() with a signal number other
> > than the 6 values required to work by the C standard?
>
> Looking closer, that probably makes more sense than my SIG_ERR redefinition
> theory.  If that assertion is getting hit, that means signal() _is_
> returning SIG_ERR (either the system one or our redefined version), and it
> looks like it's pretty common to use -1 for SIG_ERR.  That'd only affect
> Windows frontend programs, but it still sounds scary.  I'll try getting
> more details about the error with some custom cfbot runs.

Windows barely has signals, so I don't think it's too scary...  SIGINT
works but surprisingly runs the handler in another thread when you
press ^C, SIGABRT, SIGFPE, SIGSEGV presumably have the obvious
synchronous/exception-trapping implementations but we aren't even
trying to catch those*, and SIGTERM, SIGILL are pro forma, never
generated by the system.  We've basically just invented more pro forma
ones that will also never be generated except in the backend's
entirely separate fake signal system that I hope to remove.   +1 for
your idea of not defining them at all outside the backend, it's just
confusing noise.

The second surprising thing, unless you're an armchair Unix
archeologist, is that all but SIGFPE revert to SIG_DFL when they fire,
like POSIX's SA_RESETHAND mode.  I don't know if it also has
SA_NODEFER behaviour (a combination of behaviours seen in some old
Unixen of yestermillennium: your handler had to keep reinstalling
itself, cf initdb.c:trapsig, but for a brief window the default
process-terminating-core-dumping-nasal-daemon behaviour was installed
and there was nothing you could do about that race; the BSD crew fixed
that mistake a long time ago, everyone does it that way now, and POSIX
sigaction() made those policies explicit and is the recommended
replacement, but to this day the C standard has just signal() with
undefined semantics and no requirement that any of it even work).

*Even in the backend we don't catch Windows' native SIGFPE AFAICS, so
I guess those must exit instead of being converted to an ERROR by
FloatExceptionHandler.  I wonder if there is a way to reach that
condition from a SQL query.

  10. Re: convert libpgport's pqsignal() to a void function

    Nathan Bossart <nathandbossart@gmail.com> — 2025-01-15T23:47:51Z

    On Thu, Jan 16, 2025 at 11:07:41AM +1300, Thomas Munro wrote:
> +1 for your idea of not defining them at all outside the backend, it's
> just confusing noise.

I tried that, but these extra signals are needed even in the frontend for
pgkill(), etc.  My next thought was to simply ignore signal() errors for
the extra signals, but I don't think that's very nice because it just masks
broken code.  Finally, I settled on modifying initdb's setup_signals() to
use WIN32 checks instead of checking for the signals themselves.  This is
how it's done elsewhere, and this is apparently all that's needed to make
cfbot's Windows run happy.

I've also attached a 0002 that removes the redefinitions of SIG_ERR and
friends.  That looks good on cfbot, but we'll see what the buildfarm has to
say...

-- 
nathan

  11. Re: convert libpgport's pqsignal() to a void function

    Thomas Munro <thomas.munro@gmail.com> — 2025-01-16T01:18:44Z

    On Thu, Jan 16, 2025 at 12:47 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
> On Thu, Jan 16, 2025 at 11:07:41AM +1300, Thomas Munro wrote:
> > +1 for your idea of not defining them at all outside the backend, it's
> > just confusing noise.
>
> I tried that, but these extra signals are needed even in the frontend for
> pgkill(), etc.

Right, of course.

(In an unconf in Vancouver, a group of us were plotting that there
should be a more explicit control socket to talk to the postmaster, on
all OSes, to get current status, recovery progress, etc, and also
support shutdown commands more explicitly.  If you separately turn all
the backend IPC signals into interrupts (CF 5118 and further work like
that), and these cluster-level ones you just mentioned into commands
in the hypothetical new cluster control protocol, then you don't need
fake signals for Windows anymore, but apparently I got ahead of myself
with that comment :-))

>  My next thought was to simply ignore signal() errors for
> the extra signals, but I don't think that's very nice because it just masks
> broken code.  Finally, I settled on modifying initdb's setup_signals() to
> use WIN32 checks instead of checking for the signals themselves.  This is
> how it's done elsewhere, and this is apparently all that's needed to make
> cfbot's Windows run happy.

Cool.

> I've also attached a 0002 that removes the redefinitions of SIG_ERR and
> friends.  That looks good on cfbot, but we'll see what the buildfarm has to
> say...

Ditto.

  12. Re: convert libpgport's pqsignal() to a void function

    Nathan Bossart <nathandbossart@gmail.com> — 2025-01-17T03:03:43Z

    I've now committed all of this.  I ended up finding a couple other frontend
programs that called pqsignal() with an invalid signal number on Windows,
so I fixed those as well.  AFAICT the reason I didn't catch them in my
earlier testing is because they aren't tested!  I'll keep an eye on the
buildfarm for any problems with the SIG_* redefinition removal, too.

Thanks for reviewing and for sharing lots of context about this stuff.

-- 
nathan