pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedpg_basebackup behavior on non-existent slot
12+ messages / 7 participants
[nested] [flat]
Commits from this thread:
2022-02-23 0475a97f74 Quick exit on log stream child exit in pg_basebackup
Related threads (commitfest entry Trap SIGCHLD in pg_basebackup to exit early on stream child failure/3302):
Trap errors from streaming child in pg_basebackup to exit early
* pg_basebackup behavior on non-existent slot
@ 2021-08-21 13:15 Gerard H. Pille <ghpille@hotmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Gerard H. Pille @ 2021-08-21 13:15 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hallo,
I was just confronted with this behaviour described back in 2017, and
the participants in the thread seem to consider it a bug. But I'm
running on version 13. So, how did that discussion end?
My case:
@ pg_basebackup -h 192.168.1.131 -D $PWD/13/main -U repuser -v -P -S stream
Password:
pg_basebackup: initiating base backup, waiting for checkpoint to complete
pg_basebackup: checkpoint completed
pg_basebackup: write-ahead log start point: 23/4E000028 on timeline 1
pg_basebackup: starting background WAL receiver
pg_basebackup: error: could not send replication command
"START_REPLICATION": ERROR: replication slot "stream" does not exist
53790996/53790996 kB (100%), 2/2 tablespaces
pg_basebackup: write-ahead log end point: 23/4E000138
pg_basebackup: waiting for background process to finish streaming ...
pg_basebackup: error: child process exited with exit code 1
pg_basebackup: removing data directory "/var/lib/postgresql/13/main"
pg_basebackup: changes to tablespace directories will not be undone
The old thread:
https://postgrespro.com/list/thread-id/2337189#CAMkU=1wSxYBNFY9TzuVh3=mDLr4BBsMct6wcViNMH+-6Xon4Uw@m...
Kind regards,
Gerard
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_basebackup behavior on non-existent slot
@ 2021-08-23 03:54 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Gerard H. Pille <ghpille@hotmail.com>
0 siblings, 2 replies; 12+ messages in thread
From: Masahiko Sawada @ 2021-08-23 03:54 UTC (permalink / raw)
To: Gerard H. Pille <ghpille@hotmail.com>; +Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Hi,
On Sat, Aug 21, 2021 at 10:15 PM Gerard H. Pille <ghpille@hotmail.com> wrote:
>
> Hallo,
>
> I was just confronted with this behaviour described back in 2017, and
> the participants in the thread seem to consider it a bug. But I'm
> running on version 13. So, how did that discussion end?
>
> My case:
> @ pg_basebackup -h 192.168.1.131 -D $PWD/13/main -U repuser -v -P -S stream
> Password:
> pg_basebackup: initiating base backup, waiting for checkpoint to complete
> pg_basebackup: checkpoint completed
> pg_basebackup: write-ahead log start point: 23/4E000028 on timeline 1
> pg_basebackup: starting background WAL receiver
> pg_basebackup: error: could not send replication command
> "START_REPLICATION": ERROR: replication slot "stream" does not exist
> 53790996/53790996 kB (100%), 2/2 tablespaces
>
> pg_basebackup: write-ahead log end point: 23/4E000138
> pg_basebackup: waiting for background process to finish streaming ...
> pg_basebackup: error: child process exited with exit code 1
> pg_basebackup: removing data directory "/var/lib/postgresql/13/main"
> pg_basebackup: changes to tablespace directories will not be undone
>
> The old thread:
> https://postgrespro.com/list/thread-id/2337189#CAMkU=1wSxYBNFY9TzuVh3=mDLr4BBsMct6wcViNMH+-6Xon4Uw@m...
It seems it's not fixed yet even in HEAD as far as I tested. There
were some ideas to fix that on that thread but the main point was how
to fix it on Windows. I guess that since it creates a transient slot
it’s not a common case to specify a non-existence slot in pg_baseback
but what is your use case? This might help motivate to fix this issue.
BTW in that thread, there was a discussion on how to detect the
streamer process failure in the main process but probably we can fix
this by just doing an existence check for the specified name
replication slot before starting the streamer process?
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_basebackup behavior on non-existent slot
@ 2021-08-23 07:11 Michael Paquier <michael@paquier.xyz>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
1 sibling, 2 replies; 12+ messages in thread
From: Michael Paquier @ 2021-08-23 07:11 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Gerard H. Pille <ghpille@hotmail.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Mon, Aug 23, 2021 at 12:54:58PM +0900, Masahiko Sawada wrote:
> On Sat, Aug 21, 2021 at 10:15 PM Gerard H. Pille <ghpille@hotmail.com> wrote:
>> The old thread:
>> https://postgrespro.com/list/thread-id/2337189#CAMkU=1wSxYBNFY9TzuVh3=mDLr4BBsMct6wcViNMH+-6Xon4Uw@m...
A link to postgresql.org does the same work:
https://www.postgresql.org/message-id/CAMkU=1wSxYBNFY9TzuVh3=mDLr4BBsMct6wcViNMH+-6Xon4Uw@mail.gmail...
No need to redirect that elsewhere.
> It seems it's not fixed yet even in HEAD as far as I tested. There
> were some ideas to fix that on that thread but the main point was how
> to fix it on Windows. I guess that since it creates a transient slot
> it’s not a common case to specify a non-existence slot in pg_baseback
> but what is your use case? This might help motivate to fix this issue.
>
> BTW in that thread, there was a discussion on how to detect the
> streamer process failure in the main process but probably we can fix
> this by just doing an existence check for the specified name
> replication slot before starting the streamer process?
Yeah. Honestly, I am not really excited to redesign this part of base
backups just to take care of a side problem that is mitigated for most
users with the use of a temporary slot. It would be simpler, as you
say, to find a way to detect if the slot wanted exists before even
launching START_REPLICATION with the specified slot on the second
thread copying the WAL, but that would be a new thing as one cannot do
catalog lookups with a physical replication session. Some ideas here
may be to extend the result of IDENTIFY_SYSTEM to return the slots
available in a text array, invent a new command called SLOT, use SHOW
or perhaps do an extra CREATE_REPLICATION_SLOT TEMPORARY from the main
thread.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_basebackup behavior on non-existent slot
@ 2021-08-23 14:51 Gerard H. Pille <ghpille@hotmail.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
1 sibling, 0 replies; 12+ messages in thread
From: Gerard H. Pille @ 2021-08-23 14:51 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Hallo,
my "use case" was forgetting the "-C" option whilst creating a rather large standby database over a slow network connection. I didn't interrupt it since it seemed to continue without problem until al the work was done, half a day later.
Switching to a gigabit connection allowed me to repeat this much faster, and find a solution. I'm just learning about replication on Postgres.
If this is too complex to fix, perhaps adding a warning to the error message:
"pg_basebackup: error: could not send replication command, replication will fail" ?
Thanks!
Gerard
________________________________________
Van: Masahiko Sawada <sawada.mshk@gmail.com>
Verzonden: maandag 23 augustus 2021 5:54
Aan: Gerard H. Pille
CC: PostgreSQL mailing lists
Onderwerp: Re: pg_basebackup behavior on non-existent slot
Hi,
On Sat, Aug 21, 2021 at 10:15 PM Gerard H. Pille <ghpille@hotmail.com> wrote:
>
> Hallo,
>
> I was just confronted with this behaviour described back in 2017, and
> the participants in the thread seem to consider it a bug. But I'm
> running on version 13. So, how did that discussion end?
>
> My case:
> @ pg_basebackup -h 192.168.1.131 -D $PWD/13/main -U repuser -v -P -S stream
> Password:
> pg_basebackup: initiating base backup, waiting for checkpoint to complete
> pg_basebackup: checkpoint completed
> pg_basebackup: write-ahead log start point: 23/4E000028 on timeline 1
> pg_basebackup: starting background WAL receiver
> pg_basebackup: error: could not send replication command
> "START_REPLICATION": ERROR: replication slot "stream" does not exist
> 53790996/53790996 kB (100%), 2/2 tablespaces
>
> pg_basebackup: write-ahead log end point: 23/4E000138
> pg_basebackup: waiting for background process to finish streaming ...
> pg_basebackup: error: child process exited with exit code 1
> pg_basebackup: removing data directory "/var/lib/postgresql/13/main"
> pg_basebackup: changes to tablespace directories will not be undone
>
> The old thread:
> https://postgrespro.com/list/thread-id/2337189#CAMkU=1wSxYBNFY9TzuVh3=mDLr4BBsMct6wcViNMH+-6Xon4Uw@m...
It seems it's not fixed yet even in HEAD as far as I tested. There
were some ideas to fix that on that thread but the main point was how
to fix it on Windows. I guess that since it creates a transient slot
it’s not a common case to specify a non-existence slot in pg_baseback
but what is your use case? This might help motivate to fix this issue.
BTW in that thread, there was a discussion on how to detect the
streamer process failure in the main process but probably we can fix
this by just doing an existence check for the specified name
replication slot before starting the streamer process?
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_basebackup behavior on non-existent slot
@ 2021-08-24 05:06 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 12+ messages in thread
From: Masahiko Sawada @ 2021-08-24 05:06 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Gerard H. Pille <ghpille@hotmail.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Mon, Aug 23, 2021 at 4:11 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Aug 23, 2021 at 12:54:58PM +0900, Masahiko Sawada wrote:
> > On Sat, Aug 21, 2021 at 10:15 PM Gerard H. Pille <ghpille@hotmail.com> wrote:
> >> The old thread:
> >> https://postgrespro.com/list/thread-id/2337189#CAMkU=1wSxYBNFY9TzuVh3=mDLr4BBsMct6wcViNMH+-6Xon4Uw@m...
>
> A link to postgresql.org does the same work:
> https://www.postgresql.org/message-id/CAMkU=1wSxYBNFY9TzuVh3=mDLr4BBsMct6wcViNMH+-6Xon4Uw@mail.gmail...
> No need to redirect that elsewhere.
>
> > It seems it's not fixed yet even in HEAD as far as I tested. There
> > were some ideas to fix that on that thread but the main point was how
> > to fix it on Windows. I guess that since it creates a transient slot
> > it’s not a common case to specify a non-existence slot in pg_baseback
> > but what is your use case? This might help motivate to fix this issue.
> >
> > BTW in that thread, there was a discussion on how to detect the
> > streamer process failure in the main process but probably we can fix
> > this by just doing an existence check for the specified name
> > replication slot before starting the streamer process?
>
> Yeah. Honestly, I am not really excited to redesign this part of base
> backups just to take care of a side problem that is mitigated for most
> users with the use of a temporary slot.
I think that this problem can happen also when using a temporary slot.
If the slot loses WAL file due to max_slot_wal_keep_size, the streamer
process could fail even while streaming WAL. I guess that the main
process taking a base backup won't stop in this case. If it's true,
pre-checking the existence of the slot might not be enough.
> It would be simpler, as you
> say, to find a way to detect if the slot wanted exists before even
> launching START_REPLICATION with the specified slot on the second
> thread copying the WAL, but that would be a new thing as one cannot do
> catalog lookups with a physical replication session.
Good point.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_basebackup behavior on non-existent slot
@ 2021-08-26 05:31 Ronan Dunklau <ronan.dunklau@aiven.io>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 12+ messages in thread
From: Ronan Dunklau @ 2021-08-26 05:31 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; pgsql-bugs@lists.postgresql.org; +Cc: Gerard H. Pille <ghpille@hotmail.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Michael Paquier <michael@paquier.xyz>
Le lundi 23 août 2021, 09:11:12 CEST Michael Paquier a écrit :
Some ideas here
> may be to extend the result of IDENTIFY_SYSTEM to return the slots
> available in a text array, invent a new command called SLOT, use SHOW
> or perhaps do an extra CREATE_REPLICATION_SLOT TEMPORARY from the main
> thread.
This new command (albeit named differently, READ_REPLICATION_SLOT) has been
proposed here: https://www.postgresql.org/message-id/
1642037.dBafQRxpgh@aivenronan for a different purpose, although it would be
useful to have that in that case too.
I guess I can add a patch to use that in pg_basebackup itself in the same
patchset.
--
Ronan Dunklau
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_basebackup behavior on non-existent slot
@ 2021-08-26 07:26 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Ronan Dunklau <ronan.dunklau@aiven.io>
0 siblings, 1 reply; 12+ messages in thread
From: Masahiko Sawada @ 2021-08-26 07:26 UTC (permalink / raw)
To: Ronan Dunklau <ronan.dunklau@aiven.io>; +Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Gerard H. Pille <ghpille@hotmail.com>; Michael Paquier <michael@paquier.xyz>
On Thu, Aug 26, 2021 at 2:31 PM Ronan Dunklau <ronan.dunklau@aiven.io> wrote:
>
> Le lundi 23 août 2021, 09:11:12 CEST Michael Paquier a écrit :
> Some ideas here
> > may be to extend the result of IDENTIFY_SYSTEM to return the slots
> > available in a text array, invent a new command called SLOT, use SHOW
> > or perhaps do an extra CREATE_REPLICATION_SLOT TEMPORARY from the main
> > thread.
>
> This new command (albeit named differently, READ_REPLICATION_SLOT) has been
> proposed here: https://www.postgresql.org/message-id/
> 1642037.dBafQRxpgh@aivenronan for a different purpose, although it would be
> useful to have that in that case too.
Cool! I was considering other use cases of those new command ideas
proposed on this thread.
> I guess I can add a patch to use that in pg_basebackup itself in the same
> patchset.
+1
I'll look at the patches.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_basebackup behavior on non-existent slot
@ 2021-08-27 00:20 Michael Paquier <michael@paquier.xyz>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 0 replies; 12+ messages in thread
From: Michael Paquier @ 2021-08-27 00:20 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Ronan Dunklau <ronan.dunklau@aiven.io>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Gerard H. Pille <ghpille@hotmail.com>
On Thu, Aug 26, 2021 at 04:26:41PM +0900, Masahiko Sawada wrote:
> Cool! I was considering other use cases of those new command ideas
> proposed on this thread.
So that makes two use cases for that. That sounds convincing.
> I'll look at the patches.
So will I. At quick glance, there are some issues in them. Let's
move the discussion there, then.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 12+ messages in thread
* pg_basebackup behavior on non-existent slot
@ 2021-09-28 09:50 Dimitri Fontaine <Dimitri.Fontaine@microsoft.com>
0 siblings, 1 reply; 12+ messages in thread
From: Dimitri Fontaine @ 2021-09-28 09:50 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Hi,
When developing pg_auto_failover we found a bug where if the target replication slot given to pg_basebackup does not exist, then a full copy of the source PGDATA is completed before erroring out. You could easily end-up copying 100GB of data over the network just to see pg_basebackup remove them all at the end, and then when using the –progress option, you have to scroll up to the very start of the output to see the error message.
Please find attached a patch that shows a way to fix the issue. The patch is missing windows compatibility, I don’t know how to cast the WNOHANG spell on this platform. Please use the patch as you see fit, either inspiration, or maybe something you would like to commit to fix the bug.
Here what it looks like without the patch:
$ ./src/bin/pg_basebackup/pg_basebackup -p 5501 -D /tmp/bb -X stream -S SlotDoesNotExists -P
pg_basebackup: error: could not send replication command "START_REPLICATION": ERROR: replication slot "SlotDoesNotExists" does not exist
32971/32971 kB (100%), 1/1 tablespace
pg_basebackup: error: child process exited with exit code 1
pg_basebackup: removing data directory "/tmp/bb"
Here’s what it looks like with the patch applied locally:
$ ./src/bin/pg_basebackup/pg_basebackup -p 5501 -D /tmp/bb -X stream -S SlotDoesNotExists -P
pg_basebackup: error: could not send replication command "START_REPLICATION": ERROR: replication slot "SlotDoesNotExists" does not exist
pg_basebackup: error: child process exited with exit code 1
pg_basebackup: removing data directory "/tmp/bb"
Regards,
--
Dimitri Fontaine
PostgreSQL Major Contributor, Citus Data, Microsoft
Author of “The Art of PostgreSQL<https://theartofpostgresql.com/>”
Attachments:
pg_basebackup_rep_slot.v4.patch (application/octet-stream, 6.8K) download
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 669aa207a3..4cf875de45 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -173,6 +173,7 @@ static int bgpipe[2] = {-1, -1};
/* Handle to child process */
static pid_t bgchild = -1;
+static bool bgchild_exited = false;
static bool in_log_streamer = false;
/* End position for xlog streaming, empty string if unknown yet */
@@ -211,6 +212,7 @@ static bool reached_end_position(XLogRecPtr segendpos, uint32 timeline,
static const char *get_tablespace_mapping(const char *dir);
static void tablespace_list_append(const char *arg);
+static void WaitForBGChild(const char *xlogend);
static void
@@ -967,6 +969,22 @@ ReceiveCopyData(PGconn *conn, WriteDataCallback callback,
int r;
char *copybuf;
+ if (bgchild > 0)
+ {
+ /* Check to see if our background process is still running fine. */
+ int status;
+ pid_t p;
+
+ p = waitpid(bgchild, &status, WNOHANG);
+
+ /* if our child process exited unexpectedly, we stop */
+ if (WIFEXITED(p))
+ {
+ bgchild_exited = true;
+ break;
+ }
+ }
+
r = PQgetCopyData(conn, ©buf, 0);
if (r == -1)
{
@@ -1167,6 +1185,11 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
ReceiveCopyData(conn, ReceiveTarCopyChunk, &state);
+ if (bgchild_exited)
+ {
+ return;
+ }
+
/*
* End of copy data. If requested, and this is the base tablespace, write
* configuration file into the tarfile. When done, close the file (but not
@@ -1531,6 +1554,11 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
if (state.file)
fclose(state.file);
+ if (bgchild_exited)
+ {
+ return;
+ }
+
progress_report(rownum, state.filename, true, false);
if (state.file != NULL)
@@ -1796,6 +1824,95 @@ ReceiveBackupManifestInMemoryChunk(size_t r, char *copybuf,
appendPQExpBuffer(buf, copybuf, r);
}
+static void
+WaitForBGChild(const char *xlogend)
+{
+ if (bgchild > 0)
+ {
+#ifndef WIN32
+ int status;
+ pid_t r;
+#else
+ DWORD status;
+
+ /*
+ * get a pointer sized version of bgchild to avoid warnings about
+ * casting to a different size on WIN64.
+ */
+ intptr_t bgchild_handle = bgchild;
+ uint32 hi,
+ lo;
+#endif
+
+ if (verbose)
+ pg_log_info("waiting for background process to finish streaming ...");
+
+#ifndef WIN32
+ if (write(bgpipe[1], xlogend, strlen(xlogend)) != strlen(xlogend))
+ {
+ pg_log_info("could not send command to background pipe: %m");
+ exit(1);
+ }
+
+ /* Just wait for the background process to exit */
+ r = waitpid(bgchild, &status, 0);
+ if (r == (pid_t) -1)
+ {
+ pg_log_error("could not wait for child process: %m");
+ exit(1);
+ }
+ if (r != bgchild)
+ {
+ pg_log_error("child %d died, expected %d", (int) r, (int) bgchild);
+ exit(1);
+ }
+ if (status != 0)
+ {
+ pg_log_error("%s", wait_result_to_str(status));
+ exit(1);
+ }
+ /* Exited normally, we're happy! */
+#else /* WIN32 */
+
+ /*
+ * On Windows, since we are in the same process, we can just store the
+ * value directly in the variable, and then set the flag that says
+ * it's there.
+ */
+ if (sscanf(xlogend, "%X/%X", &hi, &lo) != 2)
+ {
+ pg_log_error("could not parse write-ahead log location \"%s\"",
+ xlogend);
+ exit(1);
+ }
+ xlogendptr = ((uint64) hi) << 32 | lo;
+ InterlockedIncrement(&has_xlogendptr);
+
+ /* First wait for the thread to exit */
+ if (WaitForSingleObjectEx((HANDLE) bgchild_handle, INFINITE, FALSE) !=
+ WAIT_OBJECT_0)
+ {
+ _dosmaperr(GetLastError());
+ pg_log_error("could not wait for child thread: %m");
+ exit(1);
+ }
+ if (GetExitCodeThread((HANDLE) bgchild_handle, &status) == 0)
+ {
+ _dosmaperr(GetLastError());
+ pg_log_error("could not get child thread exit status: %m");
+ exit(1);
+ }
+ if (status != 0)
+ {
+ pg_log_error("child thread exited with error %u",
+ (unsigned int) status);
+ exit(1);
+ }
+ /* Exited normally, we're happy */
+#endif
+ }
+}
+
static void
BaseBackup(void)
{
@@ -2020,8 +2137,26 @@ BaseBackup(void)
ReceiveTarFile(conn, res, i);
else
ReceiveAndUnpackTarFile(conn, res, i);
+
+ if (bgchild_exited)
+ break;
} /* Loop over all tablespaces */
+
+ /* if bgchild exited early, we need to stop now. */
+ if (bgchild_exited)
+ {
+ PQclear(res);
+ PQfinish(conn);
+ conn = NULL;
+
+ destroyPQExpBuffer(recoveryconfcontents);
+
+ /* wait for the child process to be finished entirely */
+ (void) WaitForBGChild(xlogend);
+ exit(1);
+ }
+
/*
* Now receive backup manifest, if appropriate.
*
@@ -2079,90 +2214,7 @@ BaseBackup(void)
exit(1);
}
- if (bgchild > 0)
- {
-#ifndef WIN32
- int status;
- pid_t r;
-#else
- DWORD status;
-
- /*
- * get a pointer sized version of bgchild to avoid warnings about
- * casting to a different size on WIN64.
- */
- intptr_t bgchild_handle = bgchild;
- uint32 hi,
- lo;
-#endif
-
- if (verbose)
- pg_log_info("waiting for background process to finish streaming ...");
-
-#ifndef WIN32
- if (write(bgpipe[1], xlogend, strlen(xlogend)) != strlen(xlogend))
- {
- pg_log_info("could not send command to background pipe: %m");
- exit(1);
- }
-
- /* Just wait for the background process to exit */
- r = waitpid(bgchild, &status, 0);
- if (r == (pid_t) -1)
- {
- pg_log_error("could not wait for child process: %m");
- exit(1);
- }
- if (r != bgchild)
- {
- pg_log_error("child %d died, expected %d", (int) r, (int) bgchild);
- exit(1);
- }
- if (status != 0)
- {
- pg_log_error("%s", wait_result_to_str(status));
- exit(1);
- }
- /* Exited normally, we're happy! */
-#else /* WIN32 */
-
- /*
- * On Windows, since we are in the same process, we can just store the
- * value directly in the variable, and then set the flag that says
- * it's there.
- */
- if (sscanf(xlogend, "%X/%X", &hi, &lo) != 2)
- {
- pg_log_error("could not parse write-ahead log location \"%s\"",
- xlogend);
- exit(1);
- }
- xlogendptr = ((uint64) hi) << 32 | lo;
- InterlockedIncrement(&has_xlogendptr);
-
- /* First wait for the thread to exit */
- if (WaitForSingleObjectEx((HANDLE) bgchild_handle, INFINITE, FALSE) !=
- WAIT_OBJECT_0)
- {
- _dosmaperr(GetLastError());
- pg_log_error("could not wait for child thread: %m");
- exit(1);
- }
- if (GetExitCodeThread((HANDLE) bgchild_handle, &status) == 0)
- {
- _dosmaperr(GetLastError());
- pg_log_error("could not get child thread exit status: %m");
- exit(1);
- }
- if (status != 0)
- {
- pg_log_error("child thread exited with error %u",
- (unsigned int) status);
- exit(1);
- }
- /* Exited normally, we're happy */
-#endif
- }
+ (void) WaitForBGChild(xlogend);
/* Free the configuration file contents */
destroyPQExpBuffer(recoveryconfcontents);
* Re: pg_basebackup behavior on non-existent slot
@ 2021-09-28 10:25 Magnus Hagander <magnus@hagander.net>
parent: Dimitri Fontaine <Dimitri.Fontaine@microsoft.com>
0 siblings, 1 reply; 12+ messages in thread
From: Magnus Hagander @ 2021-09-28 10:25 UTC (permalink / raw)
To: Dimitri Fontaine <Dimitri.Fontaine@microsoft.com>; +Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; Daniel Gustafsson <daniel@yesql.se>
On Tue, Sep 28, 2021 at 12:03 PM Dimitri Fontaine
<Dimitri.Fontaine@microsoft.com> wrote:
>
> Hi,
>
>
>
> When developing pg_auto_failover we found a bug where if the target replication slot given to pg_basebackup does not exist, then a full copy of the source PGDATA is completed before erroring out. You could easily end-up copying 100GB of data over the network just to see pg_basebackup remove them all at the end, and then when using the –progress option, you have to scroll up to the very start of the output to see the error message.
>
>
>
> Please find attached a patch that shows a way to fix the issue. The patch is missing windows compatibility, I don’t know how to cast the WNOHANG spell on this platform. Please use the patch as you see fit, either inspiration, or maybe something you would like to commit to fix the bug.
>
>
>
> Here what it looks like without the patch:
>
>
>
> $ ./src/bin/pg_basebackup/pg_basebackup -p 5501 -D /tmp/bb -X stream -S SlotDoesNotExists -P
>
> pg_basebackup: error: could not send replication command "START_REPLICATION": ERROR: replication slot "SlotDoesNotExists" does not exist
>
> 32971/32971 kB (100%), 1/1 tablespace
>
> pg_basebackup: error: child process exited with exit code 1
>
> pg_basebackup: removing data directory "/tmp/bb"
>
>
>
>
>
> Here’s what it looks like with the patch applied locally:
>
>
>
> $ ./src/bin/pg_basebackup/pg_basebackup -p 5501 -D /tmp/bb -X stream -S SlotDoesNotExists -P
>
> pg_basebackup: error: could not send replication command "START_REPLICATION": ERROR: replication slot "SlotDoesNotExists" does not exist
>
> pg_basebackup: error: child process exited with exit code 1
>
> pg_basebackup: removing data directory "/tmp/bb"
Isn't this solving basically the same thing as
https://commitfest.postgresql.org/34/3302/ (which does have Windows
support)? Does this implementations have some advantages over the one
Daniel posted, or should we just focus on that one since it does have
Windows support in it?
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [EXTERNAL] Re: pg_basebackup behavior on non-existent slot
@ 2021-09-28 11:12 Dimitri Fontaine <Dimitri.Fontaine@microsoft.com>
parent: Magnus Hagander <magnus@hagander.net>
0 siblings, 1 reply; 12+ messages in thread
From: Dimitri Fontaine @ 2021-09-28 11:12 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; Daniel Gustafsson <daniel@yesql.se>
Thanks Magnus for linking the two approaches together, I missed Daniel’s effort entirely. I think it’s safe to focus on Daniel’s entry in the commit fest, and use this pgsql-bugs email as a reminder that it should very probably be backported as a bug-fix to all the maintained branches.
Regards,
--
Dimitri Fontaine
PostgreSQL Major Contributor, Citus Data, Microsoft
Author of “The Art of PostgreSQL<https://theartofpostgresql.com/>”
On 28/09/2021 12:25, "Magnus Hagander" <magnus@hagander.net> wrote:
On Tue, Sep 28, 2021 at 12:03 PM Dimitri Fontaine
<Dimitri.Fontaine@microsoft.com> wrote:
>
> Hi,
>
>
>
> When developing pg_auto_failover we found a bug where if the target replication slot given to pg_basebackup does not exist, then a full copy of the source PGDATA is completed before erroring out. You could easily end-up copying 100GB of data over the network just to see pg_basebackup remove them all at the end, and then when using the –progress option, you have to scroll up to the very start of the output to see the error message.
>
>
>
> Please find attached a patch that shows a way to fix the issue. The patch is missing windows compatibility, I don’t know how to cast the WNOHANG spell on this platform. Please use the patch as you see fit, either inspiration, or maybe something you would like to commit to fix the bug.
>
>
>
> Here what it looks like without the patch:
>
>
>
> $ ./src/bin/pg_basebackup/pg_basebackup -p 5501 -D /tmp/bb -X stream -S SlotDoesNotExists -P
>
> pg_basebackup: error: could not send replication command "START_REPLICATION": ERROR: replication slot "SlotDoesNotExists" does not exist
>
> 32971/32971 kB (100%), 1/1 tablespace
>
> pg_basebackup: error: child process exited with exit code 1
>
> pg_basebackup: removing data directory "/tmp/bb"
>
>
>
>
>
> Here’s what it looks like with the patch applied locally:
>
>
>
> $ ./src/bin/pg_basebackup/pg_basebackup -p 5501 -D /tmp/bb -X stream -S SlotDoesNotExists -P
>
> pg_basebackup: error: could not send replication command "START_REPLICATION": ERROR: replication slot "SlotDoesNotExists" does not exist
>
> pg_basebackup: error: child process exited with exit code 1
>
> pg_basebackup: removing data directory "/tmp/bb"
Isn't this solving basically the same thing as
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcommitfest.postgresql.org%2F34%2F3... (which does have Windows
support)? Does this implementations have some advantages over the one
Daniel posted, or should we just focus on that one since it does have
Windows support in it?
--
Magnus Hagander
Me: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.hagander.net%2F&data=0...
Work: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.redpill-linpro.com%2F&...
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [EXTERNAL] pg_basebackup behavior on non-existent slot
@ 2021-09-29 09:24 Daniel Gustafsson <daniel@yesql.se>
parent: Dimitri Fontaine <Dimitri.Fontaine@microsoft.com>
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Gustafsson @ 2021-09-29 09:24 UTC (permalink / raw)
To: Dimitri Fontaine <Dimitri.Fontaine@microsoft.com>; +Cc: Magnus Hagander <magnus@hagander.net>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
> On 28 Sep 2021, at 13:12, Dimitri Fontaine <Dimitri.Fontaine@microsoft.com> wrote:
> I think it’s safe to focus on Daniel’s entry in the commit fest,
For reference, here is the output from your testcase running with the
referenced patch:
$ ./bin/pg_basebackup -p 5432 -D /tmp/bb -X stream -S SlotDoesNotExists -P
pg_basebackup: error: could not send replication command "START_REPLICATION": ERROR: replication slot "SlotDoesNotExists" does not exist
pg_basebackup: error: background WAL receiver terminated unexpectedly
pg_basebackup: removing data directory "/tmp/bb"
So this patch does seem to cover that case as well.
> and use this pgsql-bugs email as a reminder that it should very probably be backported as a bug-fix to all the maintained branches.
It probably should, I can't see anyone relying on the current behavior. It's
however awfully intrusive for a backport as it effectively adds functionality
to solve what could be devils-advocate argued is an inconvenience and not a
bug. Our conservative stance on backports make this not a clear-cut case, but
I'm interested in what others think here.
--
Daniel Gustafsson https://vmware.com/
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2021-09-29 09:24 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-08-21 13:15 pg_basebackup behavior on non-existent slot Gerard H. Pille <ghpille@hotmail.com>
2021-08-23 03:54 ` Masahiko Sawada <sawada.mshk@gmail.com>
2021-08-23 07:11 ` Michael Paquier <michael@paquier.xyz>
2021-08-24 05:06 ` Masahiko Sawada <sawada.mshk@gmail.com>
2021-08-26 05:31 ` Ronan Dunklau <ronan.dunklau@aiven.io>
2021-08-26 07:26 ` Masahiko Sawada <sawada.mshk@gmail.com>
2021-08-27 00:20 ` Michael Paquier <michael@paquier.xyz>
2021-08-23 14:51 ` Gerard H. Pille <ghpille@hotmail.com>
2021-09-28 09:50 pg_basebackup behavior on non-existent slot Dimitri Fontaine <Dimitri.Fontaine@microsoft.com>
2021-09-28 10:25 ` Magnus Hagander <magnus@hagander.net>
2021-09-28 11:12 ` Dimitri Fontaine <Dimitri.Fontaine@microsoft.com>
2021-09-29 09:24 ` Daniel Gustafsson <daniel@yesql.se>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox