On Mon, Oct 17, 2022 at 11:17 AM Michael Paquier <mich...@paquier.xyz> wrote: > > On Mon, Oct 17, 2022 at 02:30:52PM +0900, Kyotaro Horiguchi wrote: > > Removing PG_ENSURE_ERROR_CLEANUP() and relying on before_shmem_exit() > is fine by me, that's what I imply upthread.
Hm. Here's a v2 patch that addresses review comments. In addition to making it a before_shmem_exit() callback, this patch also does the following things: 1) Renames call_archive_module_shutdown_callback() to be more meaningful and generic as before_shmem_exit() callback. 2) Clarifies when the archive module shutdown callback gets called in documentation. 3) Defines a shutdown callback that just emits a log message in shell_archive.c and tests it. Please review it further. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
From a0d5a3b8cfe1d52900027b97b8c1bf8fd9d04362 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Date: Tue, 18 Oct 2022 08:35:23 +0000 Subject: [PATCH v2] Fixes related to archive module shutdown callback Presently, the archive module shutdown callback can get called twice as it is first registered using PG_ENSURE_ERROR_CLEANUP and then called directly in HandlePgArchInterrupts() before proc_exit(). This patch fixes it by just registering it as before_shmem_exit(). In addition to the above, this patch also does the following things: 1) Renames call_archive_module_shutdown_callback() to be more meaningful and generic as before_shmem_exit() callback. 2) Clarifies when the archive module shutdown callback gets called in documentation. 3) Defines a shutdown callback that just emits a log message in shell_archive.c and tests it. Author: Nathan Bossart Author: Bharath Rupireddy Reviewed-by: Michael Paquier, Kyotaro Horiguchi Discussion: https://www.postgresql.org/message-id/20221015221328.GB1821022%40nathanxps13 Backpatch-through: 15 --- doc/src/sgml/archive-modules.sgml | 7 ++++--- doc/src/sgml/config.sgml | 5 +++-- src/backend/postmaster/pgarch.c | 24 +++++++---------------- src/backend/postmaster/shell_archive.c | 9 +++++++++ src/test/recovery/t/020_archive_status.pl | 19 ++++++++++++++++++ 5 files changed, 42 insertions(+), 22 deletions(-) diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml index ef02051f7f..677a29182a 100644 --- a/doc/src/sgml/archive-modules.sgml +++ b/doc/src/sgml/archive-modules.sgml @@ -121,9 +121,10 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path); <sect2 id="archive-module-shutdown"> <title>Shutdown Callback</title> <para> - The <function>shutdown_cb</function> callback is called when the archiver - process exits (e.g., after an error) or the value of - <xref linkend="guc-archive-library"/> changes. If no + The <function>shutdown_cb</function> callback is called before the server + begins to shut down low-level subsystems such as shared memory upon the + archiver process exit (e.g., after an error or the value of + <xref linkend="guc-archive-library"/> changes). If no <function>shutdown_cb</function> is defined, no special action is taken in these situations. diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 66312b53b8..22bbf05fef 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -3625,8 +3625,9 @@ include_dir 'conf.d' The library to use for archiving completed WAL file segments. If set to an empty string (the default), archiving via shell is enabled, and <xref linkend="guc-archive-command"/> is used. Otherwise, the specified - shared library is used for archiving. For more information, see - <xref linkend="backup-archiving-wal"/> and + shared library is used for archiving. The WAL archiver process gets + restarted by the postmaster whenever this parameter changes. For more + information, see <xref linkend="backup-archiving-wal"/> and <xref linkend="archive-modules"/>. </para> <para> diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index 3868cd7bd3..f1d1f808a8 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -144,7 +144,7 @@ static void pgarch_die(int code, Datum arg); static void HandlePgArchInterrupts(void); static int ready_file_comparator(Datum a, Datum b, void *arg); static void LoadArchiveLibrary(void); -static void call_archive_module_shutdown_callback(int code, Datum arg); +static void pgarch_before_shmem_exit(int code, Datum arg); /* Report shared memory space needed by PgArchShmemInit */ Size @@ -232,6 +232,8 @@ PgArchiverMain(void) /* We shouldn't be launched unnecessarily. */ Assert(XLogArchivingActive()); + before_shmem_exit(pgarch_before_shmem_exit, 0); + /* Arrange to clean up at archiver exit */ on_shmem_exit(pgarch_die, 0); @@ -252,13 +254,7 @@ PgArchiverMain(void) /* Load the archive_library. */ LoadArchiveLibrary(); - PG_ENSURE_ERROR_CLEANUP(call_archive_module_shutdown_callback, 0); - { - pgarch_MainLoop(); - } - PG_END_ENSURE_ERROR_CLEANUP(call_archive_module_shutdown_callback, 0); - - call_archive_module_shutdown_callback(0, 0); + pgarch_MainLoop(); proc_exit(0); } @@ -803,12 +799,6 @@ HandlePgArchInterrupts(void) if (archiveLibChanged) { - /* - * Call the currently loaded archive module's shutdown callback, - * if one is defined. - */ - call_archive_module_shutdown_callback(0, 0); - /* * Ideally, we would simply unload the previous archive module and * load the new one, but there is presently no mechanism for @@ -861,12 +851,12 @@ LoadArchiveLibrary(void) } /* - * call_archive_module_shutdown_callback + * Before shared memory exit callback for WAL archiver process. * - * Calls the loaded archive module's shutdown callback, if one is defined. + * It calls the loaded archive module's shutdown callback, if one is defined. */ static void -call_archive_module_shutdown_callback(int code, Datum arg) +pgarch_before_shmem_exit(int code, Datum arg) { if (ArchiveContext.shutdown_cb != NULL) ArchiveContext.shutdown_cb(); diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c index 8a54d02e7b..0a0ea0e6b5 100644 --- a/src/backend/postmaster/shell_archive.c +++ b/src/backend/postmaster/shell_archive.c @@ -23,6 +23,7 @@ static bool shell_archive_configured(void); static bool shell_archive_file(const char *file, const char *path); +static void shell_archive_shutdown(void); void shell_archive_init(ArchiveModuleCallbacks *cb) @@ -31,6 +32,7 @@ shell_archive_init(ArchiveModuleCallbacks *cb) cb->check_configured_cb = shell_archive_configured; cb->archive_file_cb = shell_archive_file; + cb->shutdown_cb = shell_archive_shutdown; } static bool @@ -156,3 +158,10 @@ shell_archive_file(const char *file, const char *path) elog(DEBUG1, "archived write-ahead log file \"%s\"", file); return true; } + +static void +shell_archive_shutdown(void) +{ + ereport(DEBUG1, + (errmsg_internal("archiver process shutting down"))); +} diff --git a/src/test/recovery/t/020_archive_status.pl b/src/test/recovery/t/020_archive_status.pl index 550fb37cb6..b2c6a127f2 100644 --- a/src/test/recovery/t/020_archive_status.pl +++ b/src/test/recovery/t/020_archive_status.pl @@ -9,6 +9,7 @@ use warnings; use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More; +use Time::HiRes qw(usleep); my $primary = PostgreSQL::Test::Cluster->new('primary'); $primary->init( @@ -234,4 +235,22 @@ ok( -f "$standby2_data/$segment_path_1_done" ".done files created after archive success with archive_mode=always on standby" ); +# Check that the archiver process calls the shell archive module's shutdown +# callback. +$standby2->append_conf('postgresql.conf', "log_min_messages = debug1"); +$standby2->reload; + +$standby2->stop(); + +# wait for postgres to terminate +foreach my $i (0 .. 10 * $PostgreSQL::Test::Utils::timeout_default) +{ + last if !-f $standby2->data_dir . '/postmaster.pid'; + usleep(100_000); +} + +my $logfile = slurp_file($standby2->logfile()); +ok($logfile =~ qr/archiver process shutting down/, + 'check shutdown callback of shell archive module'); + done_testing(); -- 2.34.1
