Subject: Re: Weird failure with latches in curculio on v15

From: Tom Lane <[email-masked]>
Date: Thu, 09 Feb 2023 10:51:29 -0500

Robert Haas <[email-masked]> writes:
> I think that we could certainly, as Michael suggests, have people
> provide their own background worker rather than having the archiver
> invoke the user-supplied code directly. As long as the functions that
> you need in order to get the necessary information can be called from
> some other process, that's fine. The only difficulty I see is that if
> the archiving is happening from a separate background worker rather
> than from the archiver, then what is the archiver doing? We could
> somehow arrange to not run the archiver process at all, or I guess to
> just sit there and have it do nothing. Or, we can decide not to have a
> separate background worker and just have the archiver call the
> user-supplied core directly. I kind of like that approach at the
> moment; it seems more elegant to me.

I'm fairly concerned about the idea of making it common for people
to write their own main loop for the archiver.  That means that, if
we have a bug fix that requires the archiver to do X, we will not
just be patching our own code but trying to get an indeterminate
set of third parties to add the fix to their code.

If we think we need primitives to let the archiver hooks get all
the pending files, or whatever, by all means add those.  But don't
cede fundamental control of the archiver.  The hooks need to be
decoration on a framework we provide, not the framework themselves.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Thu, 9 Feb 2023 11:12:21 -0500

> set of third parties to add the fix to their code.

I don't know what kind of bug we could really have in the main loop
that would be common to every implementation. They're probably all
going to check for interrupts, do some work, and then wait for I/O on
some things by calling select() or some equivalent. But the work, and
the wait for the I/O, would be different for every implementation. I
would anticipate that the amount of common code would be nearly zero.

Imagine two archive modules, one of which archives files via HTTP and
the other of which archives them via SSH. They need to do a lot of the
same things, but the code is going to be totally different. When the
HTTP archiver module needs to open a new connection, it's going to
call some libcurl function. When the SSH archiver module needs to do
the same thing, it's going to call some libssh function. It seems
quite likely that the HTTP implementation would want to juggle
multiple connections in parallel, but the SSH implementation might not
want to do that, or its logic for determining how many connections to
open might be completely different based on the behavior of that
protocol vs. the other protocol. Once either implementation has sent
as much data it can over the connections it has open, it needs to wait
for those sockets to become write-ready or, possibly, read-ready.
There again, each one will be calling into a different library to do
that. It could be that in this particular case, but would be waiting
for a set of file descriptors, and we could provide some framework for
waiting on a set of file descriptors provided by the module. But you
could also have some other archiver implementation that is, say,
waiting for a process to terminate rather than for a file descriptor
to become ready for I/O.

> decoration on a framework we provide, not the framework themselves.

I don't quite see how you can make asynchronous and parallel archiving
work if the archiver process only calls into the archive module at
times that it chooses. That would mean that the module has to return
control to the archiver when it's in the middle of archiving one or
more files -- and then I don't see how it can get control back at the
appropriate time. Do you have a thought about that?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 9 Feb 2023 09:23:08 -0800

> appropriate time. Do you have a thought about that?

I've been thinking about this, actually.  I'm wondering if we could provide
a list of files to the archiving callback (configurable via a variable in
ArchiveModuleState), and then have the callback return a list of files that
are archived.  (Or maybe we just put the list of files that need archiving
in ArchiveModuleState.)  The returned list could include files that were
sent to the callback previously.  The archive module would be responsible
for creating background worker(s) (if desired), dispatching files
to-be-archived to its background worker(s), and gathering the list of
archived files to return.

This is admittedly half-formed, but I'm tempted to hack something together
quickly to see whether it might be viable.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Andres Freund <[email-masked]>
Date: Thu, 9 Feb 2023 11:29:52 -0800

Hi,

> > set of third parties to add the fix to their code.

I'm somewhat concerned about that too, but perhaps from a different
angle. First, I think we don't do our users a service by defaulting the
in-core implementation to something that doesn't scale to even a moderately
busy server. Second, I doubt we'll get the API for any of this right, without
an acutual user that does something more complicated than restoring one-by-one
in a blocking manner.

> would anticipate that the amount of common code would be nearly zero.

I don't think it's that hard to imagine problems. To be reasonably fast, a
decent restore implementation will have to 'restore ahead'. Which also
provides ample things to go wrong. E.g.

- WAL source is switched, restore module needs to react to that, but doesn't,
  we end up lots of wasted work, or worse, filename conflicts
- recovery follows a timeline, restore module doesn't catch on quickly enough
- end of recovery happens, restore just continues on

> appropriate time. Do you have a thought about that?

I don't think archiver is the hard part, that already has a dedicated
process, and it also has something of a queuing system already. The startup
process imo is the complicated one...

If we had a 'restorer' process, startup fed some sort of a queue with things
to restore in the near future, it might be more realistic to do something you
describe?

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 14 Feb 2023 09:47:55 -0800

Here is a new version of the stopgap/back-branch fix for restore_command.
This is more or less a rebased version of v4 with an added stderr message
as Andres suggested upthread.

>From 20edca59834c7755bfddb070fb9db3f59dc6ff96 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Tue, 14 Feb 2023 09:44:53 -0800
Subject: [PATCH v5 1/1] stopgap fix for restore_command

---
 src/backend/access/transam/xlogarchive.c | 15 +++++++++++----
 src/backend/postmaster/startup.c         | 20 +++++++++++++++++++-
 src/backend/storage/ipc/ipc.c            |  3 +++
 src/backend/storage/lmgr/proc.c          |  2 ++
 4 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index fcc87ff44f..41684418b6 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -159,20 +159,27 @@ RestoreArchivedFile(char *path, const char *xlogfname,
 			(errmsg_internal("executing restore command \"%s\"",
 							 xlogRestoreCmd)));
 
+	fflush(NULL);
+	pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
+
 	/*
-	 * Check signals before restore command and reset afterwards.
+	 * PreRestoreCommand() informs the SIGTERM handler for the startup process
+	 * that it should proc_exit() right away.  This is done for the duration of
+	 * the system() call because there isn't a good way to break out while it
+	 * is executing.  Since we might call proc_exit() in a signal handler, it
+	 * is best to put any additional logic before or after the
+	 * PreRestoreCommand()/PostRestoreCommand() section.
 	 */
 	PreRestoreCommand();
 
 	/*
 	 * Copy xlog from archival storage to XLOGDIR
 	 */
-	fflush(NULL);
-	pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
 	rc = system(xlogRestoreCmd);
-	pgstat_report_wait_end();
 
 	PostRestoreCommand();
+
+	pgstat_report_wait_end();
 	pfree(xlogRestoreCmd);
 
 	if (rc == 0)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..de2b56c2fa 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
  */
 #include "postgres.h"
 
+#include <unistd.h>
+
 #include "access/xlog.h"
 #include "access/xlogrecovery.h"
 #include "access/xlogutils.h"
@@ -121,7 +123,23 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
 	int			save_errno = errno;
 
 	if (in_restore_command)
-		proc_exit(1);
+	{
+		/*
+		 * If we are in a child process (e.g., forked by system() in
+		 * RestoreArchivedFile()), we don't want to call any exit callbacks.
+		 * The parent will take care of that.
+		 */
+		if (MyProcPid == (int) getpid())
+			proc_exit(1);
+		else
+		{
+			const char	msg[] = "StartupProcShutdownHandler() called in child process";
+			int			rc pg_attribute_unused();
+
+			rc = write(STDERR_FILENO, msg, sizeof(msg));
+			_exit(1);
+		}
+	}
 	else
 		shutdown_requested = true;
 	WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..6796cabc3e 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int	on_proc_exit_index,
 void
 proc_exit(int code)
 {
+	/* proc_exit() is not safe in forked processes from system(), etc. */
+	Assert(MyProcPid == getpid());
+
 	/* Clean up everything that must be cleaned up */
 	proc_exit_prepare(code);
 
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..ae845e8249 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
 	dlist_head *procgloballist;
 
 	Assert(MyProc != NULL);
+	Assert(MyProcPid == getpid());  /* not safe if forked by system(), etc. */
 
 	/* Make sure we're out of the sync rep lists */
 	SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
 	PGPROC	   *proc;
 
 	Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+	Assert(MyProcPid == getpid());	/* not safe if forked by system(), etc. */
 
 	auxproc = &AuxiliaryProcs[proctype];
 
-- 
2.25.1

---

From: Robert Haas <[email-masked]>
Date: Thu, 16 Feb 2023 15:08:14 +0530

> archived files to return.

Hmm. So in this design, the archiver doesn't really do the archiving
any more, because the interface makes that impossible. It has to use a
separate background worker process for that, full stop.

I don't think that's a good design. It's fine if some people want to
implement it that way, but it shouldn't be forced by the interface.

---

From: Robert Haas <[email-masked]>
Date: Thu, 16 Feb 2023 15:18:57 +0530

On Fri, Feb 10, 2023 at 12:59 AM Andres Freund <[email-masked]> wrote:
> busy server.

+1.

> Second, I doubt we'll get the API for any of this right, without

Fair.

> - end of recovery happens, restore just continues on

I don't see how you can prevent those things from happening. If the
restore process is working in some way that requires an event loop,
and I think that will be typical for any kind of remote archiving,
then either it has control most of the time, so the event loop can be
run inside the restore process, or, as Nathan proposes, we don't let
the archiver have control and it needs to run that restore process in
a separate background worker. The hazards that you mention here exist
either way. If the event loop is running inside the restore process,
it can decide not to call the functions that we provide in a timely
fashion and thus fail to react as it should. If the event loop runs
inside a separate background worker, then that process can fail to be
responsive in precisely the same way. Fundamentally, if the author of
a restore module writes code to have multiple I/Os in flight at the
same time and does not write code to cancel those I/Os if something
changes, then such cancellation will not occur. That remains true no
matter which process is performing the I/O.

> to restore in the near future, it might be more realistic to do something you

Some kind of queueing system might be a useful part of the interface,
and a dedicated restorer process does sound like a good idea. But the
archiver doesn't have this solved, precisely because you have to
archive a single file, return control, and wait to be invoked again
for the next file. That does not scale.

---

From: Andres Freund <[email-masked]>
Date: Thu, 16 Feb 2023 08:32:54 -0800

Hi,

> matter which process is performing the I/O.

IDK. I think we can make that easier or harder. Right now the proposed API
doesn't provide anything to allow to address this.

> for the next file. That does not scale.

But there's nothing inherent in that. We know for certain which files we're
going to archive. And we don't need to work one-by-one. The archiver could
just start multiple subprocesses at the same time. All the blocking it does
right now are artificially imposed by the use of system(). We could instead
just use something popen() like and have a configurable number of processes
running at the same time.

What I was trying to point out was that the work a "restorer" process has to
do is more speculative, because we don't know when we'll promote, whether
we'll follow a timeline increase, whether the to-be-restored WAL already
exists. That's solvable, but a bunch of the relevant work ought to be solved
in core core code, instead of just in archive modules.

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 16 Feb 2023 08:57:59 -0800

> implement it that way, but it shouldn't be forced by the interface.

I don't think it would force you to use a background worker, but if you
wanted to, the tools would be available.  At least, that is the intent.

---

From: Robert Haas <[email-masked]>
Date: Sat, 18 Feb 2023 14:19:53 +0530

> wanted to, the tools would be available.  At least, that is the intent.

I'm 100% amenable to somebody demonstrating how that is super easy,
barely an inconvenience. But I think we would need to see some code
showing at least what the API is going to look like, and ideally a
sample implementation, in order for me to be convinced of that. What I
suspect is that if somebody tries to do that they are going to find
that the core API has to be quite opinionated about how the archive
module has to do things, which I think is not what we want. But if
that turns out to be false, cool!

---

From: Robert Haas <[email-masked]>
Date: Sat, 18 Feb 2023 15:51:06 +0530

On Thu, Feb 16, 2023 at 10:02 PM Andres Freund <[email-masked]> wrote:
> just start multiple subprocesses at the same time.

But what if it doesn't want to start multiple processes, just
multiplex within a single process?

> in core core code, instead of just in archive modules.

Yep, I can see that there are some things to figure out there, and I
agree that they should be figured out in the core code.

---

From: Andres Freund <[email-masked]>
Date: Sat, 18 Feb 2023 13:15:22 -0800

Hi,

> multiplex within a single process?

To me that seems even simpler? Nothing but the archiver is supposed to create
.done files and nothing is supposed to remove .ready files without archiver
having created the .done files.  So the archiver process can scan
archive_status until its done or until N archives have been collected, and
then process them at once?  Only the creation of the .done files would be
serial, but I don't think that's commonly a problem (and could be optimized as
well, by creating multiple files and then fsyncing them in a second pass,
avoiding N filesystem journal flushes).

Maybe I am misunderstanding what you see as the problem?

Greetings,

Andres Freund

---

From: Robert Haas <[email-masked]>
Date: Sun, 19 Feb 2023 20:06:24 +0530

> Maybe I am misunderstanding what you see as the problem?

Well right now the archiver process calls ArchiveFileCB when there's a
file ready for archiving, and that process is supposed to archive the
whole thing before returning. That pretty obviously seems to preclude
having more than one file being archived at the same time. What
callback structure do you have in mind to allow for that?

I mean, my idea was to basically just have one big callback:
ArchiverModuleMainLoopCB(). Which wouldn't return, or perhaps, would
only return when archiving was totally caught up and there was nothing
more to do right now. And then that callback could call functions like
AreThereAnyMoreFilesIShouldBeArchivingAndIfYesWhatIsTheNextOne(). So
it would call that function and it would find out about a file and
start an HTTP session or whatever and then call that function again
and start another HTTP session for the second file and so on until it
had as much concurrency as it wanted. And then when it hit the
concurrency limit, it would wait until at least one HTTP request
finished. At that point it would call
HeyEverybodyISuccessfullyArchivedAWalFile(), after which it could
again ask for the next file and start a request for that one and so on
and so forth.

I don't really understand what the other possible model is here,
honestly. Right now, control remains within the archive module for the
entire time that a file is being archived. If we generalize the model
to allow multiple files to be in the process of being archived at the
same time, the archive module is going to need to have control as long
as >= 1 of them are in progress, at least AFAICS. If you have some
other idea how it would work, please explain it to me...

---

From: Michael Paquier <[email-masked]>
Date: Tue, 21 Feb 2023 07:38:17 +0900

> again ask for the next file and start a request for that one and so on

This archiving implementation is not completely impossible with the
current API infrastructure, either?  If you consider the archiving as
a two-step process where segments are first copied into a cheap,
reliable area.  Then these could be pushed in block in a more remote
area like a S3 bucket?  Of course this depends on other things like
the cluster structure, but redundancy can be added with standby
archiving, as well.

I am not sure exactly how many requirements we want to push into a
callback, to be honest, and surely more requirements pushed to the
callback increases the odds of implementation mistakes, like a full
loop.  There already many ways to get it wrong with archiving, like
missing a flush of the archived segment before the callback returns to
ensure its durability..
--
Michael

---

From: Stephen Frost <[email-masked]>
Date: Mon, 20 Feb 2023 18:17:21 -0500

Greetings,

> the cluster structure, but redundancy can be added with standby

Surely it can't be too cheap as it needs to be reliable..  We have
looked at this before (copying to a queue area before copying with a
separate process off-system) and it simply isn't great and requires more
work than you really want to do if you can help it and for no real
benefit.

> missing a flush of the archived segment before the callback returns to

Without any actual user of any of this it's surprising to me how much
effort has been put into it.  Have I missed the part where someone has
said they're actually implementing an archive library that we can look
at and see how it works and how the archive library and the core system
could work better together..?

We (pgbackrest) are generally interested in the idea to reduce the
startup time, but that's not really a big issue for us currently and so
it hasn't really risen up to the level of being something we're working
on, not to mention that if it keeps changing each release then it's just
going to end up being more work for us for a feature that doesn't gain
us all that much.

Now, all that said, at least in initial discussions, we expect the
pgbackrest archive_library to look very similar to how we handle
archive_command and async archiving today- when called if there's
multiple WAL files to process then we fork an async process off and it
goes and spawns multiple processes and does its work to move the WAL
files to the off-system storage and when we are called via
archive_command we just check a status flag to see if that WAL has been
archived yet by the async process or not.  If not and there's no async
process running then we'll start a new one (starting a new async process
periodically actually makes things a lot easier to test for us too,
which is why we don't just have an async process running around forever-
the startup time typically isn't that big of a deal), if there is a
status flag then we return whatever it says, and if the async process is
running and no status flag yet then we wait.

Once we have that going then perhaps there could be some interesting
iteration between pgbackrest and the core code to improve things, but
all this discussion and churn feels more likely to put folks off of
trying to implement something using this approach than the opposite,
unless someone in this discussion is actually working on an archive
library, but that isn't the impression I've gotten, at least (though if
there is such a work in progress out there, I'd love to see it!).

Thanks,

Stephen

---

From: Michael Paquier <[email-masked]>
Date: Tue, 21 Feb 2023 09:03:27 +0900

> as Andres suggested upthread.

So, this thread has moved around many subjects, still we did not get
to the core of the issue which is what we should try to do to avoid
sporadic failures like what the top of the thread is mentioning.

Perhaps beginning a new thread with a patch and a summary would be
better at this stage?  Another thing I am wondering is if it could be
possible to test that rather reliably.  I have been playing with a few
scenarios like holding the system() call for a bit with hardcoded
sleep()s, without much success.  I'll try harder on that part..  It's
been mentioned as well that we could just move away from system() in
the long-term.
--
Michael

---

From: Thomas Munro <[email-masked]>
Date: Tue, 21 Feb 2023 13:32:10 +1300

> been mentioned as well that we could just move away from system() in

I've been experimenting with ideas for master, which I'll start a new
thread about.  Actually I was already thinking about this before this
broken signal handler stuff came up, because it was already
unacceptable that all these places that are connected to shared memory
ignore interrupts for unbounded time while a shell script/whatever
runs.  At first I thought it would be relatively simple to replace
system() with something that has a latch wait loop (though there are
details to argue about, like whether you want to allow interrupts that
throw, and if so, how you clean up the subprocess, which have several
plausible solutions).  But once I started looking at the related
popen-based stuff where you want to communicate with the subprocess
(for example COPY FROM PROGRAM), I realised that it needs more
analysis and work: that stuff is currently entirely based on stdio
FILE (that is, fread() and fwrite()), but it's not really possible (at
least portably) to make that nonblocking, and in fact it's a pretty
terrible interface in terms of error reporting in general.  I've been
sketching/thinking about a new module called 'subprocess', with a
couple of ways to start processes, and interact with them via
WaitEventSet and direct pipe I/O; or if buffering is needed, it'd be
our own, not <stdio.h>'s.  But don't let me stop anyone else proposing
ideas.

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 20 Feb 2023 20:50:39 -0800

> been mentioned as well that we could just move away from system() in

I'm happy to create a new thread if needed, but I can't tell if there is
any interest in this stopgap/back-branch fix.  Perhaps we should just jump
straight to the long-term fix that Thomas is looking into.

---

From: Thomas Munro <[email-masked]>
Date: Wed, 22 Feb 2023 21:48:10 +1300

> straight to the long-term fix that Thomas is looking into.

Unfortunately the latch-friendly subprocess module proposal I was
talking about would be for 17.  I may post a thread fairly soon with
design ideas + list of problems and decision points as I see them, and
hopefully some sketch code, but it won't be a proposal for [/me checks
calendar] next week's commitfest and probably wouldn't be appropriate
in a final commitfest anyway, and I also have some other existing
stuff to clear first.  So please do continue with the stopgap ideas.

BTW Here's an idea (untested) about how to reproduce the problem.  You
could copy the source from a system() implementation, call it
doomed_system(), and insert kill(-getppid(), SIGQUIT) in between
sigprocmask(SIG_SETMASK, &omask, NULL) and exec*().  Parent and self
will handle the signal and both reach the proc_exit().

The systems that failed are running code like this:

https://github.com/openbsd/src/blob/master/lib/libc/stdlib/system.c
https://github.com/DragonFlyBSD/DragonFlyBSD/blob/master/lib/libc/stdlib/system.c

I'm pretty sure these other implementations could fail in just the
same way (they restore the handler before unblocking, so can run it
just before exec() replaces the image):

https://github.com/freebsd/freebsd-src/blob/main/lib/libc/stdlib/system.c
https://github.com/lattera/glibc/blob/master/sysdeps/posix/system.c

The glibc one is a bit busier and, huh, has a lock (I guess maybe
deadlockable if proc_exit() also calls system(), but hopefully it
doesn't), and uses fork() instead of vfork() but I don't think that's
a material difference here (with fork(), parent and child run
concurrently, while with vfork() the parent is suspended until the
child exists or execs, and then processes its pending signals).

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 23 Feb 2023 15:16:50 -0800

> stuff to clear first.  So please do continue with the stopgap ideas.

I've created a new thread for the stopgap fix [0].

[0] https://postgr.es/m/20230223231503.GA743455%40nathanxps13

---

From: Andres Freund <[email-masked]>
Date: Sat, 25 Feb 2023 11:00:31 -0800

Hi,

> callback structure do you have in mind to allow for that?

TBH, I think the current archive and restore module APIs aren't useful. I
think it was a mistake to add archive modules without having demonstrated that
one can do something useful with them that the restore_command didn't already
do. If anything, archive modules have made it harder to improve archiving
performance via concurrency.

My point was that it's easy to have multiple archive commands in process at
the same time, because we already have a queuing system, and that
archive_command is entire compatible with doing that, because running multiple
subprocesses is pretty trivial. It wasn't that the archive API is suitable for
that.

> other idea how it would work, please explain it to me...

I don't think that a main loop approach is the only viable one. It might be
the most likely to succeed one though. As an alternative, consider something
like

struct ArchiveFileState {
   int fd;
   enum WaitFor { READ, WRITE, CONNECT };
   void *file_private;
}

typedef bool (*ArchiveFileStartCB)(ArchiveModuleState *state,
   ArchiveFileState *file_state,
   const char *file, const char *path);

typedef bool (*ArchiveFileContinueCB)(ArchiveModuleState *state,
   ArchiveFileState *file_state);

An archive module could open an HTTP connection, do IO until it's blocked, put
the fd in file_state, return. The main loop could do big event loop around all
of the file descriptors and whenever any of FDs signal IO is ready, call
ArchiveFileContinueCB() for that file.

I don't know if that's better than ArchiverModuleMainLoopCB(). I can see both
advantages and disadvantages.

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 28 Feb 2023 20:29:19 -0800

> performance via concurrency.

I must respectfully disagree that this work is useless.  Besides the
performance and security benefits of not shelling out for every WAL file,
I've found it very useful to be able to use the standard module framework
to develop archive modules.  It's relatively easy to make use of GUCs,
background workers, compression, etc.  Of course, there is room for
improvement in areas like concurrency support as you rightly point out, but
I don't think that makes the current state worthless.

---

From: Stephen Frost <[email-masked]>
Date: Wed, 1 Mar 2023 10:00:49 -0500

Greetings,

> I don't think that makes the current state worthless.

Would be great to see these archive modules, perhaps it would help show
how this functionality is useful and what could be done in core to make
things easier for the archive module.

Thanks,

Stephen

---

From: Robert Haas <[email-masked]>
Date: Wed, 1 Mar 2023 12:44:51 -0500

> I don't think that makes the current state worthless.

I also disagree with Andres. The status quo ante was that we did not
provide any way of doing archiving correctly even to a directory on
the local machine. We could only recommend silly things like 'cp' that
are incorrect in multiple ways. basic_archive isn't the most wonderful
thing ever, and its deficiencies are more obvious to me now than they
were when I committed it. But it's better than recommending a shell
command that doesn't even work.