archive modules loose ends
Andres recently reminded me of some loose ends in archive modules [0]/messages/by-id/20230216192956.mhi6uiakchkolpki@awork3.anarazel.de, so
I'm starting a dedicated thread to address his feedback.
The first one is the requirement that archive module authors create their
own exception handlers if they want to make use of ERROR. Ideally, there
would be a handler in pgarch.c so that authors wouldn't need to deal with
this. I do see some previous dicussion about this [1]/messages/by-id/20220202224433.GA1036711@nathanxps13 in which I expressed
concerns about memory management. Looking at this again, I may have been
overthinking it. IIRC I was thinking about creating a memory context that
would be switched into for only the archiving callback (and reset
afterwards), but that might not be necessary. Instead, we could rely on
module authors to handle this. One example is basic_archive, which
maintains its own memory context. Alternatively, authors could simply
pfree() anything that was allocated.
Furthermore, by moving the exception handling to pgarch.c, module authors
can begin using PG_TRY, etc. in their archiving callbacks, which simplifies
things a bit. I've attached a work-in-progress patch for this change.
On Fri, Feb 17, 2023 at 11:41:32AM -0800, Andres Freund wrote:
On 2023-02-16 13:58:10 -0800, Nathan Bossart wrote:
On Thu, Feb 16, 2023 at 01:17:54PM -0800, Andres Freund wrote:
I'm quite baffled by:
/* Close any files left open by copy_file() or compare_files() */
AtEOSubXact_Files(false, InvalidSubTransactionId, InvalidSubTransactionId);in basic_archive_file(). It seems *really* off to call AtEOSubXact_Files()
completely outside the context of a transaction environment. And it only does
the thing you want because you pass parameters that aren't actually valid in
the normal use in AtEOSubXact_Files(). I really don't understand how that's
supposed to be ok.Hm. Should copy_file() and compare_files() have PG_FINALLY blocks that
attempt to close the files instead? What would you recommend?I don't fully now, it's not entirely clear to me what the goals here were. I
think you'd likely need to do a bit of infrastructure work to do this
sanely. So far we just didn't have the need to handle files being released in
a way like you want to do there.I suspect a good direction would be to use resource owners. Add a separate set
of functions that release files on resource owner release. Most of the
infrastructure is there already, for temporary files
(c.f. OpenTemporaryFile()).Then that resource owner could be reset in case of error.
I'm not even sure that erroring out is a reasonable way to implement
copy_file(), compare_files(), particularly because you want to return via a
return code from basic_archive_files().
To initialize this thread, I'll provide a bit more background.
basic_archive makes use of copy_file(), and it introduces a function called
compare_files() that is used to check whether two files have the same
content. These functions make use of OpenTransientFile() and
CloseTransientFile(). In basic_archive's sigsetjmp() block, there's a call
to AtEOSubXact_Files() to make sure we close any files that are open when
there is an ERROR. IIRC I was following the example set by other processes
that make use of the AtEOXact* functions in their sigsetjmp() blocks.
Looking again, I think AtEOXact_Files() would also work for basic_archive's
use-case. That would at least avoid the hack of using
InvalidSubTransactionId for the second and third arguments.
From the feedback quoted above, it sounds like improving this further will
require a bit of infrastructure work. I haven't looked too deeply into
this yet.
[0]: /messages/by-id/20230216192956.mhi6uiakchkolpki@awork3.anarazel.de
[1]: /messages/by-id/20220202224433.GA1036711@nathanxps13
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
archiver_exception_handling_WIP.patchtext/x-diff; charset=us-asciiDownload
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index cd852888ce..2c51d2b100 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -172,7 +172,6 @@ basic_archive_configured(ArchiveModuleState *state)
static bool
basic_archive_file(ArchiveModuleState *state, const char *file, const char *path)
{
- sigjmp_buf local_sigjmp_buf;
MemoryContext oldcontext;
BasicArchiveData *data = (BasicArchiveData *) state->private_data;
MemoryContext basic_archive_context = data->context;
@@ -184,51 +183,22 @@ basic_archive_file(ArchiveModuleState *state, const char *file, const char *path
*/
oldcontext = MemoryContextSwitchTo(basic_archive_context);
- /*
- * Since the archiver operates at the bottom of the exception stack,
- * ERRORs turn into FATALs and cause the archiver process to restart.
- * However, using ereport(ERROR, ...) when there are problems is easy to
- * code and maintain. Therefore, we create our own exception handler to
- * catch ERRORs and return false instead of restarting the archiver
- * whenever there is a failure.
- */
- if (sigsetjmp(local_sigjmp_buf, 1) != 0)
+ PG_TRY();
+ {
+ /* Archive the file! */
+ basic_archive_file_internal(file, path);
+ }
+ PG_CATCH();
{
- /* Since not using PG_TRY, must reset error stack by hand */
- error_context_stack = NULL;
-
- /* Prevent interrupts while cleaning up */
- HOLD_INTERRUPTS();
-
- /* Report the error and clear ErrorContext for next time */
- EmitErrorReport();
- FlushErrorState();
-
/* Close any files left open by copy_file() or compare_files() */
- AtEOSubXact_Files(false, InvalidSubTransactionId, InvalidSubTransactionId);
+ AtEOXact_Files(false);
- /* Reset our memory context and switch back to the original one */
- MemoryContextSwitchTo(oldcontext);
+ /* Reset our memory context */
MemoryContextReset(basic_archive_context);
- /* Remove our exception handler */
- PG_exception_stack = NULL;
-
- /* Now we can allow interrupts again */
- RESUME_INTERRUPTS();
-
- /* Report failure so that the archiver retries this file */
- return false;
+ PG_RE_THROW();
}
-
- /* Enable our exception handler */
- PG_exception_stack = &local_sigjmp_buf;
-
- /* Archive the file! */
- basic_archive_file_internal(file, path);
-
- /* Remove our exception handler */
- PG_exception_stack = NULL;
+ PG_END_TRY();
/* Reset our memory context and switch back to the original one */
MemoryContextSwitchTo(oldcontext);
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 46af349564..5ca8457b13 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -501,6 +501,8 @@ pgarch_ArchiverCopyLoop(void)
static bool
pgarch_archiveXlog(char *xlog)
{
+ sigjmp_buf local_sigjmp_buf;
+ MemoryContext oldcontext = CurrentMemoryContext;
char pathname[MAXPGPATH];
char activitymsg[MAXFNAMELEN + 16];
bool ret;
@@ -511,7 +513,49 @@ pgarch_archiveXlog(char *xlog)
snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
set_ps_display(activitymsg);
- ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
+ /*
+ * Since the archiver operates at the bottom of the exception stack,
+ * ERRORs turn into FATALs and cause the archiver process to restart.
+ * However, using ereport(ERROR, ...) when there are problems is easy to
+ * code and maintain. Therefore, we create our own exception handler to
+ * catch ERRORs and return false instead of restarting the archiver
+ * whenever there is a failure.
+ */
+ if (sigsetjmp(local_sigjmp_buf, 1) != 0)
+ {
+ /* Since not using PG_TRY, must reset error stack by hand */
+ error_context_stack = NULL;
+
+ /* Prevent interrupts while cleaning up */
+ HOLD_INTERRUPTS();
+
+ /* Report the error and clear ErrorContext for next time */
+ EmitErrorReport();
+ MemoryContextSwitchTo(oldcontext);
+ FlushErrorState();
+
+ /* Remove our exception handler */
+ PG_exception_stack = NULL;
+
+ /* Now we can allow interrupts again */
+ RESUME_INTERRUPTS();
+
+ /* Report failure so that the archiver retries this file */
+ ret = false;
+ }
+ else
+ {
+ /* Enable our exception handler */
+ PG_exception_stack = &local_sigjmp_buf;
+
+ /* Archive the file! */
+ ret = ArchiveCallbacks->archive_file_cb(archive_module_state,
+ xlog, pathname);
+
+ /* Remove our exception handler */
+ PG_exception_stack = NULL;
+ }
+
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
There seems to be no interest in this patch, so I plan to withdraw it from
the commitfest system by the end of the month unless such interest
materializes.
On Fri, Feb 17, 2023 at 01:56:24PM -0800, Nathan Bossart wrote:
The first one is the requirement that archive module authors create their
own exception handlers if they want to make use of ERROR. Ideally, there
would be a handler in pgarch.c so that authors wouldn't need to deal with
this. I do see some previous dicussion about this [1] in which I expressed
concerns about memory management. Looking at this again, I may have been
overthinking it. IIRC I was thinking about creating a memory context that
would be switched into for only the archiving callback (and reset
afterwards), but that might not be necessary. Instead, we could rely on
module authors to handle this. One example is basic_archive, which
maintains its own memory context. Alternatively, authors could simply
pfree() anything that was allocated.Furthermore, by moving the exception handling to pgarch.c, module authors
can begin using PG_TRY, etc. in their archiving callbacks, which simplifies
things a bit. I've attached a work-in-progress patch for this change.
I took another look at this, and I think І remembered what I was worried
about with memory management. One example is the built-in shell archiving.
Presently, whenever there is an ERROR during archiving via shell, it gets
bumped up to FATAL because the archiver operates at the bottom of the
exception stack. Consequently, there's no need to worry about managing
memory contexts to ensure that palloc'd memory is cleared up after an
error. With the attached patch, we no longer call the archiving callback
while we're at the bottom of the exception stack, so ERRORs no longer get
bumped up to FATALs, and any palloc'd memory won't be freed.
I see two main options for dealing with this. One option is to simply have
shell_archive (and any other archive modules out there) maintain its own
memory context like basic_archive does. This ends up requiring a whole lot
of duplicate code between the two built-in modules, though. Another option
is to have the archiver manage a memory context that it resets after every
invocation of the archiving callback, ERROR or not. This has the advantage
of avoiding code duplication and simplifying things for the built-in
modules, but any external modules that rely on palloc'd state being
long-lived would need to be adjusted to manage their own long-lived
context. (This would need to be appropriately documented.) However, I'm
not aware of any archive modules that would be impacted by this.
The attached patch is an attempt at the latter option. As I noted above,
this probably deserves some discussion in the archive modules
documentation, but I don't intend to spend too much more time on this patch
right now given it is likely going to be withdrawn.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
archiver_exception_handling_v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 4d78c31859..bf9dd3f8e7 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -40,26 +40,19 @@
PG_MODULE_MAGIC;
-typedef struct BasicArchiveData
-{
- MemoryContext context;
-} BasicArchiveData;
-
static char *archive_directory = NULL;
-static void basic_archive_startup(ArchiveModuleState *state);
static bool basic_archive_configured(ArchiveModuleState *state);
static bool basic_archive_file(ArchiveModuleState *state, const char *file, const char *path);
static void basic_archive_file_internal(const char *file, const char *path);
static bool check_archive_directory(char **newval, void **extra, GucSource source);
static bool compare_files(const char *file1, const char *file2);
-static void basic_archive_shutdown(ArchiveModuleState *state);
static const ArchiveModuleCallbacks basic_archive_callbacks = {
- .startup_cb = basic_archive_startup,
+ .startup_cb = NULL,
.check_configured_cb = basic_archive_configured,
.archive_file_cb = basic_archive_file,
- .shutdown_cb = basic_archive_shutdown
+ .shutdown_cb = NULL
};
/*
@@ -93,24 +86,6 @@ _PG_archive_module_init(void)
return &basic_archive_callbacks;
}
-/*
- * basic_archive_startup
- *
- * Creates the module's memory context.
- */
-void
-basic_archive_startup(ArchiveModuleState *state)
-{
- BasicArchiveData *data;
-
- data = (BasicArchiveData *) MemoryContextAllocZero(TopMemoryContext,
- sizeof(BasicArchiveData));
- data->context = AllocSetContextCreate(TopMemoryContext,
- "basic_archive",
- ALLOCSET_DEFAULT_SIZES);
- state->private_data = (void *) data;
-}
-
/*
* check_archive_directory
*
@@ -172,67 +147,19 @@ basic_archive_configured(ArchiveModuleState *state)
static bool
basic_archive_file(ArchiveModuleState *state, const char *file, const char *path)
{
- sigjmp_buf local_sigjmp_buf;
- MemoryContext oldcontext;
- BasicArchiveData *data = (BasicArchiveData *) state->private_data;
- MemoryContext basic_archive_context = data->context;
-
- /*
- * We run basic_archive_file_internal() in our own memory context so that
- * we can easily reset it during error recovery (thus avoiding memory
- * leaks).
- */
- oldcontext = MemoryContextSwitchTo(basic_archive_context);
-
- /*
- * Since the archiver operates at the bottom of the exception stack,
- * ERRORs turn into FATALs and cause the archiver process to restart.
- * However, using ereport(ERROR, ...) when there are problems is easy to
- * code and maintain. Therefore, we create our own exception handler to
- * catch ERRORs and return false instead of restarting the archiver
- * whenever there is a failure.
- */
- if (sigsetjmp(local_sigjmp_buf, 1) != 0)
+ PG_TRY();
+ {
+ /* Archive the file! */
+ basic_archive_file_internal(file, path);
+ }
+ PG_CATCH();
{
- /* Since not using PG_TRY, must reset error stack by hand */
- error_context_stack = NULL;
-
- /* Prevent interrupts while cleaning up */
- HOLD_INTERRUPTS();
-
- /* Report the error and clear ErrorContext for next time */
- EmitErrorReport();
- FlushErrorState();
-
/* Close any files left open by copy_file() or compare_files() */
- AtEOSubXact_Files(false, InvalidSubTransactionId, InvalidSubTransactionId);
-
- /* Reset our memory context and switch back to the original one */
- MemoryContextSwitchTo(oldcontext);
- MemoryContextReset(basic_archive_context);
-
- /* Remove our exception handler */
- PG_exception_stack = NULL;
+ AtEOXact_Files(false);
- /* Now we can allow interrupts again */
- RESUME_INTERRUPTS();
-
- /* Report failure so that the archiver retries this file */
- return false;
+ PG_RE_THROW();
}
-
- /* Enable our exception handler */
- PG_exception_stack = &local_sigjmp_buf;
-
- /* Archive the file! */
- basic_archive_file_internal(file, path);
-
- /* Remove our exception handler */
- PG_exception_stack = NULL;
-
- /* Reset our memory context and switch back to the original one */
- MemoryContextSwitchTo(oldcontext);
- MemoryContextReset(basic_archive_context);
+ PG_END_TRY();
return true;
}
@@ -394,35 +321,3 @@ compare_files(const char *file1, const char *file2)
return ret;
}
-
-/*
- * basic_archive_shutdown
- *
- * Frees our allocated state.
- */
-static void
-basic_archive_shutdown(ArchiveModuleState *state)
-{
- BasicArchiveData *data = (BasicArchiveData *) state->private_data;
- MemoryContext basic_archive_context;
-
- /*
- * If we didn't get to storing the pointer to our allocated state, we
- * don't have anything to clean up.
- */
- if (data == NULL)
- return;
-
- basic_archive_context = data->context;
- Assert(CurrentMemoryContext != basic_archive_context);
-
- if (MemoryContextIsValid(basic_archive_context))
- MemoryContextDelete(basic_archive_context);
- data->context = NULL;
-
- /*
- * Finally, free the state.
- */
- pfree(data);
- state->private_data = NULL;
-}
diff --git a/src/backend/archive/shell_archive.c b/src/backend/archive/shell_archive.c
index 157ca4c751..793f607db8 100644
--- a/src/backend/archive/shell_archive.c
+++ b/src/backend/archive/shell_archive.c
@@ -66,9 +66,6 @@ shell_archive_file(ArchiveModuleState *state, const char *file,
"archive_command", "fp",
file, nativePath);
- if (nativePath)
- pfree(nativePath);
-
ereport(DEBUG3,
(errmsg_internal("executing archive command \"%s\"",
xlogarchcmd)));
@@ -127,8 +124,6 @@ shell_archive_file(ArchiveModuleState *state, const char *file,
return false;
}
- pfree(xlogarchcmd);
-
elog(DEBUG1, "archived write-ahead log file \"%s\"", file);
return true;
}
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 46af349564..bb9c437f95 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -101,6 +101,7 @@ static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
static const ArchiveModuleCallbacks *ArchiveCallbacks;
static ArchiveModuleState *archive_module_state;
+static MemoryContext archive_context;
/*
@@ -252,6 +253,11 @@ PgArchiverMain(void)
arch_files->arch_heap = binaryheap_allocate(NUM_FILES_PER_DIRECTORY_SCAN,
ready_file_comparator, NULL);
+ /* Initialize our memory context. */
+ archive_context = AllocSetContextCreate(TopMemoryContext,
+ "archiver",
+ ALLOCSET_DEFAULT_SIZES);
+
/* Load the archive_library. */
LoadArchiveLibrary();
@@ -501,6 +507,8 @@ pgarch_ArchiverCopyLoop(void)
static bool
pgarch_archiveXlog(char *xlog)
{
+ sigjmp_buf local_sigjmp_buf;
+ MemoryContext oldcontext;
char pathname[MAXPGPATH];
char activitymsg[MAXFNAMELEN + 16];
bool ret;
@@ -511,7 +519,58 @@ pgarch_archiveXlog(char *xlog)
snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
set_ps_display(activitymsg);
- ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
+ oldcontext = MemoryContextSwitchTo(archive_context);
+
+ /*
+ * Since the archiver operates at the bottom of the exception stack,
+ * ERRORs turn into FATALs and cause the archiver process to restart.
+ * However, using ereport(ERROR, ...) when there are problems is easy to
+ * code and maintain. Therefore, we create our own exception handler to
+ * catch ERRORs and return false instead of restarting the archiver
+ * whenever there is a failure.
+ */
+ if (sigsetjmp(local_sigjmp_buf, 1) != 0)
+ {
+ /* Since not using PG_TRY, must reset error stack by hand */
+ error_context_stack = NULL;
+
+ /* Prevent interrupts while cleaning up */
+ HOLD_INTERRUPTS();
+
+ /* Report the error and clear ErrorContext for next time */
+ EmitErrorReport();
+ MemoryContextSwitchTo(oldcontext);
+ FlushErrorState();
+
+ /* Flush any leaked data */
+ MemoryContextReset(archive_context);
+
+ /* Remove our exception handler */
+ PG_exception_stack = NULL;
+
+ /* Now we can allow interrupts again */
+ RESUME_INTERRUPTS();
+
+ /* Report failure so that the archiver retries this file */
+ ret = false;
+ }
+ else
+ {
+ /* Enable our exception handler */
+ PG_exception_stack = &local_sigjmp_buf;
+
+ /* Archive the file! */
+ ret = ArchiveCallbacks->archive_file_cb(archive_module_state,
+ xlog, pathname);
+
+ /* Remove our exception handler */
+ PG_exception_stack = NULL;
+
+ /* Reset our memory context and switch back to the original one */
+ MemoryContextSwitchTo(oldcontext);
+ MemoryContextReset(archive_context);
+ }
+
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
Hi,
On 2023-11-13 16:42:31 -0600, Nathan Bossart wrote:
There seems to be no interest in this patch, so I plan to withdraw it from
the commitfest system by the end of the month unless such interest
materializes.
I think it might just have arrived too shortly before the feature freeze to be
worth looking at at the time, and then it didn't really re-raise attention
until now. I'm so far behind on keeping up with the list that I rarely end up
looking far back for things I'd like to have answered... Sorry.
I think it's somewhat important to fix this - having a dedicated "recover from
error" implementation in a bunch of extension modules seems quite likely to
cause problems down the line, when another type of resource needs to be dealt
with after errors. I think many non-toy implementations would e.g. need to
release lwlocks in case of errors (e.g. because they use a shared hashtable to
queue jobs for workers or such).
On Fri, Feb 17, 2023 at 01:56:24PM -0800, Nathan Bossart wrote:
The first one is the requirement that archive module authors create their
own exception handlers if they want to make use of ERROR. Ideally, there
would be a handler in pgarch.c so that authors wouldn't need to deal with
this. I do see some previous dicussion about this [1] in which I expressed
concerns about memory management. Looking at this again, I may have been
overthinking it. IIRC I was thinking about creating a memory context that
would be switched into for only the archiving callback (and reset
afterwards), but that might not be necessary. Instead, we could rely on
module authors to handle this. One example is basic_archive, which
maintains its own memory context. Alternatively, authors could simply
pfree() anything that was allocated.Furthermore, by moving the exception handling to pgarch.c, module authors
can begin using PG_TRY, etc. in their archiving callbacks, which simplifies
things a bit. I've attached a work-in-progress patch for this change.I took another look at this, and I think І remembered what I was worried
about with memory management. One example is the built-in shell archiving.
Presently, whenever there is an ERROR during archiving via shell, it gets
bumped up to FATAL because the archiver operates at the bottom of the
exception stack. Consequently, there's no need to worry about managing
memory contexts to ensure that palloc'd memory is cleared up after an
error. With the attached patch, we no longer call the archiving callback
while we're at the bottom of the exception stack, so ERRORs no longer get
bumped up to FATALs, and any palloc'd memory won't be freed.I see two main options for dealing with this. One option is to simply have
shell_archive (and any other archive modules out there) maintain its own
memory context like basic_archive does. This ends up requiring a whole lot
of duplicate code between the two built-in modules, though. Another option
is to have the archiver manage a memory context that it resets after every
invocation of the archiving callback, ERROR or not.
I think passing in a short-lived memory context is a lot nicer to deal with.
This has the advantage of avoiding code duplication and simplifying things
for the built-in modules, but any external modules that rely on palloc'd
state being long-lived would need to be adjusted to manage their own
long-lived context. (This would need to be appropriately documented.)
Alternatively we could provide a longer-lived memory context in
ArchiveModuleState, set up by the genric infrastructure. That context would
obviously still need to be explicitly utilized by a module, but no duplicated
setup code would be required.
/* * check_archive_directory * @@ -172,67 +147,19 @@ basic_archive_configured(ArchiveModuleState *state) static bool basic_archive_file(ArchiveModuleState *state, const char *file, const char *path) { ... + PG_TRY(); + { + /* Archive the file! */ + basic_archive_file_internal(file, path); + } + PG_CATCH(); { - /* Since not using PG_TRY, must reset error stack by hand */ - error_context_stack = NULL; - - /* Prevent interrupts while cleaning up */ - HOLD_INTERRUPTS(); - - /* Report the error and clear ErrorContext for next time */ - EmitErrorReport(); - FlushErrorState(); - /* Close any files left open by copy_file() or compare_files() */ - AtEOSubXact_Files(false, InvalidSubTransactionId, InvalidSubTransactionId); - - /* Reset our memory context and switch back to the original one */ - MemoryContextSwitchTo(oldcontext); - MemoryContextReset(basic_archive_context); - - /* Remove our exception handler */ - PG_exception_stack = NULL; + AtEOXact_Files(false);- /* Now we can allow interrupts again */ - RESUME_INTERRUPTS(); - - /* Report failure so that the archiver retries this file */ - return false; + PG_RE_THROW(); }
I think we should just have the AtEOXact_Files() in pgarch.c, then no
PG_TRY/CATCH is needed here. At the moment I think just about every possible
use of an archive modules would require using files, so there doesn't seem
much of a reason to not handle it in pgarch.c.
I'd probably reset a few other subsystems at the same time (there's probably
more):
- disable_all_timeouts()
- LWLockReleaseAll()
- ConditionVariableCancelSleep()
- pgstat_report_wait_end()
- ReleaseAuxProcessResources()
@@ -511,7 +519,58 @@ pgarch_archiveXlog(char *xlog)
snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
set_ps_display(activitymsg);- ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname); + oldcontext = MemoryContextSwitchTo(archive_context); + + /* + * Since the archiver operates at the bottom of the exception stack, + * ERRORs turn into FATALs and cause the archiver process to restart. + * However, using ereport(ERROR, ...) when there are problems is easy to + * code and maintain. Therefore, we create our own exception handler to + * catch ERRORs and return false instead of restarting the archiver + * whenever there is a failure. + */ + if (sigsetjmp(local_sigjmp_buf, 1) != 0) + { + /* Since not using PG_TRY, must reset error stack by hand */ + error_context_stack = NULL; + + /* Prevent interrupts while cleaning up */ + HOLD_INTERRUPTS(); + + /* Report the error and clear ErrorContext for next time */ + EmitErrorReport(); + MemoryContextSwitchTo(oldcontext); + FlushErrorState(); + + /* Flush any leaked data */ + MemoryContextReset(archive_context); + + /* Remove our exception handler */ + PG_exception_stack = NULL; + + /* Now we can allow interrupts again */ + RESUME_INTERRUPTS(); + + /* Report failure so that the archiver retries this file */ + ret = false; + } + else + { + /* Enable our exception handler */ + PG_exception_stack = &local_sigjmp_buf; + + /* Archive the file! */ + ret = ArchiveCallbacks->archive_file_cb(archive_module_state, + xlog, pathname); + + /* Remove our exception handler */ + PG_exception_stack = NULL; + + /* Reset our memory context and switch back to the original one */ + MemoryContextSwitchTo(oldcontext); + MemoryContextReset(archive_context); + }
It could be worth setting up an errcontext providing the module and file
that's being processed. I personally find that at least as important as
setting up a ps string detailing the log file... But I guess that could be a
separate patch.
It'd be nice to add a comment explaining why pgarch_archiveXlog() is the right
place to handle errors.
Greetings,
Andres Freund
On Mon, Nov 13, 2023 at 03:35:28PM -0800, Andres Freund wrote:
On 2023-11-13 16:42:31 -0600, Nathan Bossart wrote:
There seems to be no interest in this patch, so I plan to withdraw it from
the commitfest system by the end of the month unless such interest
materializes.I think it might just have arrived too shortly before the feature freeze to be
worth looking at at the time, and then it didn't really re-raise attention
until now. I'm so far behind on keeping up with the list that I rarely end up
looking far back for things I'd like to have answered... Sorry.
No worries. I appreciate the review.
I see two main options for dealing with this. One option is to simply have
shell_archive (and any other archive modules out there) maintain its own
memory context like basic_archive does. This ends up requiring a whole lot
of duplicate code between the two built-in modules, though. Another option
is to have the archiver manage a memory context that it resets after every
invocation of the archiving callback, ERROR or not.I think passing in a short-lived memory context is a lot nicer to deal with.
Cool.
This has the advantage of avoiding code duplication and simplifying things
for the built-in modules, but any external modules that rely on palloc'd
state being long-lived would need to be adjusted to manage their own
long-lived context. (This would need to be appropriately documented.)Alternatively we could provide a longer-lived memory context in
ArchiveModuleState, set up by the genric infrastructure. That context would
obviously still need to be explicitly utilized by a module, but no duplicated
setup code would be required.
Sure. Right now, I'm not sure there's too much need for that. A module
could just throw stuff in TopMemoryContext, and you probably wouldn't have
any leaks because the archiver just restarts on any ERROR or
archive_library change. But that's probably not a pattern we want to
encourage long-term. I'll jot this down for a follow-up patch idea.
I think we should just have the AtEOXact_Files() in pgarch.c, then no
PG_TRY/CATCH is needed here. At the moment I think just about every possible
use of an archive modules would require using files, so there doesn't seem
much of a reason to not handle it in pgarch.c.
WFM
I'd probably reset a few other subsystems at the same time (there's probably
more):
- disable_all_timeouts()
- LWLockReleaseAll()
- ConditionVariableCancelSleep()
- pgstat_report_wait_end()
- ReleaseAuxProcessResources()
I looked around a bit and thought AtEOXact_HashTables() belonged here as
well. I'll probably give this one another pass to see if there's anything
else obvious.
It could be worth setting up an errcontext providing the module and file
that's being processed. I personally find that at least as important as
setting up a ps string detailing the log file... But I guess that could be a
separate patch.
Indeed. Right now we rely on the module to emit sufficiently-detailed
logs, but it'd be nice if they got that for free.
It'd be nice to add a comment explaining why pgarch_archiveXlog() is the right
place to handle errors.
Will do.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Here is a new version of the patch with feedback addressed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v3-0001-archiver-exception-handling.patchtext/x-diff; charset=us-asciiDownload
From 3cda5bb87c82738ad6f8a082ef5dfeb49cd51392 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 28 Nov 2023 11:17:13 -0600
Subject: [PATCH v3 1/1] archiver exception handling
---
contrib/basic_archive/basic_archive.c | 134 +-------------------------
doc/src/sgml/archive-modules.sgml | 11 ++-
src/backend/archive/shell_archive.c | 5 -
src/backend/postmaster/pgarch.c | 93 +++++++++++++++++-
4 files changed, 107 insertions(+), 136 deletions(-)
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 4d78c31859..d575faab93 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -40,26 +40,18 @@
PG_MODULE_MAGIC;
-typedef struct BasicArchiveData
-{
- MemoryContext context;
-} BasicArchiveData;
-
static char *archive_directory = NULL;
-static void basic_archive_startup(ArchiveModuleState *state);
static bool basic_archive_configured(ArchiveModuleState *state);
static bool basic_archive_file(ArchiveModuleState *state, const char *file, const char *path);
-static void basic_archive_file_internal(const char *file, const char *path);
static bool check_archive_directory(char **newval, void **extra, GucSource source);
static bool compare_files(const char *file1, const char *file2);
-static void basic_archive_shutdown(ArchiveModuleState *state);
static const ArchiveModuleCallbacks basic_archive_callbacks = {
- .startup_cb = basic_archive_startup,
+ .startup_cb = NULL,
.check_configured_cb = basic_archive_configured,
.archive_file_cb = basic_archive_file,
- .shutdown_cb = basic_archive_shutdown
+ .shutdown_cb = NULL
};
/*
@@ -93,24 +85,6 @@ _PG_archive_module_init(void)
return &basic_archive_callbacks;
}
-/*
- * basic_archive_startup
- *
- * Creates the module's memory context.
- */
-void
-basic_archive_startup(ArchiveModuleState *state)
-{
- BasicArchiveData *data;
-
- data = (BasicArchiveData *) MemoryContextAllocZero(TopMemoryContext,
- sizeof(BasicArchiveData));
- data->context = AllocSetContextCreate(TopMemoryContext,
- "basic_archive",
- ALLOCSET_DEFAULT_SIZES);
- state->private_data = (void *) data;
-}
-
/*
* check_archive_directory
*
@@ -171,74 +145,6 @@ basic_archive_configured(ArchiveModuleState *state)
*/
static bool
basic_archive_file(ArchiveModuleState *state, const char *file, const char *path)
-{
- sigjmp_buf local_sigjmp_buf;
- MemoryContext oldcontext;
- BasicArchiveData *data = (BasicArchiveData *) state->private_data;
- MemoryContext basic_archive_context = data->context;
-
- /*
- * We run basic_archive_file_internal() in our own memory context so that
- * we can easily reset it during error recovery (thus avoiding memory
- * leaks).
- */
- oldcontext = MemoryContextSwitchTo(basic_archive_context);
-
- /*
- * Since the archiver operates at the bottom of the exception stack,
- * ERRORs turn into FATALs and cause the archiver process to restart.
- * However, using ereport(ERROR, ...) when there are problems is easy to
- * code and maintain. Therefore, we create our own exception handler to
- * catch ERRORs and return false instead of restarting the archiver
- * whenever there is a failure.
- */
- if (sigsetjmp(local_sigjmp_buf, 1) != 0)
- {
- /* Since not using PG_TRY, must reset error stack by hand */
- error_context_stack = NULL;
-
- /* Prevent interrupts while cleaning up */
- HOLD_INTERRUPTS();
-
- /* Report the error and clear ErrorContext for next time */
- EmitErrorReport();
- FlushErrorState();
-
- /* Close any files left open by copy_file() or compare_files() */
- AtEOSubXact_Files(false, InvalidSubTransactionId, InvalidSubTransactionId);
-
- /* Reset our memory context and switch back to the original one */
- MemoryContextSwitchTo(oldcontext);
- MemoryContextReset(basic_archive_context);
-
- /* Remove our exception handler */
- PG_exception_stack = NULL;
-
- /* Now we can allow interrupts again */
- RESUME_INTERRUPTS();
-
- /* Report failure so that the archiver retries this file */
- return false;
- }
-
- /* Enable our exception handler */
- PG_exception_stack = &local_sigjmp_buf;
-
- /* Archive the file! */
- basic_archive_file_internal(file, path);
-
- /* Remove our exception handler */
- PG_exception_stack = NULL;
-
- /* Reset our memory context and switch back to the original one */
- MemoryContextSwitchTo(oldcontext);
- MemoryContextReset(basic_archive_context);
-
- return true;
-}
-
-static void
-basic_archive_file_internal(const char *file, const char *path)
{
char destination[MAXPGPATH];
char temp[MAXPGPATH + 256];
@@ -272,7 +178,7 @@ basic_archive_file_internal(const char *file, const char *path)
fsync_fname(destination, false);
fsync_fname(archive_directory, true);
- return;
+ return true;
}
ereport(ERROR,
@@ -312,6 +218,8 @@ basic_archive_file_internal(const char *file, const char *path)
ereport(DEBUG1,
(errmsg("archived \"%s\" via basic_archive", file)));
+
+ return true;
}
/*
@@ -394,35 +302,3 @@ compare_files(const char *file1, const char *file2)
return ret;
}
-
-/*
- * basic_archive_shutdown
- *
- * Frees our allocated state.
- */
-static void
-basic_archive_shutdown(ArchiveModuleState *state)
-{
- BasicArchiveData *data = (BasicArchiveData *) state->private_data;
- MemoryContext basic_archive_context;
-
- /*
- * If we didn't get to storing the pointer to our allocated state, we
- * don't have anything to clean up.
- */
- if (data == NULL)
- return;
-
- basic_archive_context = data->context;
- Assert(CurrentMemoryContext != basic_archive_context);
-
- if (MemoryContextIsValid(basic_archive_context))
- MemoryContextDelete(basic_archive_context);
- data->context = NULL;
-
- /*
- * Finally, free the state.
- */
- pfree(data);
- state->private_data = NULL;
-}
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index 7064307d9e..f9b8d6c740 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -128,12 +128,21 @@ typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, cons
If <literal>true</literal> is returned, the server proceeds as if the file
was successfully archived, which may include recycling or removing the
- original WAL file. If <literal>false</literal> is returned, the server will
+ original WAL file. If <literal>false</literal> is returned or an error is thrown, the server will
keep the original WAL file and retry archiving later.
<replaceable>file</replaceable> will contain just the file name of the WAL
file to archive, while <replaceable>path</replaceable> contains the full
path of the WAL file (including the file name).
</para>
+
+ <note>
+ <para>
+ The <function>archive_file_cb</function> callback is called in a
+ short-lived memory context that will be reset between invocations. If you
+ need longer-lived storage, create a memory context in the module's
+ <function>startup_cb</function> callback.
+ </para>
+ </note>
</sect2>
<sect2 id="archive-module-shutdown">
diff --git a/src/backend/archive/shell_archive.c b/src/backend/archive/shell_archive.c
index 157ca4c751..793f607db8 100644
--- a/src/backend/archive/shell_archive.c
+++ b/src/backend/archive/shell_archive.c
@@ -66,9 +66,6 @@ shell_archive_file(ArchiveModuleState *state, const char *file,
"archive_command", "fp",
file, nativePath);
- if (nativePath)
- pfree(nativePath);
-
ereport(DEBUG3,
(errmsg_internal("executing archive command \"%s\"",
xlogarchcmd)));
@@ -127,8 +124,6 @@ shell_archive_file(ArchiveModuleState *state, const char *file,
return false;
}
- pfree(xlogarchcmd);
-
elog(DEBUG1, "archived write-ahead log file \"%s\"", file);
return true;
}
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 46af349564..8bb15ac960 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -38,6 +38,7 @@
#include "pgstat.h"
#include "postmaster/interrupt.h"
#include "postmaster/pgarch.h"
+#include "storage/condition_variable.h"
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/latch.h"
@@ -49,6 +50,8 @@
#include "utils/guc.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
+#include "utils/resowner.h"
+#include "utils/timeout.h"
/* ----------
@@ -101,6 +104,7 @@ static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
static const ArchiveModuleCallbacks *ArchiveCallbacks;
static ArchiveModuleState *archive_module_state;
+static MemoryContext archive_context;
/*
@@ -252,6 +256,11 @@ PgArchiverMain(void)
arch_files->arch_heap = binaryheap_allocate(NUM_FILES_PER_DIRECTORY_SCAN,
ready_file_comparator, NULL);
+ /* Initialize our memory context. */
+ archive_context = AllocSetContextCreate(TopMemoryContext,
+ "archiver",
+ ALLOCSET_DEFAULT_SIZES);
+
/* Load the archive_library. */
LoadArchiveLibrary();
@@ -501,6 +510,8 @@ pgarch_ArchiverCopyLoop(void)
static bool
pgarch_archiveXlog(char *xlog)
{
+ sigjmp_buf local_sigjmp_buf;
+ MemoryContext oldcontext;
char pathname[MAXPGPATH];
char activitymsg[MAXFNAMELEN + 16];
bool ret;
@@ -511,7 +522,87 @@ pgarch_archiveXlog(char *xlog)
snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
set_ps_display(activitymsg);
- ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
+ oldcontext = MemoryContextSwitchTo(archive_context);
+
+ /*
+ * Since the archiver operates at the bottom of the exception stack,
+ * ERRORs turn into FATALs and cause the archiver process to restart.
+ * However, using ereport(ERROR, ...) when there are problems is easy to
+ * code and maintain. Therefore, we create our own exception handler to
+ * catch ERRORs and return false instead of restarting the archiver
+ * whenever there is a failure.
+ *
+ * We assume ERRORs from the archiving callback are the most common
+ * exceptions experienced by the archiver, so we opt to handle exceptions
+ * here instead of PgArchiverMain() to avoid reinitializing the archiver
+ * too frequently. We could instead add a sigsetjmp() block to
+ * PgArchiverMain() and use PG_TRY/PG_CATCH here, but the extra code to
+ * avoid the odd archiver restart doesn't seem worth it.
+ */
+ if (sigsetjmp(local_sigjmp_buf, 1) != 0)
+ {
+ /* Since not using PG_TRY, must reset error stack by hand */
+ error_context_stack = NULL;
+
+ /* Prevent interrupts while cleaning up */
+ HOLD_INTERRUPTS();
+
+ /* Report the error to the server log. */
+ EmitErrorReport();
+
+ /*
+ * Try to clean up anything the archive module left behind. We try to
+ * cover anything that an archive module could conceivably have left
+ * behind, but it is of course possible that modules could be doing
+ * unexpected things that require additional cleanup. Module authors
+ * should be sure to do any extra required cleanup in a PG_CATCH block
+ * within the archiving callback, and they are encouraged to notify
+ * the pgsql-hackers mailing list so that we can add it here.
+ */
+ disable_all_timeouts(false);
+ LWLockReleaseAll();
+ ConditionVariableCancelSleep();
+ pgstat_report_wait_end();
+ ReleaseAuxProcessResources(false);
+ AtEOXact_Files(false);
+ AtEOXact_HashTables(false);
+
+ /*
+ * Return to the original memory context and clear ErrorContext for
+ * next time.
+ */
+ MemoryContextSwitchTo(oldcontext);
+ FlushErrorState();
+
+ /* Flush any leaked data */
+ MemoryContextReset(archive_context);
+
+ /* Remove our exception handler */
+ PG_exception_stack = NULL;
+
+ /* Now we can allow interrupts again */
+ RESUME_INTERRUPTS();
+
+ /* Report failure so that the archiver retries this file */
+ ret = false;
+ }
+ else
+ {
+ /* Enable our exception handler */
+ PG_exception_stack = &local_sigjmp_buf;
+
+ /* Archive the file! */
+ ret = ArchiveCallbacks->archive_file_cb(archive_module_state,
+ xlog, pathname);
+
+ /* Remove our exception handler */
+ PG_exception_stack = NULL;
+
+ /* Reset our memory context and switch back to the original one */
+ MemoryContextSwitchTo(oldcontext);
+ MemoryContextReset(archive_context);
+ }
+
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
--
2.25.1
On Nov 29, 2023, at 01:18, Nathan Bossart <nathandbossart@gmail.com> wrote:
External Email
Here is a new version of the patch with feedback addressed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Hi Nathan,
The patch looks good to me. With the context explained in the thread, the patch is easy to understand.
The patch serves as a refactoring which pulls up common memory management and error handling concerns into the pgarch.c. With the patch, individual archive callbacks can focus on copying the files and leave the boilerplate code to pgarch.c.
The patch applies cleanly to HEAD. “make check-world” also runs cleanly with no error.
Regards,
Yong
On Mon, Jan 15, 2024 at 12:21:44PM +0000, Li, Yong wrote:
The patch looks good to me. With the context explained in the thread,
the patch is easy to understand.
The patch serves as a refactoring which pulls up common memory management
and error handling concerns into the pgarch.c. With the patch,
individual archive callbacks can focus on copying the files and leave the
boilerplate code to pgarch.c..The patch applies cleanly to HEAD. “make check-world” also runs cleanly
with no error.
Thanks for reviewing. I've marked this as ready-for-committer, and I'm
hoping to commit it in the near future.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Mon, Jan 15, 2024 at 08:50:25AM -0600, Nathan Bossart wrote:
Thanks for reviewing. I've marked this as ready-for-committer, and I'm
hoping to commit it in the near future.
This one probably ought to go into v17, but I wanted to do one last call
for feedback prior to committing.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Mar 26, 2024 at 02:14:14PM -0500, Nathan Bossart wrote:
On Mon, Jan 15, 2024 at 08:50:25AM -0600, Nathan Bossart wrote:
Thanks for reviewing. I've marked this as ready-for-committer, and I'm
hoping to commit it in the near future.This one probably ought to go into v17, but I wanted to do one last call
for feedback prior to committing.
Committed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com