public inbox for [email protected]help / color / mirror / Atom feed
Re: Weird failure with latches in curculio on v15 25+ messages / 8 participants [nested] [flat]
* Re: Weird failure with latches in curculio on v15 @ 2023-02-09 15:51 Tom Lane <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Tom Lane @ 2023-02-09 15:51 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> Robert Haas <[email protected]> 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 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-09 16:12 Robert Haas <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 2 replies; 25+ messages in thread From: Robert Haas @ 2023-02-09 16:12 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Thu, Feb 9, 2023 at 10:51 AM Tom Lane <[email protected]> wrote: > 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. 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. > 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. 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 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-09 17:23 Nathan Bossart <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Nathan Bossart @ 2023-02-09 17:23 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Thu, Feb 09, 2023 at 11:12:21AM -0500, Robert Haas wrote: > On Thu, Feb 9, 2023 at 10:51 AM Tom Lane <[email protected]> wrote: >> 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. > > 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? 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 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-09 19:29 Andres Freund <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 2 replies; 25+ messages in thread From: Andres Freund @ 2023-02-09 19:29 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> Hi, On 2023-02-09 11:12:21 -0500, Robert Haas wrote: > On Thu, Feb 9, 2023 at 10:51 AM Tom Lane <[email protected]> wrote: > > 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. 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. > 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. 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 > > 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. > > 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? 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 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-14 17:47 Nathan Bossart <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Nathan Bossart @ 2023-02-14 17:47 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> 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. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v5-0001-stopgap-fix-for-restore_command.patch (3.9K, ../../20230214174755.GA1348509@nathanxps13/2-v5-0001-stopgap-fix-for-restore_command.patch) download | inline diff: From 20edca59834c7755bfddb070fb9db3f59dc6ff96 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> 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 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-16 09:38 Robert Haas <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Robert Haas @ 2023-02-16 09:38 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Tom Lane <[email protected]>; Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Thu, Feb 9, 2023 at 10:53 PM Nathan Bossart <[email protected]> wrote: > 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. 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. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-16 09:48 Robert Haas <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Robert Haas @ 2023-02-16 09:48 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Fri, Feb 10, 2023 at 12:59 AM Andres Freund <[email protected]> wrote: > 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. +1. > 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. Fair. > 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 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. > > 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? > > 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? 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. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-16 16:32 Andres Freund <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Andres Freund @ 2023-02-16 16:32 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> Hi, On 2023-02-16 15:18:57 +0530, Robert Haas wrote: > On Fri, Feb 10, 2023 at 12:59 AM Andres Freund <[email protected]> wrote: > > 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 > > 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. IDK. I think we can make that easier or harder. Right now the proposed API doesn't provide anything to allow to address this. > > > 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? > > > > 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? > > 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. 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 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-16 16:57 Nathan Bossart <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Nathan Bossart @ 2023-02-16 16:57 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Thu, Feb 16, 2023 at 03:08:14PM +0530, Robert Haas wrote: > On Thu, Feb 9, 2023 at 10:53 PM Nathan Bossart <[email protected]> wrote: >> 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. > > 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. 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. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-18 08:49 Robert Haas <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Robert Haas @ 2023-02-18 08:49 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Tom Lane <[email protected]>; Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Thu, Feb 16, 2023 at 10:28 PM Nathan Bossart <[email protected]> wrote: > > 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. > > 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. 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! -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-18 10:21 Robert Haas <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Robert Haas @ 2023-02-18 10:21 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Thu, Feb 16, 2023 at 10:02 PM Andres Freund <[email protected]> wrote: > 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. But what if it doesn't want to start multiple processes, just multiplex within a single process? > 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. 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. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-18 21:15 Andres Freund <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Andres Freund @ 2023-02-18 21:15 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> Hi, On 2023-02-18 15:51:06 +0530, Robert Haas wrote: > On Thu, Feb 16, 2023 at 10:02 PM Andres Freund <[email protected]> wrote: > > 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. > > But what if it doesn't want to start multiple processes, just > 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 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-19 14:36 Robert Haas <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 2 replies; 25+ messages in thread From: Robert Haas @ 2023-02-19 14:36 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Sun, Feb 19, 2023 at 2:45 AM Andres Freund <[email protected]> wrote: > 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? 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... -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15[ @ 2023-02-20 22:38 Michael Paquier <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Michael Paquier @ 2023-02-20 22:38 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Sun, Feb 19, 2023 at 08:06:24PM +0530, Robert Haas wrote: > 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. 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 Attachments: [application/pgp-signature] signature.asc (833B, ../../Y%2FP2WfDOMVw59%[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15[ @ 2023-02-20 23:17 Stephen Frost <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Stephen Frost @ 2023-02-20 23:17 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> Greetings, * Michael Paquier ([email protected]) wrote: > On Sun, Feb 19, 2023 at 08:06:24PM +0530, Robert Haas wrote: > > 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. > > 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. 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. > 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.. 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 Attachments: [application/pgp-signature] signature.asc (833B, ../../Y%2FP%[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-21 00:03 Michael Paquier <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 2 replies; 25+ messages in thread From: Michael Paquier @ 2023-02-21 00:03 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Tue, Feb 14, 2023 at 09:47:55AM -0800, Nathan Bossart wrote: > 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. 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 Attachments: [application/pgp-signature] signature.asc (833B, ../../Y%2FQKT%2FvgQajgO%[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-21 00:32 Thomas Munro <[email protected]> parent: Michael Paquier <[email protected]> 1 sibling, 0 replies; 25+ messages in thread From: Thomas Munro @ 2023-02-21 00:32 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Tue, Feb 21, 2023 at 1:03 PM Michael Paquier <[email protected]> wrote: > 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. 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. ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-21 04:50 Nathan Bossart <[email protected]> parent: Michael Paquier <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Nathan Bossart @ 2023-02-21 04:50 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Tue, Feb 21, 2023 at 09:03:27AM +0900, Michael Paquier wrote: > On Tue, Feb 14, 2023 at 09:47:55AM -0800, Nathan Bossart wrote: >> 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. > > 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. 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. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-22 08:48 Thomas Munro <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Thomas Munro @ 2023-02-22 08:48 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Tue, Feb 21, 2023 at 5:50 PM Nathan Bossart <[email protected]> wrote: > On Tue, Feb 21, 2023 at 09:03:27AM +0900, Michael Paquier wrote: > > 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. > > 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. 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). ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-23 23:16 Nathan Bossart <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Nathan Bossart @ 2023-02-23 23:16 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Wed, Feb 22, 2023 at 09:48:10PM +1300, Thomas Munro wrote: > On Tue, Feb 21, 2023 at 5:50 PM Nathan Bossart <[email protected]> wrote: >> On Tue, Feb 21, 2023 at 09:03:27AM +0900, Michael Paquier wrote: >> > 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. >> >> 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. > > 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. I've created a new thread for the stopgap fix [0]. [0] https://postgr.es/m/20230223231503.GA743455%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-02-25 19:00 Andres Freund <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Andres Freund @ 2023-02-25 19:00 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> Hi, On 2023-02-19 20:06:24 +0530, Robert Haas wrote: > On Sun, Feb 19, 2023 at 2:45 AM Andres Freund <[email protected]> wrote: > > 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? > > 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? 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. > 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... 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 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-03-01 04:29 Nathan Bossart <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 2 replies; 25+ messages in thread From: Nathan Bossart @ 2023-03-01 04:29 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Sat, Feb 25, 2023 at 11:00:31AM -0800, Andres Freund wrote: > 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. 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. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-03-01 15:00 Stephen Frost <[email protected]> parent: Nathan Bossart <[email protected]> 1 sibling, 0 replies; 25+ messages in thread From: Stephen Frost @ 2023-03-01 15:00 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> Greetings, * Nathan Bossart ([email protected]) wrote: > On Sat, Feb 25, 2023 at 11:00:31AM -0800, Andres Freund wrote: > > 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. > > 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. 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 Attachments: [application/pgp-signature] signature.asc (833B, ../../Y%[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Weird failure with latches in curculio on v15 @ 2023-03-01 17:44 Robert Haas <[email protected]> parent: Nathan Bossart <[email protected]> 1 sibling, 0 replies; 25+ messages in thread From: Robert Haas @ 2023-03-01 17:44 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Fujii Masao <[email protected]>; Postgres hackers <[email protected]> On Tue, Feb 28, 2023 at 11:29 PM Nathan Bossart <[email protected]> wrote: > On Sat, Feb 25, 2023 at 11:00:31AM -0800, Andres Freund wrote: > > 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. > > 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. 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. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 25+ messages in thread
* [PATCH v7 06/13] table_scan_bitmap_next_block() returns lossy or exact @ 2024-02-27 01:34 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw) Future commits will remove the TBMIterateResult from BitmapHeapNext() -- pushing it into the table AM-specific code. So, the table AM must inform BitmapHeapNext() whether or not the current block is lossy or exact for the purposes of the counters used in EXPLAIN. --- src/backend/access/heap/heapam_handler.c | 5 ++++- src/backend/executor/nodeBitmapHeapscan.c | 10 +++++----- src/include/access/tableam.h | 14 ++++++++++---- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index c9b9b4c00f1..10c1c3b616b 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2112,7 +2112,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths, static bool heapam_scan_bitmap_next_block(TableScanDesc scan, - TBMIterateResult *tbmres) + TBMIterateResult *tbmres, + bool *lossy) { HeapScanDesc hscan = (HeapScanDesc) scan; BlockNumber block = tbmres->blockno; @@ -2240,6 +2241,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan, Assert(ntup <= MaxHeapTuplesPerPage); hscan->rs_ntuples = ntup; + *lossy = tbmres->ntuples < 0; + return ntup > 0; } diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index a6b98fa12a1..b2397fe2054 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node) for (;;) { - bool valid; + bool valid, lossy; CHECK_FOR_INTERRUPTS(); @@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node) BitmapAdjustPrefetchIterator(node, tbmres->blockno); - valid = table_scan_bitmap_next_block(scan, tbmres); + valid = table_scan_bitmap_next_block(scan, tbmres, &lossy); - if (tbmres->ntuples >= 0) - node->exact_pages++; - else + if (lossy) node->lossy_pages++; + else + node->exact_pages++; if (!valid) { diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index c43a8b3dea5..f1d0d4b78e3 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -796,6 +796,9 @@ typedef struct TableAmRoutine * on the page have to be returned, otherwise the tuples at offsets in * `tbmres->offsets` need to be returned. * + * lossy indicates whether or not the block's representation in the bitmap + * is lossy or exact. + * * XXX: Currently this may only be implemented if the AM uses md.c as its * storage manager, and uses ItemPointer->ip_blkid in a manner that maps * blockids directly to the underlying storage. nodeBitmapHeapscan.c @@ -811,7 +814,8 @@ typedef struct TableAmRoutine * scan_bitmap_next_tuple need to exist, or neither. */ bool (*scan_bitmap_next_block) (TableScanDesc scan, - struct TBMIterateResult *tbmres); + struct TBMIterateResult *tbmres, + bool *lossy); /* * Fetch the next tuple of a bitmap table scan into `slot` and return true @@ -1951,14 +1955,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths, * Prepare to fetch / check / return tuples from `tbmres->blockno` as part of * a bitmap table scan. `scan` needs to have been started via * table_beginscan_bm(). Returns false if there are no tuples to be found on - * the page, true otherwise. + * the page, true otherwise. lossy is set to true if bitmap is lossy for the + * selected block and false otherwise. * * Note, this is an optionally implemented function, therefore should only be * used after verifying the presence (at plan time or such). */ static inline bool table_scan_bitmap_next_block(TableScanDesc scan, - struct TBMIterateResult *tbmres) + struct TBMIterateResult *tbmres, + bool *lossy) { /* * We don't expect direct calls to table_scan_bitmap_next_block with valid @@ -1969,7 +1975,7 @@ table_scan_bitmap_next_block(TableScanDesc scan, elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding"); return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan, - tbmres); + tbmres, lossy); } /* -- 2.40.1 --kqqpqghcwbcc3dt5 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch" ^ permalink raw reply [nested|flat] 25+ messages in thread
end of thread, other threads:[~2024-02-27 01:34 UTC | newest] Thread overview: 25+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-02-09 15:51 Re: Weird failure with latches in curculio on v15 Tom Lane <[email protected]> 2023-02-09 16:12 ` Robert Haas <[email protected]> 2023-02-09 17:23 ` Nathan Bossart <[email protected]> 2023-02-16 09:38 ` Robert Haas <[email protected]> 2023-02-16 16:57 ` Nathan Bossart <[email protected]> 2023-02-18 08:49 ` Robert Haas <[email protected]> 2023-02-09 19:29 ` Andres Freund <[email protected]> 2023-02-14 17:47 ` Nathan Bossart <[email protected]> 2023-02-21 00:03 ` Michael Paquier <[email protected]> 2023-02-21 00:32 ` Thomas Munro <[email protected]> 2023-02-21 04:50 ` Nathan Bossart <[email protected]> 2023-02-22 08:48 ` Thomas Munro <[email protected]> 2023-02-23 23:16 ` Nathan Bossart <[email protected]> 2023-02-16 09:48 ` Robert Haas <[email protected]> 2023-02-16 16:32 ` Andres Freund <[email protected]> 2023-02-18 10:21 ` Robert Haas <[email protected]> 2023-02-18 21:15 ` Andres Freund <[email protected]> 2023-02-19 14:36 ` Robert Haas <[email protected]> 2023-02-20 22:38 ` Re: Weird failure with latches in curculio on v15[ Michael Paquier <[email protected]> 2023-02-20 23:17 ` Re: Weird failure with latches in curculio on v15[ Stephen Frost <[email protected]> 2023-02-25 19:00 ` Andres Freund <[email protected]> 2023-03-01 04:29 ` Nathan Bossart <[email protected]> 2023-03-01 15:00 ` Stephen Frost <[email protected]> 2023-03-01 17:44 ` Robert Haas <[email protected]> 2024-02-27 01:34 [PATCH v7 06/13] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox