On Fri, Oct 21, 2022 at 7:52 AM Kyotaro Horiguchi
<horikyota....@gmail.com> wrote:
>
> > +1. How about emitting a message like its friend pg_receivewal, like
> > the attached patch?
>
> I'm not a fan of treating SIGINT as an error in this case. It calls
> prepareToTerminate() when time_to_abort and everything goes fine after
> then. So I think we should do the same thing after receiving an
> interrupt. This also does file-sync naturally as a part of normal
> shutdown. I'm also not a fan of doing fsync at error.
I think the pg_recvlogical can gracefully exit on both SIGINT and
SIGTERM to keep things simple.
> > > I also then noticed that we don't fsync the output file in cases of
> > > errors -
> > > that seems wrong to me? Looks to me like that block should be moved till
> > > after
> > > the error:?
> >
> > How about something like the attached patch?
The attached patch (pg_recvlogical_graceful_interrupt.text) has a
couple of problems, I believe. We're losing prepareToTerminate() with
keepalive true and we're not skipping pg_log_error("unexpected
termination of replication stream: %s" upon interrupt, after all we're
here discussing how to avoid it.
I came up with the attached v2 patch, please have a look.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 56e25373796b114254f5995701b07b05381f28ef Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sat, 22 Oct 2022 08:35:16 +0000
Subject: [PATCH v2] pg_recvlogical fixes
---
src/bin/pg_basebackup/pg_recvlogical.c | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 5f2e6af445..849e9d9071 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -54,7 +54,8 @@ static const char *plugin = "test_decoding";
/* Global State */
static int outfd = -1;
-static volatile sig_atomic_t time_to_abort = false;
+static bool time_to_abort = false;
+static volatile sig_atomic_t ready_to_exit = false;
static volatile sig_atomic_t output_reopen = false;
static bool output_isfile;
static TimestampTz output_last_fsync = -1;
@@ -283,6 +284,23 @@ StreamLogicalLog(void)
copybuf = NULL;
}
+ /* When we get SIGINT/SIGTERM, we exit */
+ if (ready_to_exit)
+ {
+ /*
+ * Try informing the server about our exit, but don't wait around
+ * or retry on failure.
+ */
+ (void) PQputCopyEnd(conn, NULL);
+ (void) PQflush(conn);
+ time_to_abort = ready_to_exit;
+
+ if (verbose)
+ pg_log_info("received interrupt signal, exiting");
+
+ break;
+ }
+
/*
* Potentially send a status message to the primary.
*/
@@ -614,7 +632,8 @@ StreamLogicalLog(void)
res = PQgetResult(conn);
}
- if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ if (!ready_to_exit &&
+ PQresultStatus(res) != PGRES_COMMAND_OK)
{
pg_log_error("unexpected termination of replication stream: %s",
PQresultErrorMessage(res));
@@ -656,7 +675,7 @@ error:
static void
sigexit_handler(SIGNAL_ARGS)
{
- time_to_abort = true;
+ ready_to_exit = true;
}
/*
--
2.34.1
