From c627d944e6c2620fb3b28f2e4b27e19212f84045 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Tue, 2 Apr 2024 22:28:11 -0500 Subject: [PATCH] Add built-in ERROR handling for archive callbacks. Presently, the archiver process restarts when an archive callback ERRORs. To avoid this, archive module authors can use sigsetjmp(), manage a memory context, etc., but that requires a lot of extra code that will likely look roughly the same between modules. This commit adds basic archive callback ERROR handling to pgarch.c so that module authors won't ordinarily need to worry about this. While this built-in handler attempts to clean up anything that an archive module could conceivably have left behind, it is possible that some modules are 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. The archiving callback is now called in a short-lived memory context that the archiver process resets between invocations. If a module requires longer-lived storage, it must maintain its own memory context. Thanks to these changes, the basic_archive module can be greatly simplified. Suggested-by: Andres Freund Reviewed-by: Andres Freund, Yong Li Discussion: https://postgr.es/m/20230217215624.GA3131134%40nathanxps13 --- 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 6b102e9072..028cf51c25 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 * @@ -176,74 +150,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]; @@ -277,7 +183,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, @@ -317,6 +223,8 @@ basic_archive_file_internal(const char *file, const char *path) ereport(DEBUG1, (errmsg("archived \"%s\" via basic_archive", file))); + + return true; } /* @@ -399,35 +307,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 cf7438a759..10ec96eae9 100644 --- a/doc/src/sgml/archive-modules.sgml +++ b/doc/src/sgml/archive-modules.sgml @@ -140,12 +140,21 @@ typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, cons If true is returned, the server proceeds as if the file was successfully archived, which may include recycling or removing the - original WAL file. If false is returned, the server will + original WAL file. If false is returned or an error is thrown, the server will keep the original WAL file and retry archiving later. file will contain just the file name of the WAL file to archive, while path contains the full path of the WAL file (including the file name). + + + + The archive_file_cb 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 + startup_cb callback. + + diff --git a/src/backend/archive/shell_archive.c b/src/backend/archive/shell_archive.c index bff0ab800d..0925348bfe 100644 --- a/src/backend/archive/shell_archive.c +++ b/src/backend/archive/shell_archive.c @@ -71,9 +71,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))); @@ -132,8 +129,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 2b4e5a623c..251f75e91d 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -39,6 +39,7 @@ #include "postmaster/auxprocess.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" /* ---------- @@ -100,6 +103,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; /* @@ -257,6 +261,11 @@ PgArchiverMain(char *startup_data, size_t startup_data_len) ready_file_comparator, false, NULL); + /* Initialize our memory context. */ + archive_context = AllocSetContextCreate(TopMemoryContext, + "archiver", + ALLOCSET_DEFAULT_SIZES); + /* Load the archive_library. */ LoadArchiveLibrary(); @@ -507,6 +516,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; @@ -517,7 +528,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