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
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
