pg_receivewal and SIGTERM
There's a smallish backup tool called pg_backupcluster in Debian's
postgresql-common which also ships a systemd service that runs
pg_receivewal for wal archiving, and supplies a pg_getwal script for
reading the files back on restore, including support for .partial
files.
So far the machinery was using plain files and relied on compressing
the WALs from time to time, but now I wanted to compress the files
directly from pg_receivewal --compress=5. Unfortunately this broke the
regression tests that include a test for the .partial files where
pg_receivewal.service is shut down before the segment is full.
The problem was that systemd's default KillSignal is SIGTERM, while
pg_receivewal flushes the output compression buffers on SIGINT only.
The attached patch makes it do the same for SIGTERM as well. (Most
places in PG that install a SIGINT handler also install a SIGTERM
handler already.)
Christoph
Attachments:
0001-pg_receivewal-Exit-cleanly-on-SIGTERM.patchtext/x-diff; charset=us-asciiDownload
From 8e42bf5458ae00050327da07c09a77649f24e36d Mon Sep 17 00:00:00 2001
From: Christoph Berg <myon@debian.org>
Date: Mon, 15 Aug 2022 14:29:43 +0200
Subject: [PATCH] pg_receivewal: Exit cleanly on SIGTERM
Compressed output is only flushed on clean exits. The reason to support
SIGTERM here as well is that pg_receivewal might well be running as a
daemon, and systemd's default KillSignal is SIGTERM.
---
doc/src/sgml/ref/pg_receivewal.sgml | 8 +++++---
src/bin/pg_basebackup/pg_receivewal.c | 1 +
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_receivewal.sgml
index 4fe9e1a874..5f83ba1893 100644
--- a/doc/src/sgml/ref/pg_receivewal.sgml
+++ b/doc/src/sgml/ref/pg_receivewal.sgml
@@ -118,8 +118,9 @@ PostgreSQL documentation
<para>
In the absence of fatal errors, <application>pg_receivewal</application>
- will run until terminated by the <systemitem>SIGINT</systemitem> signal
- (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>).
+ will run until terminated by the <systemitem>SIGINT</systemitem>
+ (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>)
+ or <systemitem>SIGTERM</systemitem> signal.
</para>
</refsect1>
@@ -457,7 +458,8 @@ PostgreSQL documentation
<para>
<application>pg_receivewal</application> will exit with status 0 when
- terminated by the <systemitem>SIGINT</systemitem> signal. (That is the
+ terminated by the <systemitem>SIGINT</systemitem> or
+ <systemitem>SIGTERM</systemitem> signal. (That is the
normal way to end it. Hence it is not an error.) For fatal errors or
other signals, the exit status will be nonzero.
</para>
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index f064cff4ab..4acd0654b9 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -933,6 +933,7 @@ main(int argc, char **argv)
*/
#ifndef WIN32
pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGTERM, sigint_handler);
#endif
/*
--
2.35.1
On 15 Aug 2022, at 14:45, Christoph Berg <myon@debian.org> wrote:
The problem was that systemd's default KillSignal is SIGTERM, while
pg_receivewal flushes the output compression buffers on SIGINT only.
Supporting SIGTERM here makes sense, especially given how systemd works.
The attached patch makes it do the same for SIGTERM as well. (Most
places in PG that install a SIGINT handler also install a SIGTERM
handler already.)
Not really when it comes to utilities though; initdb, pg_dump and pg_test_fsync
seems to be the ones doing so. (That's probably mostly due to them not running
in a daemon-like way as what's discussed here.)
Do you think pg_recvlogical should support SIGTERM as well? (The signals which
it does trap should be added to the documentation which just now says "until
terminated by a signal" but that's a separate thing.)
pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGTERM, sigint_handler);
Tiny nitpick, I think we should rename sigint_handler to just sig_handler as it
does handle more than sigint.
In relation to this. Reading over this and looking around I realized that the
documentation for pg_waldump lacks a closing parenthesis on Ctrl+C so I will be
pushing the below to fix it:
--- a/doc/src/sgml/ref/pg_waldump.sgml
+++ b/doc/src/sgml/ref/pg_waldump.sgml
@@ -263,7 +263,7 @@ PostgreSQL documentation
<para>
If <application>pg_waldump</application> is terminated by signal
<systemitem>SIGINT</systemitem>
- (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>,
+ (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>),
the summary of the statistics computed is displayed up to the
termination point. This operation is not supported on
<productname>Windows</productname>.--
Daniel Gustafsson https://vmware.com/
Re: Daniel Gustafsson
Do you think pg_recvlogical should support SIGTERM as well? (The signals which
it does trap should be added to the documentation which just now says "until
terminated by a signal" but that's a separate thing.)
Ack, that makes sense, added in the attached updated patch.
pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGTERM, sigint_handler);
Tiny nitpick, I think we should rename sigint_handler to just sig_handler as it
does handle more than sigint.
I went with sigexit_handler since pg_recvlogical has also a
sighup_handler and "sig_handler" would be confusing there.
Christoph
Attachments:
0001-pg_receivewal-pg_recvlogical-Exit-cleanly-on-SIGTERM.patchtext/x-diff; charset=us-asciiDownload
From beb3bcb32a9eabf2bf83e259706f89ccdac276d3 Mon Sep 17 00:00:00 2001
From: Christoph Berg <myon@debian.org>
Date: Mon, 15 Aug 2022 14:29:43 +0200
Subject: [PATCH] pg_receivewal, pg_recvlogical: Exit cleanly on SIGTERM
In pg_receivewal, compressed output is only flushed on clean exits. The
reason to support SIGTERM here as well is that pg_receivewal might well
be running as a daemon, and systemd's default KillSignal is SIGTERM.
Since pg_recvlogical is also supposed to run as a daemon, teach it about
SIGTERM as well.
---
doc/src/sgml/ref/pg_receivewal.sgml | 8 +++++---
src/bin/pg_basebackup/pg_receivewal.c | 7 ++++---
src/bin/pg_basebackup/pg_recvlogical.c | 7 ++++---
3 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_receivewal.sgml
index 4fe9e1a874..5f83ba1893 100644
--- a/doc/src/sgml/ref/pg_receivewal.sgml
+++ b/doc/src/sgml/ref/pg_receivewal.sgml
@@ -118,8 +118,9 @@ PostgreSQL documentation
<para>
In the absence of fatal errors, <application>pg_receivewal</application>
- will run until terminated by the <systemitem>SIGINT</systemitem> signal
- (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>).
+ will run until terminated by the <systemitem>SIGINT</systemitem>
+ (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>)
+ or <systemitem>SIGTERM</systemitem> signal.
</para>
</refsect1>
@@ -457,7 +458,8 @@ PostgreSQL documentation
<para>
<application>pg_receivewal</application> will exit with status 0 when
- terminated by the <systemitem>SIGINT</systemitem> signal. (That is the
+ terminated by the <systemitem>SIGINT</systemitem> or
+ <systemitem>SIGTERM</systemitem> signal. (That is the
normal way to end it. Hence it is not an error.) For fatal errors or
other signals, the exit status will be nonzero.
</para>
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index f064cff4ab..e2cf924017 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -673,13 +673,13 @@ StreamLog(void)
}
/*
- * When sigint is called, just tell the system to exit at the next possible
+ * When SIGINT/SIGTERM are caught, just tell the system to exit at the next possible
* moment.
*/
#ifndef WIN32
static void
-sigint_handler(int signum)
+sigexit_handler(int signum)
{
time_to_stop = true;
}
@@ -932,7 +932,8 @@ main(int argc, char **argv)
* if one is needed, in GetConnection.)
*/
#ifndef WIN32
- pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGINT, sigexit_handler);
+ pqsignal(SIGTERM, sigexit_handler);
#endif
/*
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 2a4c8b130a..363102cf85 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -650,11 +650,11 @@ error:
#ifndef WIN32
/*
- * When sigint is called, just tell the system to exit at the next possible
+ * When SIGINT/SIGTERM are caught, just tell the system to exit at the next possible
* moment.
*/
static void
-sigint_handler(int signum)
+sigexit_handler(int signum)
{
time_to_abort = true;
}
@@ -922,7 +922,8 @@ main(int argc, char **argv)
* if one is needed, in GetConnection.)
*/
#ifndef WIN32
- pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGINT, sigexit_handler);
+ pqsignal(SIGTERM, sigexit_handler);
pqsignal(SIGHUP, sighup_handler);
#endif
--
2.35.1
On Tue, Aug 16, 2022 at 5:06 PM Christoph Berg <myon@debian.org> wrote:
Re: Daniel Gustafsson
Do you think pg_recvlogical should support SIGTERM as well? (The signals which
it does trap should be added to the documentation which just now says "until
terminated by a signal" but that's a separate thing.)Ack, that makes sense, added in the attached updated patch.
pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGTERM, sigint_handler);
Tiny nitpick, I think we should rename sigint_handler to just sig_handler as it
does handle more than sigint.I went with sigexit_handler since pg_recvlogical has also a
sighup_handler and "sig_handler" would be confusing there.
Can we move these signal handlers to streamutil.h/.c so that both
pg_receivewal and pg_recvlogical can make use of it avoiding duplicate
code?
--
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/
On 16 Aug 2022, at 13:40, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
On Tue, Aug 16, 2022 at 5:06 PM Christoph Berg <myon@debian.org> wrote:
I went with sigexit_handler since pg_recvlogical has also a
sighup_handler and "sig_handler" would be confusing there.Can we move these signal handlers to streamutil.h/.c so that both
pg_receivewal and pg_recvlogical can make use of it avoiding duplicate
code?
In general that's a good idea, but they are so trivial that I don't really see
much point in doing that in this particular case.
--
Daniel Gustafsson https://vmware.com/
Re: Daniel Gustafsson
In general that's a good idea, but they are so trivial that I don't really see
much point in doing that in this particular case.
Plus the variable they set is called differently...
Christoph
On 16 Aug 2022, at 13:36, Christoph Berg <myon@debian.org> wrote:
pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGTERM, sigint_handler);
Tiny nitpick, I think we should rename sigint_handler to just sig_handler as it
does handle more than sigint.I went with sigexit_handler since pg_recvlogical has also a
sighup_handler and "sig_handler" would be confusing there.
Good point, sigexit_handler is a better name here.
--
Daniel Gustafsson https://vmware.com/
On Tue, Aug 16, 2022 at 5:15 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 16 Aug 2022, at 13:36, Christoph Berg <myon@debian.org> wrote:
pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGTERM, sigint_handler);
Tiny nitpick, I think we should rename sigint_handler to just sig_handler as it
does handle more than sigint.I went with sigexit_handler since pg_recvlogical has also a
sighup_handler and "sig_handler" would be confusing there.Good point, sigexit_handler is a better name here.
+1.
Don't we need a similar explanation [1]<para> In the absence of fatal errors, <application>pg_receivewal</application> - will run until terminated by the <systemitem>SIGINT</systemitem> signal - (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>). + will run until terminated by the <systemitem>SIGINT</systemitem> + (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>) + or <systemitem>SIGTERM</systemitem> signal. for pg_recvlogical docs?
[1]
<para>
In the absence of fatal errors, <application>pg_receivewal</application>
- will run until terminated by the <systemitem>SIGINT</systemitem> signal
- (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>).
+ will run until terminated by the <systemitem>SIGINT</systemitem>
+ (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>)
+ or <systemitem>SIGTERM</systemitem> signal.--
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/
Re: Bharath Rupireddy
Don't we need a similar explanation [1] for pg_recvlogical docs?
[1] <para> In the absence of fatal errors, <application>pg_receivewal</application> - will run until terminated by the <systemitem>SIGINT</systemitem> signal - (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>). + will run until terminated by the <systemitem>SIGINT</systemitem> + (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>) + or <systemitem>SIGTERM</systemitem> signal.
Coped that from pg_receivewal(1) now.
Christoph
Attachments:
0001-pg_receivewal-pg_recvlogical-Exit-cleanly-on-SIGTERM.patchtext/x-diff; charset=us-asciiDownload
From 6c51c559a86754623cd33fe4cef3563c18fccea3 Mon Sep 17 00:00:00 2001
From: Christoph Berg <myon@debian.org>
Date: Mon, 15 Aug 2022 14:29:43 +0200
Subject: [PATCH] pg_receivewal, pg_recvlogical: Exit cleanly on SIGTERM
In pg_receivewal, compressed output is only flushed on clean exits. The
reason to support SIGTERM here as well is that pg_receivewal might well
be running as a daemon, and systemd's default KillSignal is SIGTERM.
Since pg_recvlogical is also supposed to run as a daemon, teach it about
SIGTERM as well.
---
doc/src/sgml/ref/pg_receivewal.sgml | 8 +++++---
doc/src/sgml/ref/pg_recvlogical.sgml | 7 +++++++
src/bin/pg_basebackup/pg_receivewal.c | 7 ++++---
src/bin/pg_basebackup/pg_recvlogical.c | 7 ++++---
4 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_receivewal.sgml
index 4fe9e1a874..5f83ba1893 100644
--- a/doc/src/sgml/ref/pg_receivewal.sgml
+++ b/doc/src/sgml/ref/pg_receivewal.sgml
@@ -118,8 +118,9 @@ PostgreSQL documentation
<para>
In the absence of fatal errors, <application>pg_receivewal</application>
- will run until terminated by the <systemitem>SIGINT</systemitem> signal
- (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>).
+ will run until terminated by the <systemitem>SIGINT</systemitem>
+ (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>)
+ or <systemitem>SIGTERM</systemitem> signal.
</para>
</refsect1>
@@ -457,7 +458,8 @@ PostgreSQL documentation
<para>
<application>pg_receivewal</application> will exit with status 0 when
- terminated by the <systemitem>SIGINT</systemitem> signal. (That is the
+ terminated by the <systemitem>SIGINT</systemitem> or
+ <systemitem>SIGTERM</systemitem> signal. (That is the
normal way to end it. Hence it is not an error.) For fatal errors or
other signals, the exit status will be nonzero.
</para>
diff --git a/doc/src/sgml/ref/pg_recvlogical.sgml b/doc/src/sgml/ref/pg_recvlogical.sgml
index 1a88225409..012ee4468b 100644
--- a/doc/src/sgml/ref/pg_recvlogical.sgml
+++ b/doc/src/sgml/ref/pg_recvlogical.sgml
@@ -46,6 +46,13 @@ PostgreSQL documentation
a slot without consuming it, use
<link linkend="functions-replication"><function>pg_logical_slot_peek_changes</function></link>.
</para>
+
+ <para>
+ In the absence of fatal errors, <application>pg_recvlogical</application>
+ will run until terminated by the <systemitem>SIGINT</systemitem>
+ (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>)
+ or <systemitem>SIGTERM</systemitem> signal.
+ </para>
</refsect1>
<refsect1>
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index f064cff4ab..e2cf924017 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -673,13 +673,13 @@ StreamLog(void)
}
/*
- * When sigint is called, just tell the system to exit at the next possible
+ * When SIGINT/SIGTERM are caught, just tell the system to exit at the next possible
* moment.
*/
#ifndef WIN32
static void
-sigint_handler(int signum)
+sigexit_handler(int signum)
{
time_to_stop = true;
}
@@ -932,7 +932,8 @@ main(int argc, char **argv)
* if one is needed, in GetConnection.)
*/
#ifndef WIN32
- pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGINT, sigexit_handler);
+ pqsignal(SIGTERM, sigexit_handler);
#endif
/*
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 2a4c8b130a..363102cf85 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -650,11 +650,11 @@ error:
#ifndef WIN32
/*
- * When sigint is called, just tell the system to exit at the next possible
+ * When SIGINT/SIGTERM are caught, just tell the system to exit at the next possible
* moment.
*/
static void
-sigint_handler(int signum)
+sigexit_handler(int signum)
{
time_to_abort = true;
}
@@ -922,7 +922,8 @@ main(int argc, char **argv)
* if one is needed, in GetConnection.)
*/
#ifndef WIN32
- pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGINT, sigexit_handler);
+ pqsignal(SIGTERM, sigexit_handler);
pqsignal(SIGHUP, sighup_handler);
#endif
--
2.35.1
On Tue, Aug 16, 2022 at 7:26 PM Christoph Berg <myon@debian.org> wrote:
Re: Bharath Rupireddy
Don't we need a similar explanation [1] for pg_recvlogical docs?
[1] <para> In the absence of fatal errors, <application>pg_receivewal</application> - will run until terminated by the <systemitem>SIGINT</systemitem> signal - (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>). + will run until terminated by the <systemitem>SIGINT</systemitem> + (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>) + or <systemitem>SIGTERM</systemitem> signal.Coped that from pg_receivewal(1) now.
Thanks.
<application>pg_receivewal</application> will exit with status 0 when
- terminated by the <systemitem>SIGINT</systemitem> signal. (That is the
+ terminated by the <systemitem>SIGINT</systemitem> or
+ <systemitem>SIGTERM</systemitem> signal. (That is the
normal way to end it. Hence it is not an error.) For fatal errors or
other signals, the exit status will be nonzero.Can we specify the reason in the docs why a SIGTERM causes (which
typically would cause a program to end with non-zero exit code)
pg_receivewal and pg_recvlogical exit with zero exit code? Having this
in the commit message would help developers but the documentation will
help users out there.
Thoughts?
[1]: pg_receivewal, pg_recvlogical: Exit cleanly on SIGTERM
pg_receivewal, pg_recvlogical: Exit cleanly on SIGTERM
In pg_receivewal, compressed output is only flushed on clean exits. The
reason to support SIGTERM here as well is that pg_receivewal might well
be running as a daemon, and systemd's default KillSignal is SIGTERM.
Since pg_recvlogical is also supposed to run as a daemon, teach it about
SIGTERM as well.
--
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/
Re: Bharath Rupireddy
<application>pg_receivewal</application> will exit with status 0 when - terminated by the <systemitem>SIGINT</systemitem> signal. (That is the + terminated by the <systemitem>SIGINT</systemitem> or + <systemitem>SIGTERM</systemitem> signal. (That is the normal way to end it. Hence it is not an error.) For fatal errors or other signals, the exit status will be nonzero.Can we specify the reason in the docs why a SIGTERM causes (which
typically would cause a program to end with non-zero exit code)
pg_receivewal and pg_recvlogical exit with zero exit code? Having this
in the commit message would help developers but the documentation will
help users out there.
We could add "because you want that if it's running as a daemon", but
TBH, I'd rather remove the parentheses part. It sounds too much like
"it works that way because that way is the sane way".
Christoph
On Fri, Aug 19, 2022 at 4:24 PM Christoph Berg <myon@debian.org> wrote:
Re: Bharath Rupireddy
<application>pg_receivewal</application> will exit with status 0 when - terminated by the <systemitem>SIGINT</systemitem> signal. (That is the + terminated by the <systemitem>SIGINT</systemitem> or + <systemitem>SIGTERM</systemitem> signal. (That is the normal way to end it. Hence it is not an error.) For fatal errors or other signals, the exit status will be nonzero.Can we specify the reason in the docs why a SIGTERM causes (which
typically would cause a program to end with non-zero exit code)
pg_receivewal and pg_recvlogical exit with zero exit code? Having this
in the commit message would help developers but the documentation will
help users out there.We could add "because you want that if it's running as a daemon", but
+1 to add "some" info in the docs (I'm not sure about the better
wording though), we can try to be more specific of the use case if
required.
--
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/
On Fri, Aug 19, 2022 at 05:34:56PM +0530, Bharath Rupireddy wrote:
+1 to add "some" info in the docs (I'm not sure about the better
wording though), we can try to be more specific of the use case if
required.
Yes, the amount of extra docs provided by the patch proposed by
Christoph looks fine by me.
FWIW, grouping the signal handlers into a common area like
streamutil.c seems rather confusing to me, as they set different
variable names that rely on their own assumptions in their local file,
so I would leave that out, like the patch.
While looking at the last patch proposed, it strikes me that
time_to_stop should be sig_atomic_t in pg_receivewal.c, as the safe
type of variable to set in a signal handler. We could change that,
while on it..
Backpatching this stuff is not an issue here.
--
Michael
Re: Michael Paquier
While looking at the last patch proposed, it strikes me that
time_to_stop should be sig_atomic_t in pg_receivewal.c, as the safe
type of variable to set in a signal handler. We could change that,
while on it..
Done in the attached patch.
Backpatching this stuff is not an issue here.
Do you mean it can, or can not be backpatched? (I'd argue for
backpatching since the behaviour is slightly broken at the moment.)
Christoph
Attachments:
0001-pg_receivewal-pg_recvlogical-Exit-cleanly-on-SIGTERM.patchtext/x-diff; charset=us-asciiDownload
From 3d17d6f77566047fc63e05c2f33106a09b2c9793 Mon Sep 17 00:00:00 2001
From: Christoph Berg <myon@debian.org>
Date: Mon, 15 Aug 2022 14:29:43 +0200
Subject: [PATCH] pg_receivewal, pg_recvlogical: Exit cleanly on SIGTERM
In pg_receivewal, compressed output is only flushed on clean exits. The
reason to support SIGTERM here as well is that pg_receivewal might well
be running as a daemon, and systemd's default KillSignal is SIGTERM.
Since pg_recvlogical is also supposed to run as a daemon, teach it about
SIGTERM as well.
While at it, change pg_receivewal's time_to_stop to sig_atomic_t like in
pg_recvlogical.
---
doc/src/sgml/ref/pg_receivewal.sgml | 8 +++++---
doc/src/sgml/ref/pg_recvlogical.sgml | 7 +++++++
src/bin/pg_basebackup/pg_receivewal.c | 9 +++++----
src/bin/pg_basebackup/pg_recvlogical.c | 7 ++++---
4 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_receivewal.sgml
index 4fe9e1a874..5f83ba1893 100644
--- a/doc/src/sgml/ref/pg_receivewal.sgml
+++ b/doc/src/sgml/ref/pg_receivewal.sgml
@@ -118,8 +118,9 @@ PostgreSQL documentation
<para>
In the absence of fatal errors, <application>pg_receivewal</application>
- will run until terminated by the <systemitem>SIGINT</systemitem> signal
- (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>).
+ will run until terminated by the <systemitem>SIGINT</systemitem>
+ (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>)
+ or <systemitem>SIGTERM</systemitem> signal.
</para>
</refsect1>
@@ -457,7 +458,8 @@ PostgreSQL documentation
<para>
<application>pg_receivewal</application> will exit with status 0 when
- terminated by the <systemitem>SIGINT</systemitem> signal. (That is the
+ terminated by the <systemitem>SIGINT</systemitem> or
+ <systemitem>SIGTERM</systemitem> signal. (That is the
normal way to end it. Hence it is not an error.) For fatal errors or
other signals, the exit status will be nonzero.
</para>
diff --git a/doc/src/sgml/ref/pg_recvlogical.sgml b/doc/src/sgml/ref/pg_recvlogical.sgml
index 1a88225409..012ee4468b 100644
--- a/doc/src/sgml/ref/pg_recvlogical.sgml
+++ b/doc/src/sgml/ref/pg_recvlogical.sgml
@@ -46,6 +46,13 @@ PostgreSQL documentation
a slot without consuming it, use
<link linkend="functions-replication"><function>pg_logical_slot_peek_changes</function></link>.
</para>
+
+ <para>
+ In the absence of fatal errors, <application>pg_recvlogical</application>
+ will run until terminated by the <systemitem>SIGINT</systemitem>
+ (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>)
+ or <systemitem>SIGTERM</systemitem> signal.
+ </para>
</refsect1>
<refsect1>
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index f064cff4ab..4b37a98fd6 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -45,7 +45,7 @@ static int verbose = 0;
static int compresslevel = 0;
static int noloop = 0;
static int standby_message_timeout = 10 * 1000; /* 10 sec = default */
-static volatile bool time_to_stop = false;
+static volatile sig_atomic_t time_to_stop = false;
static bool do_create_slot = false;
static bool slot_exists_ok = false;
static bool do_drop_slot = false;
@@ -673,13 +673,13 @@ StreamLog(void)
}
/*
- * When sigint is called, just tell the system to exit at the next possible
+ * When SIGINT/SIGTERM are caught, just tell the system to exit at the next possible
* moment.
*/
#ifndef WIN32
static void
-sigint_handler(int signum)
+sigexit_handler(int signum)
{
time_to_stop = true;
}
@@ -932,7 +932,8 @@ main(int argc, char **argv)
* if one is needed, in GetConnection.)
*/
#ifndef WIN32
- pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGINT, sigexit_handler);
+ pqsignal(SIGTERM, sigexit_handler);
#endif
/*
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 2a4c8b130a..363102cf85 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -650,11 +650,11 @@ error:
#ifndef WIN32
/*
- * When sigint is called, just tell the system to exit at the next possible
+ * When SIGINT/SIGTERM are caught, just tell the system to exit at the next possible
* moment.
*/
static void
-sigint_handler(int signum)
+sigexit_handler(int signum)
{
time_to_abort = true;
}
@@ -922,7 +922,8 @@ main(int argc, char **argv)
* if one is needed, in GetConnection.)
*/
#ifndef WIN32
- pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGINT, sigexit_handler);
+ pqsignal(SIGTERM, sigexit_handler);
pqsignal(SIGHUP, sighup_handler);
#endif
--
2.35.1
On Mon, Aug 22, 2022 at 04:05:16PM +0200, Christoph Berg wrote:
Do you mean it can, or can not be backpatched? (I'd argue for
backpatching since the behaviour is slightly broken at the moment.)
I mean that it is fine to backpatch that, in my opinion.
--
Michael
On 23 Aug 2022, at 02:15, Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Aug 22, 2022 at 04:05:16PM +0200, Christoph Berg wrote:
Do you mean it can, or can not be backpatched? (I'd argue for
backpatching since the behaviour is slightly broken at the moment.)I mean that it is fine to backpatch that, in my opinion.
I think this can be argued both for and against backpatching. Catching SIGTERM
makes a lot of sense, especially given systemd's behavior. On the other hand,
This adds functionality to something arguably working as intended, regardless
of what one thinks about the intent.
The attached adds the Exit Status section to pg_recvlogical docs which is
present in pg_receivewal to make them more aligned, and tweaks comments to
pgindent standards. This is the version I think is ready to commit.
--
Daniel Gustafsson https://vmware.com/
Attachments:
v2-0001-Handle-SIGTERM-in-pg_receivewal-and-pg_recvlogica.patchapplication/octet-stream; name=v2-0001-Handle-SIGTERM-in-pg_receivewal-and-pg_recvlogica.patch; x-unix-mode=0644Download
From c5eec5541170e2e1ba46fe97b62308e610ad1d0d Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Thu, 25 Aug 2022 11:05:16 +0200
Subject: [PATCH v2] Handle SIGTERM in pg_receivewal and pg_recvlogical
In pg_receivewal, compressed output is only flushed on clean exits. The
reason to support SIGTERM as well as SIGINT (which is currently handled)
is that pg_receivewal might well be running as a daemon, and systemd's
default KillSignal is SIGTERM.
Since pg_recvlogical is also supposed to run as a daemon, teach it about
SIGTERM as well. While there, change pg_receivewal's time_to_stop to be
sig_atomic_t like in pg_recvlogical.
Author: Christoph Berg <myon@debian.org>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/Yvo/5No5S0c4EFMj@msg.df7cb.de
---
doc/src/sgml/ref/pg_receivewal.sgml | 8 +++++---
doc/src/sgml/ref/pg_recvlogical.sgml | 18 ++++++++++++++++++
src/bin/pg_basebackup/pg_receivewal.c | 11 ++++++-----
src/bin/pg_basebackup/pg_recvlogical.c | 9 +++++----
4 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_receivewal.sgml
index 4fe9e1a874..5f83ba1893 100644
--- a/doc/src/sgml/ref/pg_receivewal.sgml
+++ b/doc/src/sgml/ref/pg_receivewal.sgml
@@ -118,8 +118,9 @@ PostgreSQL documentation
<para>
In the absence of fatal errors, <application>pg_receivewal</application>
- will run until terminated by the <systemitem>SIGINT</systemitem> signal
- (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>).
+ will run until terminated by the <systemitem>SIGINT</systemitem>
+ (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>)
+ or <systemitem>SIGTERM</systemitem> signal.
</para>
</refsect1>
@@ -457,7 +458,8 @@ PostgreSQL documentation
<para>
<application>pg_receivewal</application> will exit with status 0 when
- terminated by the <systemitem>SIGINT</systemitem> signal. (That is the
+ terminated by the <systemitem>SIGINT</systemitem> or
+ <systemitem>SIGTERM</systemitem> signal. (That is the
normal way to end it. Hence it is not an error.) For fatal errors or
other signals, the exit status will be nonzero.
</para>
diff --git a/doc/src/sgml/ref/pg_recvlogical.sgml b/doc/src/sgml/ref/pg_recvlogical.sgml
index 1a88225409..6d75b6fa4c 100644
--- a/doc/src/sgml/ref/pg_recvlogical.sgml
+++ b/doc/src/sgml/ref/pg_recvlogical.sgml
@@ -46,6 +46,13 @@ PostgreSQL documentation
a slot without consuming it, use
<link linkend="functions-replication"><function>pg_logical_slot_peek_changes</function></link>.
</para>
+
+ <para>
+ In the absence of fatal errors, <application>pg_recvlogical</application>
+ will run until terminated by the <systemitem>SIGINT</systemitem>
+ (<keycombo action="simul"><keycap>Control</keycap><keycap>C</keycap></keycombo>)
+ or <systemitem>SIGTERM</systemitem> signal.
+ </para>
</refsect1>
<refsect1>
@@ -407,6 +414,17 @@ PostgreSQL documentation
</para>
</refsect1>
+ <refsect1>
+ <title>Exit Status</title>
+ <para>
+ <application>pg_recvlogical</application> will exit with status 0 when
+ terminated by the <systemitem>SIGINT</systemitem> or
+ <systemitem>SIGTERM</systemitem> signal. (That is the
+ normal way to end it. Hence it is not an error.) For fatal errors or
+ other signals, the exit status will be nonzero.
+ </para>
+ </refsect1>
+
<refsect1>
<title>Environment</title>
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index f064cff4ab..b0eb1d378b 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -45,7 +45,7 @@ static int verbose = 0;
static int compresslevel = 0;
static int noloop = 0;
static int standby_message_timeout = 10 * 1000; /* 10 sec = default */
-static volatile bool time_to_stop = false;
+static volatile sig_atomic_t time_to_stop = false;
static bool do_create_slot = false;
static bool slot_exists_ok = false;
static bool do_drop_slot = false;
@@ -673,13 +673,13 @@ StreamLog(void)
}
/*
- * When sigint is called, just tell the system to exit at the next possible
- * moment.
+ * When SIGINT/SIGTERM are caught, just tell the system to exit at the next
+ * possible moment.
*/
#ifndef WIN32
static void
-sigint_handler(int signum)
+sigexit_handler(int signum)
{
time_to_stop = true;
}
@@ -932,7 +932,8 @@ main(int argc, char **argv)
* if one is needed, in GetConnection.)
*/
#ifndef WIN32
- pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGINT, sigexit_handler);
+ pqsignal(SIGTERM, sigexit_handler);
#endif
/*
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 2a4c8b130a..a86739ec12 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -650,11 +650,11 @@ error:
#ifndef WIN32
/*
- * When sigint is called, just tell the system to exit at the next possible
- * moment.
+ * When SIGINT/SIGTERM are caught, just tell the system to exit at the next
+ * possible moment.
*/
static void
-sigint_handler(int signum)
+sigexit_handler(int signum)
{
time_to_abort = true;
}
@@ -922,7 +922,8 @@ main(int argc, char **argv)
* if one is needed, in GetConnection.)
*/
#ifndef WIN32
- pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGINT, sigexit_handler);
+ pqsignal(SIGTERM, sigexit_handler);
pqsignal(SIGHUP, sighup_handler);
#endif
--
2.32.1 (Apple Git-133)
On Thu, Aug 25, 2022 at 11:19:05AM +0200, Daniel Gustafsson wrote:
I think this can be argued both for and against backpatching. Catching SIGTERM
makes a lot of sense, especially given systemd's behavior. On the other hand,
This adds functionality to something arguably working as intended, regardless
of what one thinks about the intent.
Sure. My view on this matter is that the behavior of the patch is
more useful to users as, on HEAD, a SIGTERM is equivalent to a drop of
the connection followed by a retry when not using -n. Or do you think
that there could be cases where the behavior of HEAD (force a
connection drop with the backend and handle the retry infinitely in
pg_receivewal/recvlogical) is more useful? systemd can also do
retries a certain given of times, so that's moving the ball one layer
to the other, at the end. We could also say to just set KillSignal to
SIGINT in the docs, but my guess is that few users would actually
notice that until they see how pg_receiwal/recvlogical work with
systemd's default.
FWIW, I've worked on an archiver integration a few years ago and got
annoyed that we use SIGINT while SIGTERM was the default (systemd was
not directly used there but the signal problem was the same, so we had
to go through some loops to make the stop signal configurable, like
systemd).
--
Michael
Re: Michael Paquier
FWIW, I've worked on an archiver integration a few years ago and got
annoyed that we use SIGINT while SIGTERM was the default (systemd was
not directly used there but the signal problem was the same, so we had
to go through some loops to make the stop signal configurable, like
systemd).
SIGTERM is really the default for any init system or run-a-daemon system.
Christoph
On Thu, Aug 25, 2022 at 5:13 PM Christoph Berg <myon@debian.org> wrote:
Re: Michael Paquier
FWIW, I've worked on an archiver integration a few years ago and got
annoyed that we use SIGINT while SIGTERM was the default (systemd was
not directly used there but the signal problem was the same, so we had
to go through some loops to make the stop signal configurable, like
systemd).SIGTERM is really the default for any init system or run-a-daemon system.
It is, but there is also precedent for not using it for graceful
shutdown. Apache, for example, will do what we do today on SIGTERM and
you use SIGWINCH to make it shut down gracefully (which would be the
equivalent of us flushing the compression buffers, I'd say).
I'm not saying we shouldn't change -- I fully approve of making the
change. But the world is full of fairly prominent examples of the
other way as well.
I'm leaning towards considering it a feature-change and thus not
something to backpatch (I'd be OK sneaking it into 15 though, as that
one is not released yet and it feels like a perfectly *safe* change).
Not enough to insist on it, but it seems "slightly more correct".
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
On Thu, Aug 25, 2022 at 08:45:05PM +0200, Magnus Hagander wrote:
I'm leaning towards considering it a feature-change and thus not
something to backpatch (I'd be OK sneaking it into 15 though, as that
one is not released yet and it feels like a perfectly *safe* change).
Not enough to insist on it, but it seems "slightly more correct".
Fine by me if both you and Daniel want to be more careful with this
change. We could always argue about a backpatch later if there is
more ask for it, as well.
--
Michael
Re: Daniel Gustafsson
The attached adds the Exit Status section to pg_recvlogical docs which is
present in pg_receivewal to make them more aligned, and tweaks comments to
pgindent standards. This is the version I think is ready to commit.
Looks good to me.
Thanks,
Christoph
On Fri, Aug 26, 2022 at 09:51:26AM +0900, Michael Paquier wrote:
Fine by me if both you and Daniel want to be more careful with this
change. We could always argue about a backpatch later if there is
more ask for it, as well.
Daniel, are you planning to apply this one on HEAD?
--
Michael
On 2 Sep 2022, at 10:00, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Aug 26, 2022 at 09:51:26AM +0900, Michael Paquier wrote:
Fine by me if both you and Daniel want to be more careful with this
change. We could always argue about a backpatch later if there is
more ask for it, as well.Daniel, are you planning to apply this one on HEAD?
Yes, it's on my TODO for this CF.
--
Daniel Gustafsson https://vmware.com/
On 2 Sep 2022, at 10:00, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Aug 26, 2022 at 09:51:26AM +0900, Michael Paquier wrote:
Fine by me if both you and Daniel want to be more careful with this
change. We could always argue about a backpatch later if there is
more ask for it, as well.Daniel, are you planning to apply this one on HEAD?
I had another look over this and pushed it.
--
Daniel Gustafsson https://vmware.com/