agora inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH 3/6] Add primary_slot_name to init_from_backup in TAP test.
33+ messages / 10 participants
[nested] [flat]

* [PATCH 3/6] Add primary_slot_name to init_from_backup in TAP test.
@ 2018-12-19 03:43  Kyotaro Horiguchi <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Kyotaro Horiguchi @ 2018-12-19 03:43 UTC (permalink / raw)

It is convenient that priary_slot_name can be specified on taking a
base backup. This adds a new parameter of the name to the perl
function.
---
 src/test/perl/PostgresNode.pm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 8a2c6fc122..daca2e0085 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -672,11 +672,11 @@ sub init_from_backup
 	chmod(0700, $data_path);
 
 	# Base configuration for this node
-	$self->append_conf(
-		'postgresql.conf',
-		qq(
-port = $port
-));
+	$self->append_conf('postgresql.conf', qq(port = $port));
+	$self->append_conf('postgresql.conf',
+					   qq(primary_slot_name = $params{primary_slot_name}))
+	  if (defined $params{primary_slot_name});
+
 	$self->enable_streaming($root_node) if $params{has_streaming};
 	$self->enable_restoring($root_node) if $params{has_restoring};
 	return;
-- 
2.16.3


----Next_Part(Thu_Dec_20_16_24_38_2018_792)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="v11-0004-TAP-test-for-the-slot-limit-feature.patch"



^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH 3/6] Add primary_slot_name to init_from_backup in TAP test.
@ 2018-12-19 03:43  Kyotaro Horiguchi <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Kyotaro Horiguchi @ 2018-12-19 03:43 UTC (permalink / raw)

It is convenient that priary_slot_name can be specified on taking a
base backup. This adds a new parameter of the name to the perl
function.
---
 src/test/perl/PostgresNode.pm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 8a2c6fc122..daca2e0085 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -672,11 +672,11 @@ sub init_from_backup
 	chmod(0700, $data_path);
 
 	# Base configuration for this node
-	$self->append_conf(
-		'postgresql.conf',
-		qq(
-port = $port
-));
+	$self->append_conf('postgresql.conf', qq(port = $port));
+	$self->append_conf('postgresql.conf',
+					   qq(primary_slot_name = $params{primary_slot_name}))
+	  if (defined $params{primary_slot_name});
+
 	$self->enable_streaming($root_node) if $params{has_streaming};
 	$self->enable_restoring($root_node) if $params{has_restoring};
 	return;
-- 
2.16.3


----Next_Part(Wed_Jan_30_10_42_04_2019_681)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="v12-0004-TAP-test-for-the-slot-limit-feature.patch"



^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Missing generated column in ALTER TABLE ADD COLUMN doc
@ 2019-06-07 09:07  Masahiko Sawada <[email protected]>
  0 siblings, 1 reply; 33+ messages in thread

From: Masahiko Sawada @ 2019-06-07 09:07 UTC (permalink / raw)
  To: pgsql-hackers

Hi,

We support ALTER TABLE ADD COLUMN .. GENERATED ALWAYS AS .. but the
doc is missing it. Attached small patch fixes this.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


Attachments:

  [application/octet-stream] doc_alter_column_add_generated_column.patch (776B, ../../CAD21AoAA_gvZ002U6kovOHu0FsM7ieoCzdSqWBd7_KaQL0UMKg@mail.gmail.com/2-doc_alter_column_add_generated_column.patch)
  download | inline diff:
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 49b081a..f5ad765 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -98,6 +98,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
   NULL |
   CHECK ( <replaceable class="parameter">expression</replaceable> ) [ NO INHERIT ] |
   DEFAULT <replaceable>default_expr</replaceable> |
+  GENERATED ALWAYS AS ( <replaceable>generation_expr</replaceable> ) STORED |
   GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( <replaceable>sequence_options</replaceable> ) ] |
   UNIQUE <replaceable class="parameter">index_parameters</replaceable> |
   PRIMARY KEY <replaceable class="parameter">index_parameters</replaceable> |


^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: Missing generated column in ALTER TABLE ADD COLUMN doc
@ 2019-06-10 08:05  Michael Paquier <[email protected]>
  parent: Masahiko Sawada <[email protected]>
  0 siblings, 1 reply; 33+ messages in thread

From: Michael Paquier @ 2019-06-10 08:05 UTC (permalink / raw)
  To: Masahiko Sawada <[email protected]>; +Cc: pgsql-hackers

On Fri, Jun 07, 2019 at 06:07:34PM +0900, Masahiko Sawada wrote:
> We support ALTER TABLE ADD COLUMN .. GENERATED ALWAYS AS .. but the
> doc is missing it. Attached small patch fixes this.

Your patch updates the section related to constraint_name.  Don't you
need an extra line for the "action" part?
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: Missing generated column in ALTER TABLE ADD COLUMN doc
@ 2019-06-10 09:09  Masahiko Sawada <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 33+ messages in thread

From: Masahiko Sawada @ 2019-06-10 09:09 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers

On Mon, Jun 10, 2019 at 5:05 PM Michael Paquier <[email protected]> wrote:
>
> On Fri, Jun 07, 2019 at 06:07:34PM +0900, Masahiko Sawada wrote:
> > We support ALTER TABLE ADD COLUMN .. GENERATED ALWAYS AS .. but the
> > doc is missing it. Attached small patch fixes this.
>
> Your patch updates the section related to constraint_name.  Don't you
> need an extra line for the "action" part?

We already have the following line in action part but you mean we need
an extra line for that?

   ADD [ COLUMN ] [ IF NOT EXISTS ] column_name data_type [ COLLATE
collation ] [ column_constraint [ ... ] ]

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: Missing generated column in ALTER TABLE ADD COLUMN doc
@ 2019-06-11 04:02  Michael Paquier <[email protected]>
  parent: Masahiko Sawada <[email protected]>
  0 siblings, 1 reply; 33+ messages in thread

From: Michael Paquier @ 2019-06-11 04:02 UTC (permalink / raw)
  To: Masahiko Sawada <[email protected]>; +Cc: pgsql-hackers

On Mon, Jun 10, 2019 at 06:09:53PM +0900, Masahiko Sawada wrote:
> We already have the following line in action part but you mean we need
> an extra line for that?
> 
>    ADD [ COLUMN ] [ IF NOT EXISTS ] column_name data_type [ COLLATE
> collation ] [ column_constraint [ ... ] ]

I was looking at the grammar extensions for ADD GENERATED and noticed
what looked like inconsistencies, but your patch as well as the parsed
query are right.  Committed, thanks!
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: Missing generated column in ALTER TABLE ADD COLUMN doc
@ 2019-06-11 04:49  Masahiko Sawada <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Masahiko Sawada @ 2019-06-11 04:49 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers

On Tue, Jun 11, 2019 at 1:02 PM Michael Paquier <[email protected]> wrote:
>
> On Mon, Jun 10, 2019 at 06:09:53PM +0900, Masahiko Sawada wrote:
> > We already have the following line in action part but you mean we need
> > an extra line for that?
> >
> >    ADD [ COLUMN ] [ IF NOT EXISTS ] column_name data_type [ COLLATE
> > collation ] [ column_constraint [ ... ] ]
>
> I was looking at the grammar extensions for ADD GENERATED and noticed
> what looked like inconsistencies, but your patch as well as the parsed
> query are right.  Committed, thanks!

Thank you!

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v6 3/3] restructure archive modules API
@ 2023-01-31 22:58  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-01-31 22:58 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c   | 53 +++++++++++++++++++------
 doc/src/sgml/archive-modules.sgml       | 35 ++++++++++++----
 src/backend/postmaster/pgarch.c         | 23 ++++++-----
 src/backend/postmaster/shell_archive.c  | 29 ++++++++------
 src/include/postmaster/archive_module.h | 21 +++++++---
 src/include/postmaster/shell_archive.h  |  2 +-
 6 files changed, 115 insertions(+), 48 deletions(-)

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 87bbb2174d..74a51b9288 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -40,15 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+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 const ArchiveModuleCallbacks basic_archive_callbacks = {
+	.startup_cb = basic_archive_startup,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = NULL
+};
+
 /*
  * _PG_init
  *
@@ -67,10 +79,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,13 +86,30 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
-void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
 {
 	AssertVariableIsOfType(&_PG_archive_module_init, ArchiveModuleInit);
 
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	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;
 }
 
 /*
@@ -135,7 +160,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -146,10 +171,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+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
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..2db1b19216 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,23 +47,30 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
-   individual actions.
+   <function>_PG_archive_module_init</function>.  This function must return an
+   <structname>ArchiveModuleCallbacks</structname> struct filled with the
+   callback function pointers for individual actions.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
    others are optional.
   </para>
+
+  <note>
+   <para>
+    <varname>archive_library</varname> is only loaded in the archiver process.
+   </para>
+  </note>
  </sect1>
 
  <sect1 id="archive-module-callbacks">
@@ -73,6 +80,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module needs to have a state, it
+    can use <structfield>state->private_data</structfield> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +104,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +126,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -128,7 +149,7 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     these situations.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 281d9fd8b7..99a07996f7 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -99,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveCallbacks;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -408,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveCallbacks.check_configured_cb != NULL &&
-				!ArchiveCallbacks.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -510,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveCallbacks.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -829,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveCallbacks, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -846,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveCallbacks);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveCallbacks.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -861,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveCallbacks.shutdown_cb != NULL)
-		ArchiveCallbacks.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 35f88c1222..cc2585218d 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -23,28 +23,33 @@
 #include "postmaster/archive_module.h"
 #include "postmaster/shell_archive.h"
 
-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)
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static void shell_archive_shutdown(ArchiveModuleState *state);
+
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
 	AssertVariableIsOfType(&shell_archive_init, ArchiveModuleInit);
 
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(ArchiveModuleState *state, const char *file, const char *path)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -126,7 +131,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/include/postmaster/archive_module.h b/src/include/postmaster/archive_module.h
index f5015ff95d..79e9d53534 100644
--- a/src/include/postmaster/archive_module.h
+++ b/src/include/postmaster/archive_module.h
@@ -17,6 +17,15 @@
  */
 extern PGDLLIMPORT char *XLogArchiveLibrary;
 
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
 /*
  * Archive module callbacks
  *
@@ -25,12 +34,14 @@ extern PGDLLIMPORT char *XLogArchiveLibrary;
  * For more information about the purpose of each callback, refer to the
  * archive modules documentation.
  */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 
 typedef struct ArchiveModuleCallbacks
 {
+	ArchiveStartupCB startup_cb;
 	ArchiveCheckConfiguredCB check_configured_cb;
 	ArchiveFileCB archive_file_cb;
 	ArchiveShutdownCB shutdown_cb;
@@ -40,8 +51,8 @@ typedef struct ArchiveModuleCallbacks
  * Type of the shared library symbol _PG_archive_module_init that is looked
  * up when loading an archive library.
  */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
 
 #endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/postmaster/shell_archive.h b/src/include/postmaster/shell_archive.h
index 3cf737ce8e..be83c7715a 100644
--- a/src/include/postmaster/shell_archive.h
+++ b/src/include/postmaster/shell_archive.h
@@ -19,6 +19,6 @@
  * and does not need to be loaded via a shared library, it has a special
  * initialization function.
  */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
 
 #endif							/* _SHELL_ARCHIVE_H */
-- 
2.25.1


--OXfL5xGRrasGEqWY--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v3 3/3] restructure archive modules API
@ 2023-01-31 22:58  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-01-31 22:58 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c   | 51 ++++++++++++++++++-------
 doc/src/sgml/archive-modules.sgml       | 33 +++++++++++++---
 src/backend/postmaster/pgarch.c         | 23 ++++++-----
 src/backend/postmaster/shell_archive.c  | 29 ++++++++------
 src/include/postmaster/archive_module.h | 21 +++++++---
 src/include/postmaster/shell_archive.h  |  2 +-
 6 files changed, 112 insertions(+), 47 deletions(-)

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 87bbb2174d..f36a1def15 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -40,15 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+static void basic_archive_startup(ArchiveModuleState *state);
+static bool basic_archive_configured(ArchiveModuleState *state);
+static bool basic_archive_file(const char *file, const char *path, ArchiveModuleState *state);
 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 const ArchiveModuleCallbacks basic_archive_callbacks = {
+	.startup_cb = basic_archive_startup,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = NULL
+};
+
 /*
  * _PG_init
  *
@@ -67,10 +79,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,13 +86,28 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
-void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
 {
 	AssertVariableIsOfType(&_PG_archive_module_init, ArchiveModuleInit);
 
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	return &basic_archive_callbacks;
+}
+
+/*
+ * basic_archive_startup
+ *
+ * Creates the module's memory context.
+ */
+void
+basic_archive_startup(ArchiveModuleState *state)
+{
+	BasicArchiveData *data = palloc0(sizeof(BasicArchiveData));
+
+	data->context = AllocSetContextCreate(TopMemoryContext,
+										  "basic_archive",
+										  ALLOCSET_DEFAULT_SIZES);
+	state->private_data = (void *) data;
 }
 
 /*
@@ -135,7 +158,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -146,10 +169,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+basic_archive_file(const char *file, const char *path, ArchiveModuleState *state)
 {
 	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
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..cc8a834a2e 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,23 +47,30 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
+   <function>_PG_archive_module_init</function>.  This function must return an
+   ArchiveModuleCallbacks struct filled with the callback function pointers for
    individual actions.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
    others are optional.
   </para>
+
+  <note>
+   <para>
+    <varname>archive_library</varname> is only loaded in the archiver process.
+   </para>
+  </note>
  </sect1>
 
  <sect1 id="archive-module-callbacks">
@@ -73,6 +80,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module needs to have a state, it
+    can use <literal>state->private_data</literal> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +104,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +126,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (const char *file, const char *path, ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -128,7 +149,7 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     these situations.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index ec47b2cc20..253d456837 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -99,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveCallbacks;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -408,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveCallbacks.check_configured_cb != NULL &&
-				!ArchiveCallbacks.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -510,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveCallbacks.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(xlog, pathname, archive_module_state);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -829,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveCallbacks, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -846,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveCallbacks);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveCallbacks.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -861,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveCallbacks.shutdown_cb != NULL)
-		ArchiveCallbacks.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 35f88c1222..5eda7671e8 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -23,28 +23,33 @@
 #include "postmaster/archive_module.h"
 #include "postmaster/shell_archive.h"
 
-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)
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(const char *file, const char *path, ArchiveModuleState *state);
+static void shell_archive_shutdown(ArchiveModuleState *state);
+
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
 	AssertVariableIsOfType(&shell_archive_init, ArchiveModuleInit);
 
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(const char *file, const char *path, ArchiveModuleState *state)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -126,7 +131,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/include/postmaster/archive_module.h b/src/include/postmaster/archive_module.h
index f5015ff95d..d07dd26c6e 100644
--- a/src/include/postmaster/archive_module.h
+++ b/src/include/postmaster/archive_module.h
@@ -17,6 +17,15 @@
  */
 extern PGDLLIMPORT char *XLogArchiveLibrary;
 
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
 /*
  * Archive module callbacks
  *
@@ -25,12 +34,14 @@ extern PGDLLIMPORT char *XLogArchiveLibrary;
  * For more information about the purpose of each callback, refer to the
  * archive modules documentation.
  */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (const char *file, const char *path, ArchiveModuleState *state);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 
 typedef struct ArchiveModuleCallbacks
 {
+	ArchiveStartupCB startup_cb;
 	ArchiveCheckConfiguredCB check_configured_cb;
 	ArchiveFileCB archive_file_cb;
 	ArchiveShutdownCB shutdown_cb;
@@ -40,8 +51,8 @@ typedef struct ArchiveModuleCallbacks
  * Type of the shared library symbol _PG_archive_module_init that is looked
  * up when loading an archive library.
  */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
 
 #endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/postmaster/shell_archive.h b/src/include/postmaster/shell_archive.h
index f6943c0aba..66b2fda401 100644
--- a/src/include/postmaster/shell_archive.h
+++ b/src/include/postmaster/shell_archive.h
@@ -17,6 +17,6 @@
  * and does not need to be loaded via a shared library, it has a special
  * initialization function.
  */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
 
 #endif							/* _SHELL_ARCHIVE_H */
-- 
2.25.1


--sm4nu43k4a2Rpi4c--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v4 3/3] restructure archive modules API
@ 2023-01-31 22:58  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-01-31 22:58 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c   | 51 ++++++++++++++++++-------
 doc/src/sgml/archive-modules.sgml       | 33 +++++++++++++---
 src/backend/postmaster/pgarch.c         | 23 ++++++-----
 src/backend/postmaster/shell_archive.c  | 29 ++++++++------
 src/include/postmaster/archive_module.h | 21 +++++++---
 src/include/postmaster/shell_archive.h  |  2 +-
 6 files changed, 112 insertions(+), 47 deletions(-)

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 87bbb2174d..3b4373b562 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -40,15 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+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 const ArchiveModuleCallbacks basic_archive_callbacks = {
+	.startup_cb = basic_archive_startup,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = NULL
+};
+
 /*
  * _PG_init
  *
@@ -67,10 +79,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,13 +86,28 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
-void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
 {
 	AssertVariableIsOfType(&_PG_archive_module_init, ArchiveModuleInit);
 
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	return &basic_archive_callbacks;
+}
+
+/*
+ * basic_archive_startup
+ *
+ * Creates the module's memory context.
+ */
+void
+basic_archive_startup(ArchiveModuleState *state)
+{
+	BasicArchiveData *data = palloc0(sizeof(BasicArchiveData));
+
+	data->context = AllocSetContextCreate(TopMemoryContext,
+										  "basic_archive",
+										  ALLOCSET_DEFAULT_SIZES);
+	state->private_data = (void *) data;
 }
 
 /*
@@ -135,7 +158,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -146,10 +169,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+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
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..2ec5d12d97 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,23 +47,30 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
+   <function>_PG_archive_module_init</function>.  This function must return an
+   ArchiveModuleCallbacks struct filled with the callback function pointers for
    individual actions.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
    others are optional.
   </para>
+
+  <note>
+   <para>
+    <varname>archive_library</varname> is only loaded in the archiver process.
+   </para>
+  </note>
  </sect1>
 
  <sect1 id="archive-module-callbacks">
@@ -73,6 +80,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module needs to have a state, it
+    can use <literal>state->private_data</literal> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +104,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +126,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -128,7 +149,7 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     these situations.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index ec47b2cc20..5931d5ffa0 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -99,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveCallbacks;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -408,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveCallbacks.check_configured_cb != NULL &&
-				!ArchiveCallbacks.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -510,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveCallbacks.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -829,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveCallbacks, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -846,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveCallbacks);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveCallbacks.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -861,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveCallbacks.shutdown_cb != NULL)
-		ArchiveCallbacks.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 35f88c1222..cc2585218d 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -23,28 +23,33 @@
 #include "postmaster/archive_module.h"
 #include "postmaster/shell_archive.h"
 
-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)
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static void shell_archive_shutdown(ArchiveModuleState *state);
+
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
 	AssertVariableIsOfType(&shell_archive_init, ArchiveModuleInit);
 
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(ArchiveModuleState *state, const char *file, const char *path)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -126,7 +131,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/include/postmaster/archive_module.h b/src/include/postmaster/archive_module.h
index f5015ff95d..79e9d53534 100644
--- a/src/include/postmaster/archive_module.h
+++ b/src/include/postmaster/archive_module.h
@@ -17,6 +17,15 @@
  */
 extern PGDLLIMPORT char *XLogArchiveLibrary;
 
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
 /*
  * Archive module callbacks
  *
@@ -25,12 +34,14 @@ extern PGDLLIMPORT char *XLogArchiveLibrary;
  * For more information about the purpose of each callback, refer to the
  * archive modules documentation.
  */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 
 typedef struct ArchiveModuleCallbacks
 {
+	ArchiveStartupCB startup_cb;
 	ArchiveCheckConfiguredCB check_configured_cb;
 	ArchiveFileCB archive_file_cb;
 	ArchiveShutdownCB shutdown_cb;
@@ -40,8 +51,8 @@ typedef struct ArchiveModuleCallbacks
  * Type of the shared library symbol _PG_archive_module_init that is looked
  * up when loading an archive library.
  */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
 
 #endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/postmaster/shell_archive.h b/src/include/postmaster/shell_archive.h
index 3cf737ce8e..be83c7715a 100644
--- a/src/include/postmaster/shell_archive.h
+++ b/src/include/postmaster/shell_archive.h
@@ -19,6 +19,6 @@
  * and does not need to be loaded via a shared library, it has a special
  * initialization function.
  */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
 
 #endif							/* _SHELL_ARCHIVE_H */
-- 
2.25.1


--9amGYk9869ThD9tj--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v5 3/3] restructure archive modules API
@ 2023-01-31 22:58  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-01-31 22:58 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c   | 53 +++++++++++++++++++------
 doc/src/sgml/archive-modules.sgml       | 33 ++++++++++++---
 src/backend/postmaster/pgarch.c         | 23 ++++++-----
 src/backend/postmaster/shell_archive.c  | 29 ++++++++------
 src/include/postmaster/archive_module.h | 21 +++++++---
 src/include/postmaster/shell_archive.h  |  2 +-
 6 files changed, 114 insertions(+), 47 deletions(-)

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 87bbb2174d..74a51b9288 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -40,15 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+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 const ArchiveModuleCallbacks basic_archive_callbacks = {
+	.startup_cb = basic_archive_startup,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = NULL
+};
+
 /*
  * _PG_init
  *
@@ -67,10 +79,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,13 +86,30 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
-void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
 {
 	AssertVariableIsOfType(&_PG_archive_module_init, ArchiveModuleInit);
 
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	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;
 }
 
 /*
@@ -135,7 +160,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -146,10 +171,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+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
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..2ec5d12d97 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,23 +47,30 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
+   <function>_PG_archive_module_init</function>.  This function must return an
+   ArchiveModuleCallbacks struct filled with the callback function pointers for
    individual actions.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
    others are optional.
   </para>
+
+  <note>
+   <para>
+    <varname>archive_library</varname> is only loaded in the archiver process.
+   </para>
+  </note>
  </sect1>
 
  <sect1 id="archive-module-callbacks">
@@ -73,6 +80,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module needs to have a state, it
+    can use <literal>state->private_data</literal> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +104,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +126,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -128,7 +149,7 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     these situations.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index ec47b2cc20..5931d5ffa0 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -99,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveCallbacks;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -408,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveCallbacks.check_configured_cb != NULL &&
-				!ArchiveCallbacks.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -510,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveCallbacks.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -829,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveCallbacks, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -846,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveCallbacks);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveCallbacks.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -861,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveCallbacks.shutdown_cb != NULL)
-		ArchiveCallbacks.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 35f88c1222..cc2585218d 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -23,28 +23,33 @@
 #include "postmaster/archive_module.h"
 #include "postmaster/shell_archive.h"
 
-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)
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static void shell_archive_shutdown(ArchiveModuleState *state);
+
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
 	AssertVariableIsOfType(&shell_archive_init, ArchiveModuleInit);
 
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(ArchiveModuleState *state, const char *file, const char *path)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -126,7 +131,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/include/postmaster/archive_module.h b/src/include/postmaster/archive_module.h
index f5015ff95d..79e9d53534 100644
--- a/src/include/postmaster/archive_module.h
+++ b/src/include/postmaster/archive_module.h
@@ -17,6 +17,15 @@
  */
 extern PGDLLIMPORT char *XLogArchiveLibrary;
 
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
 /*
  * Archive module callbacks
  *
@@ -25,12 +34,14 @@ extern PGDLLIMPORT char *XLogArchiveLibrary;
  * For more information about the purpose of each callback, refer to the
  * archive modules documentation.
  */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 
 typedef struct ArchiveModuleCallbacks
 {
+	ArchiveStartupCB startup_cb;
 	ArchiveCheckConfiguredCB check_configured_cb;
 	ArchiveFileCB archive_file_cb;
 	ArchiveShutdownCB shutdown_cb;
@@ -40,8 +51,8 @@ typedef struct ArchiveModuleCallbacks
  * Type of the shared library symbol _PG_archive_module_init that is looked
  * up when loading an archive library.
  */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
 
 #endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/postmaster/shell_archive.h b/src/include/postmaster/shell_archive.h
index 3cf737ce8e..be83c7715a 100644
--- a/src/include/postmaster/shell_archive.h
+++ b/src/include/postmaster/shell_archive.h
@@ -19,6 +19,6 @@
  * and does not need to be loaded via a shared library, it has a special
  * initialization function.
  */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
 
 #endif							/* _SHELL_ARCHIVE_H */
-- 
2.25.1


--+QahgC5+KEYLbs62--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v7 3/4] restructure archive modules API
@ 2023-01-31 22:58  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-01-31 22:58 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c   | 53 +++++++++++++++++++------
 doc/src/sgml/archive-modules.sgml       | 35 ++++++++++++----
 src/backend/postmaster/pgarch.c         | 23 ++++++-----
 src/backend/postmaster/shell_archive.c  | 29 ++++++++------
 src/include/postmaster/archive_module.h | 21 +++++++---
 src/include/postmaster/shell_archive.h  |  2 +-
 6 files changed, 115 insertions(+), 48 deletions(-)

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 87bbb2174d..74a51b9288 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -40,15 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+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 const ArchiveModuleCallbacks basic_archive_callbacks = {
+	.startup_cb = basic_archive_startup,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = NULL
+};
+
 /*
  * _PG_init
  *
@@ -67,10 +79,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,13 +86,30 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
-void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
 {
 	AssertVariableIsOfType(&_PG_archive_module_init, ArchiveModuleInit);
 
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	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;
 }
 
 /*
@@ -135,7 +160,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -146,10 +171,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+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
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..2db1b19216 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,23 +47,30 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
-   individual actions.
+   <function>_PG_archive_module_init</function>.  This function must return an
+   <structname>ArchiveModuleCallbacks</structname> struct filled with the
+   callback function pointers for individual actions.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
    others are optional.
   </para>
+
+  <note>
+   <para>
+    <varname>archive_library</varname> is only loaded in the archiver process.
+   </para>
+  </note>
  </sect1>
 
  <sect1 id="archive-module-callbacks">
@@ -73,6 +80,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module needs to have a state, it
+    can use <structfield>state->private_data</structfield> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +104,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +126,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -128,7 +149,7 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     these situations.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 281d9fd8b7..99a07996f7 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -99,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveCallbacks;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -408,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveCallbacks.check_configured_cb != NULL &&
-				!ArchiveCallbacks.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -510,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveCallbacks.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -829,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveCallbacks, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -846,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveCallbacks);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveCallbacks.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -861,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveCallbacks.shutdown_cb != NULL)
-		ArchiveCallbacks.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 35f88c1222..cc2585218d 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -23,28 +23,33 @@
 #include "postmaster/archive_module.h"
 #include "postmaster/shell_archive.h"
 
-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)
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static void shell_archive_shutdown(ArchiveModuleState *state);
+
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
 	AssertVariableIsOfType(&shell_archive_init, ArchiveModuleInit);
 
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(ArchiveModuleState *state, const char *file, const char *path)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -126,7 +131,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/include/postmaster/archive_module.h b/src/include/postmaster/archive_module.h
index f5015ff95d..79e9d53534 100644
--- a/src/include/postmaster/archive_module.h
+++ b/src/include/postmaster/archive_module.h
@@ -17,6 +17,15 @@
  */
 extern PGDLLIMPORT char *XLogArchiveLibrary;
 
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
 /*
  * Archive module callbacks
  *
@@ -25,12 +34,14 @@ extern PGDLLIMPORT char *XLogArchiveLibrary;
  * For more information about the purpose of each callback, refer to the
  * archive modules documentation.
  */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 
 typedef struct ArchiveModuleCallbacks
 {
+	ArchiveStartupCB startup_cb;
 	ArchiveCheckConfiguredCB check_configured_cb;
 	ArchiveFileCB archive_file_cb;
 	ArchiveShutdownCB shutdown_cb;
@@ -40,8 +51,8 @@ typedef struct ArchiveModuleCallbacks
  * Type of the shared library symbol _PG_archive_module_init that is looked
  * up when loading an archive library.
  */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
 
 #endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/postmaster/shell_archive.h b/src/include/postmaster/shell_archive.h
index 3cf737ce8e..be83c7715a 100644
--- a/src/include/postmaster/shell_archive.h
+++ b/src/include/postmaster/shell_archive.h
@@ -19,6 +19,6 @@
  * and does not need to be loaded via a shared library, it has a special
  * initialization function.
  */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
 
 #endif							/* _SHELL_ARCHIVE_H */
-- 
2.25.1


--/04w6evG8XlLl3ft
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v7-0004-remove-unnecessary-assertions-for-functions-with-.patch"



^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v8 4/4] restructure archive modules API
@ 2023-01-31 22:58  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-01-31 22:58 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c   | 51 +++++++++++++++++++------
 doc/src/sgml/archive-modules.sgml       | 35 +++++++++++++----
 src/backend/postmaster/pgarch.c         | 23 ++++++-----
 src/backend/postmaster/shell_archive.c  | 29 ++++++++------
 src/include/postmaster/archive_module.h | 21 +++++++---
 src/include/postmaster/shell_archive.h  |  2 +-
 6 files changed, 114 insertions(+), 47 deletions(-)

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 384336884d..9afa03c5e2 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -40,15 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+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 const ArchiveModuleCallbacks basic_archive_callbacks = {
+	.startup_cb = basic_archive_startup,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = NULL
+};
+
 /*
  * _PG_init
  *
@@ -67,10 +79,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,11 +86,28 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
+{
+	return &basic_archive_callbacks;
+}
+
+/*
+ * basic_archive_startup
+ *
+ * Creates the module's memory context.
+ */
 void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+basic_archive_startup(ArchiveModuleState *state)
 {
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	BasicArchiveData *data;
+
+	data = (BasicArchiveData *) MemoryContextAllocZero(TopMemoryContext,
+													   sizeof(BasicArchiveData));
+	data->context = AllocSetContextCreate(TopMemoryContext,
+										  "basic_archive",
+										  ALLOCSET_DEFAULT_SIZES);
+	state->private_data = (void *) data;
 }
 
 /*
@@ -133,7 +158,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -144,10 +169,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+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
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..2db1b19216 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,23 +47,30 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
-   individual actions.
+   <function>_PG_archive_module_init</function>.  This function must return an
+   <structname>ArchiveModuleCallbacks</structname> struct filled with the
+   callback function pointers for individual actions.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
    others are optional.
   </para>
+
+  <note>
+   <para>
+    <varname>archive_library</varname> is only loaded in the archiver process.
+   </para>
+  </note>
  </sect1>
 
  <sect1 id="archive-module-callbacks">
@@ -73,6 +80,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module needs to have a state, it
+    can use <structfield>state->private_data</structfield> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +104,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +126,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -128,7 +149,7 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     these situations.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 281d9fd8b7..99a07996f7 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -99,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveCallbacks;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -408,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveCallbacks.check_configured_cb != NULL &&
-				!ArchiveCallbacks.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -510,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveCallbacks.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -829,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveCallbacks, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -846,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveCallbacks);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveCallbacks.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -861,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveCallbacks.shutdown_cb != NULL)
-		ArchiveCallbacks.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 0f0558e155..54b02ca152 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -23,26 +23,31 @@
 #include "postmaster/archive_module.h"
 #include "postmaster/shell_archive.h"
 
-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)
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static void shell_archive_shutdown(ArchiveModuleState *state);
+
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(ArchiveModuleState *state, const char *file, const char *path)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -124,7 +129,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/include/postmaster/archive_module.h b/src/include/postmaster/archive_module.h
index f5015ff95d..79e9d53534 100644
--- a/src/include/postmaster/archive_module.h
+++ b/src/include/postmaster/archive_module.h
@@ -17,6 +17,15 @@
  */
 extern PGDLLIMPORT char *XLogArchiveLibrary;
 
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
 /*
  * Archive module callbacks
  *
@@ -25,12 +34,14 @@ extern PGDLLIMPORT char *XLogArchiveLibrary;
  * For more information about the purpose of each callback, refer to the
  * archive modules documentation.
  */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 
 typedef struct ArchiveModuleCallbacks
 {
+	ArchiveStartupCB startup_cb;
 	ArchiveCheckConfiguredCB check_configured_cb;
 	ArchiveFileCB archive_file_cb;
 	ArchiveShutdownCB shutdown_cb;
@@ -40,8 +51,8 @@ typedef struct ArchiveModuleCallbacks
  * Type of the shared library symbol _PG_archive_module_init that is looked
  * up when loading an archive library.
  */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
 
 #endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/postmaster/shell_archive.h b/src/include/postmaster/shell_archive.h
index 3cf737ce8e..be83c7715a 100644
--- a/src/include/postmaster/shell_archive.h
+++ b/src/include/postmaster/shell_archive.h
@@ -19,6 +19,6 @@
  * and does not need to be loaded via a shared library, it has a special
  * initialization function.
  */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
 
 #endif							/* _SHELL_ARCHIVE_H */
-- 
2.25.1


--4Ckj6UjgE2iN1+kY--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v9 3/3] restructure archive modules API
@ 2023-01-31 22:58  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-01-31 22:58 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c   | 51 +++++++++++++++++++------
 doc/src/sgml/archive-modules.sgml       | 35 +++++++++++++----
 src/backend/postmaster/pgarch.c         | 23 ++++++-----
 src/backend/postmaster/shell_archive.c  | 29 ++++++++------
 src/include/postmaster/archive_module.h | 21 +++++++---
 src/include/postmaster/shell_archive.h  |  2 +-
 6 files changed, 114 insertions(+), 47 deletions(-)

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 384336884d..9afa03c5e2 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -40,15 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+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 const ArchiveModuleCallbacks basic_archive_callbacks = {
+	.startup_cb = basic_archive_startup,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = NULL
+};
+
 /*
  * _PG_init
  *
@@ -67,10 +79,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,11 +86,28 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
+{
+	return &basic_archive_callbacks;
+}
+
+/*
+ * basic_archive_startup
+ *
+ * Creates the module's memory context.
+ */
 void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+basic_archive_startup(ArchiveModuleState *state)
 {
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	BasicArchiveData *data;
+
+	data = (BasicArchiveData *) MemoryContextAllocZero(TopMemoryContext,
+													   sizeof(BasicArchiveData));
+	data->context = AllocSetContextCreate(TopMemoryContext,
+										  "basic_archive",
+										  ALLOCSET_DEFAULT_SIZES);
+	state->private_data = (void *) data;
 }
 
 /*
@@ -133,7 +158,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -144,10 +169,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+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
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..2db1b19216 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,23 +47,30 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
-   individual actions.
+   <function>_PG_archive_module_init</function>.  This function must return an
+   <structname>ArchiveModuleCallbacks</structname> struct filled with the
+   callback function pointers for individual actions.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
    others are optional.
   </para>
+
+  <note>
+   <para>
+    <varname>archive_library</varname> is only loaded in the archiver process.
+   </para>
+  </note>
  </sect1>
 
  <sect1 id="archive-module-callbacks">
@@ -73,6 +80,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module needs to have a state, it
+    can use <structfield>state->private_data</structfield> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +104,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +126,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -128,7 +149,7 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     these situations.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 281d9fd8b7..99a07996f7 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -99,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveCallbacks;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -408,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveCallbacks.check_configured_cb != NULL &&
-				!ArchiveCallbacks.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -510,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveCallbacks.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -829,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveCallbacks, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -846,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveCallbacks);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveCallbacks.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -861,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveCallbacks.shutdown_cb != NULL)
-		ArchiveCallbacks.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 0f0558e155..54b02ca152 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -23,26 +23,31 @@
 #include "postmaster/archive_module.h"
 #include "postmaster/shell_archive.h"
 
-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)
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static void shell_archive_shutdown(ArchiveModuleState *state);
+
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(ArchiveModuleState *state, const char *file, const char *path)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -124,7 +129,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/include/postmaster/archive_module.h b/src/include/postmaster/archive_module.h
index f5015ff95d..79e9d53534 100644
--- a/src/include/postmaster/archive_module.h
+++ b/src/include/postmaster/archive_module.h
@@ -17,6 +17,15 @@
  */
 extern PGDLLIMPORT char *XLogArchiveLibrary;
 
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
 /*
  * Archive module callbacks
  *
@@ -25,12 +34,14 @@ extern PGDLLIMPORT char *XLogArchiveLibrary;
  * For more information about the purpose of each callback, refer to the
  * archive modules documentation.
  */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 
 typedef struct ArchiveModuleCallbacks
 {
+	ArchiveStartupCB startup_cb;
 	ArchiveCheckConfiguredCB check_configured_cb;
 	ArchiveFileCB archive_file_cb;
 	ArchiveShutdownCB shutdown_cb;
@@ -40,8 +51,8 @@ typedef struct ArchiveModuleCallbacks
  * Type of the shared library symbol _PG_archive_module_init that is looked
  * up when loading an archive library.
  */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
 
 #endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/postmaster/shell_archive.h b/src/include/postmaster/shell_archive.h
index 3cf737ce8e..be83c7715a 100644
--- a/src/include/postmaster/shell_archive.h
+++ b/src/include/postmaster/shell_archive.h
@@ -19,6 +19,6 @@
  * and does not need to be loaded via a shared library, it has a special
  * initialization function.
  */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
 
 #endif							/* _SHELL_ARCHIVE_H */
-- 
2.25.1


--2oS5YaxWCcQjTEyO--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v2 3/3] restructure archive modules API
@ 2023-01-31 22:58  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-01-31 22:58 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c   | 42 +++++++++++++++++--------
 doc/src/sgml/archive-modules.sgml       | 30 +++++++++++++-----
 src/backend/postmaster/pgarch.c         | 22 +++++++------
 src/backend/postmaster/shell_archive.c  | 29 ++++++++++-------
 src/include/postmaster/archive_module.h | 14 +++++----
 5 files changed, 89 insertions(+), 48 deletions(-)

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 87bbb2174d..2b7680947f 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -41,14 +41,21 @@
 PG_MODULE_MAGIC;
 
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+static void *basic_archive_startup(void);
+static bool basic_archive_configured(void *private_state);
+static bool basic_archive_file(const char *file, const char *path, void *private_state);
 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 const ArchiveModuleCallbacks basic_archive_callbacks = {
+	.startup_cb = basic_archive_startup,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = NULL
+};
+
 /*
  * _PG_init
  *
@@ -67,10 +74,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,13 +81,25 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
-void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
 {
 	AssertVariableIsOfType(&_PG_archive_module_init, ArchiveModuleInit);
 
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	return &basic_archive_callbacks;
+}
+
+/*
+ * basic_archive_startup
+ *
+ * Creates the module's memory context.
+ */
+void *
+basic_archive_startup(void)
+{
+	return (void *) AllocSetContextCreate(TopMemoryContext,
+										  "basic_archive",
+										  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -135,7 +150,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(void *private_state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -146,10 +161,11 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+basic_archive_file(const char *file, const char *path, void *private_state)
 {
 	sigjmp_buf	local_sigjmp_buf;
 	MemoryContext oldcontext;
+	MemoryContext basic_archive_context = (MemoryContext) private_state;
 
 	/*
 	 * We run basic_archive_file_internal() in our own memory context so that
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..8c4a3d9365 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,18 +47,18 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
-   individual actions.
+   <function>_PG_archive_module_init</function>.  This function must return a
+   struct filled with the callback function pointers for individual actions.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
@@ -73,6 +73,22 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module needs a state, it should
+    return a pointer to the state.  This pointer will be passed to each of the
+    module's other callbacks via the <literal>void *private_state</literal>
+    argument.
+
+<programlisting>
+typedef void *(*ArchiveStartupCB) (void);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +99,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (void *private_state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +121,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (const char *file, const char *path, void *private_state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -128,7 +144,7 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     these situations.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (void *private_state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index eca02d5e74..421425d0dd 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -98,7 +98,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveCallbacks;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static void *private_state = NULL;
 
 
 /*
@@ -417,8 +418,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveCallbacks.check_configured_cb != NULL &&
-				!ArchiveCallbacks.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(private_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -519,7 +520,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveCallbacks.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(xlog, pathname, private_state);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -838,8 +839,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveCallbacks, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -855,12 +854,15 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveCallbacks);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveCallbacks.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	if (ArchiveCallbacks->startup_cb != NULL)
+		private_state = ArchiveCallbacks->startup_cb();
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -870,6 +872,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveCallbacks.shutdown_cb != NULL)
-		ArchiveCallbacks.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(private_state);
 }
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index b64297e3bb..dbd633477a 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -22,28 +22,33 @@
 #include "pgstat.h"
 #include "postmaster/archive_module.h"
 
-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)
+static bool shell_archive_configured(void *private_state);
+static bool shell_archive_file(const char *file, const char *path, void *private_state);
+static void shell_archive_shutdown(void *private_state);
+
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
 	AssertVariableIsOfType(&shell_archive_init, ArchiveModuleInit);
 
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(void *private_state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(const char *file, const char *path, void *private_state)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -125,7 +130,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(void *private_state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/include/postmaster/archive_module.h b/src/include/postmaster/archive_module.h
index 099050c1ca..b4ea4f5dcd 100644
--- a/src/include/postmaster/archive_module.h
+++ b/src/include/postmaster/archive_module.h
@@ -25,12 +25,14 @@ extern PGDLLIMPORT char *XLogArchiveLibrary;
  * For more information about the purpose of each callback, refer to the
  * archive modules documentation.
  */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
+typedef void *(*ArchiveStartupCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (void *private_state);
+typedef bool (*ArchiveFileCB) (const char *file, const char *path, void *private_state);
+typedef void (*ArchiveShutdownCB) (void *private_state);
 
 typedef struct ArchiveModuleCallbacks
 {
+	ArchiveStartupCB startup_cb;
 	ArchiveCheckConfiguredCB check_configured_cb;
 	ArchiveFileCB archive_file_cb;
 	ArchiveShutdownCB shutdown_cb;
@@ -40,15 +42,15 @@ typedef struct ArchiveModuleCallbacks
  * Type of the shared library symbol _PG_archive_module_init that is looked
  * up when loading an archive library.
  */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
 
 /*
  * Since the logic for archiving via a shell command is in the core server
  * and does not need to be loaded via a shared library, it has a special
  * initialization function.
  */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
 
 #endif							/* _ARCHIVE_MODULE_H */
-- 
2.25.1


--cWoXeonUoKmBZSoM--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v11 1/1] restructure archive modules API
@ 2023-02-09 21:49  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-02-09 21:49 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c         | 53 ++++++++++++-----
 doc/src/sgml/archive-modules.sgml             | 32 +++++++---
 src/backend/Makefile                          |  2 +-
 src/backend/archive/Makefile                  | 18 ++++++
 src/backend/archive/meson.build               |  5 ++
 .../{postmaster => archive}/shell_archive.c   | 30 ++++++----
 src/backend/meson.build                       |  1 +
 src/backend/postmaster/Makefile               |  1 -
 src/backend/postmaster/meson.build            |  1 -
 src/backend/postmaster/pgarch.c               | 27 +++++----
 src/backend/utils/misc/guc_tables.c           |  1 +
 src/include/archive/archive_module.h          | 58 +++++++++++++++++++
 src/include/archive/shell_archive.h           | 22 +++++++
 src/include/postmaster/pgarch.h               | 39 -------------
 14 files changed, 205 insertions(+), 85 deletions(-)
 create mode 100644 src/backend/archive/Makefile
 create mode 100644 src/backend/archive/meson.build
 rename src/backend/{postmaster => archive}/shell_archive.c (78%)
 create mode 100644 src/include/archive/archive_module.h
 create mode 100644 src/include/archive/shell_archive.h

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 36b7a4814a..8a6bc25db5 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -30,9 +30,9 @@
 #include <sys/time.h>
 #include <unistd.h>
 
+#include "archive/archive_module.h"
 #include "common/int.h"
 #include "miscadmin.h"
-#include "postmaster/pgarch.h"
 #include "storage/copydir.h"
 #include "storage/fd.h"
 #include "utils/guc.h"
@@ -40,15 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+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 const ArchiveModuleCallbacks basic_archive_callbacks = {
+	.startup_cb = basic_archive_startup,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = NULL
+};
+
 /*
  * _PG_init
  *
@@ -67,10 +79,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,11 +86,28 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
+{
+	return &basic_archive_callbacks;
+}
+
+/*
+ * basic_archive_startup
+ *
+ * Creates the module's memory context.
+ */
 void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+basic_archive_startup(ArchiveModuleState *state)
 {
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	BasicArchiveData *data;
+
+	data = (BasicArchiveData *) MemoryContextAllocZero(TopMemoryContext,
+													   sizeof(BasicArchiveData));
+	data->context = AllocSetContextCreate(TopMemoryContext,
+										  "basic_archive",
+										  ALLOCSET_DEFAULT_SIZES);
+	state->private_data = (void *) data;
 }
 
 /*
@@ -133,7 +158,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -144,10 +169,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+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
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..8ea697a5bc 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,18 +47,22 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
-   individual actions.
+   <function>_PG_archive_module_init</function>.  The result of the function
+   must be a pointer to a struct of type
+   <structname>ArchiveModuleCallbacks</structname>, which contains everything
+   that the core code needs to know how to make use of the archive module.  The
+   return value needs to be of server lifetime, which is typically achieved by
+   defining it as a <literal>static const</literal> variable in global scope.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
@@ -73,6 +77,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module has state, it can use
+    <structfield>state->private_data</structfield> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +101,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +123,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -128,7 +146,7 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     these situations.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 86e6dcb792..e4bf0fe9c0 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -17,7 +17,7 @@ subdir = src/backend
 top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 
-SUBDIRS = access backup bootstrap catalog parser commands executor \
+SUBDIRS = access archive backup bootstrap catalog parser commands executor \
 	foreign lib libpq \
 	main nodes optimizer partitioning port postmaster \
 	regex replication rewrite \
diff --git a/src/backend/archive/Makefile b/src/backend/archive/Makefile
new file mode 100644
index 0000000000..8d2860d0df
--- /dev/null
+++ b/src/backend/archive/Makefile
@@ -0,0 +1,18 @@
+#-------------------------------------------------------------------------
+#
+# Makefile--
+#    Makefile for src/backend/archive
+#
+# IDENTIFICATION
+#    src/backend/archive/Makefile
+#
+#-------------------------------------------------------------------------
+
+subdir = src/backend/archive
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
+
+OBJS = \
+	shell_archive.o
+
+include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/archive/meson.build b/src/backend/archive/meson.build
new file mode 100644
index 0000000000..e194282931
--- /dev/null
+++ b/src/backend/archive/meson.build
@@ -0,0 +1,5 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+backend_sources += files(
+  'shell_archive.c'
+)
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/archive/shell_archive.c
similarity index 78%
rename from src/backend/postmaster/shell_archive.c
rename to src/backend/archive/shell_archive.c
index 7771b951b7..48e8952c0c 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/archive/shell_archive.c
@@ -18,30 +18,36 @@
 #include <sys/wait.h>
 
 #include "access/xlog.h"
+#include "archive/archive_module.h"
+#include "archive/shell_archive.h"
 #include "common/percentrepl.h"
 #include "pgstat.h"
-#include "postmaster/pgarch.h"
 
-static bool shell_archive_configured(void);
-static bool shell_archive_file(const char *file, const char *path);
-static void shell_archive_shutdown(void);
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static void shell_archive_shutdown(ArchiveModuleState *state);
 
-void
-shell_archive_init(ArchiveModuleCallbacks *cb)
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(ArchiveModuleState *state, const char *file, const char *path)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -123,7 +129,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/backend/meson.build b/src/backend/meson.build
index b1db3ba75b..4fdd209b82 100644
--- a/src/backend/meson.build
+++ b/src/backend/meson.build
@@ -7,6 +7,7 @@ backend_link_with = [pgport_srv, common_srv]
 generated_backend_sources = []
 
 subdir('access')
+subdir('archive')
 subdir('backup')
 subdir('bootstrap')
 subdir('catalog')
diff --git a/src/backend/postmaster/Makefile b/src/backend/postmaster/Makefile
index 3a794e54d6..047448b34e 100644
--- a/src/backend/postmaster/Makefile
+++ b/src/backend/postmaster/Makefile
@@ -22,7 +22,6 @@ OBJS = \
 	interrupt.o \
 	pgarch.o \
 	postmaster.o \
-	shell_archive.o \
 	startup.o \
 	syslogger.o \
 	walwriter.o
diff --git a/src/backend/postmaster/meson.build b/src/backend/postmaster/meson.build
index 9079922de7..cda921fd10 100644
--- a/src/backend/postmaster/meson.build
+++ b/src/backend/postmaster/meson.build
@@ -10,7 +10,6 @@ backend_sources += files(
   'interrupt.c',
   'pgarch.c',
   'postmaster.c',
-  'shell_archive.c',
   'startup.c',
   'syslogger.c',
   'walwriter.c',
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index e551af2905..46af349564 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -31,6 +31,8 @@
 
 #include "access/xlog.h"
 #include "access/xlog_internal.h"
+#include "archive/archive_module.h"
+#include "archive/shell_archive.h"
 #include "lib/binaryheap.h"
 #include "libpq/pqsignal.h"
 #include "pgstat.h"
@@ -97,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -406,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveContext.check_configured_cb != NULL &&
-				!ArchiveContext.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveContext.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -814,7 +817,7 @@ HandlePgArchInterrupts(void)
 /*
  * LoadArchiveLibrary
  *
- * Loads the archiving callbacks into our local ArchiveContext.
+ * Loads the archiving callbacks into our local ArchiveCallbacks.
  */
 static void
 LoadArchiveLibrary(void)
@@ -827,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveContext, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -844,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveContext);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveContext.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -859,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveContext.shutdown_cb != NULL)
-		ArchiveContext.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index b46e3b8c55..91cb64602b 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -33,6 +33,7 @@
 #include "access/xlog_internal.h"
 #include "access/xlogprefetcher.h"
 #include "access/xlogrecovery.h"
+#include "archive/archive_module.h"
 #include "catalog/namespace.h"
 #include "catalog/storage.h"
 #include "commands/async.h"
diff --git a/src/include/archive/archive_module.h b/src/include/archive/archive_module.h
new file mode 100644
index 0000000000..92ced4b222
--- /dev/null
+++ b/src/include/archive/archive_module.h
@@ -0,0 +1,58 @@
+/*-------------------------------------------------------------------------
+ *
+ * archive_module.h
+ *		Exports for archive modules.
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * src/include/archive/archive_module.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef _ARCHIVE_MODULE_H
+#define _ARCHIVE_MODULE_H
+
+/*
+ * The value of the archive_library GUC.
+ */
+extern PGDLLIMPORT char *XLogArchiveLibrary;
+
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
+/*
+ * Archive module callbacks
+ *
+ * These callback functions should be defined by archive libraries and returned
+ * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
+ * For more information about the purpose of each callback, refer to the
+ * archive modules documentation.
+ */
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
+
+typedef struct ArchiveModuleCallbacks
+{
+	ArchiveStartupCB startup_cb;
+	ArchiveCheckConfiguredCB check_configured_cb;
+	ArchiveFileCB archive_file_cb;
+	ArchiveShutdownCB shutdown_cb;
+} ArchiveModuleCallbacks;
+
+/*
+ * Type of the shared library symbol _PG_archive_module_init that is looked
+ * up when loading an archive library.
+ */
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
+
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
+
+#endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/archive/shell_archive.h b/src/include/archive/shell_archive.h
new file mode 100644
index 0000000000..1c843c13c5
--- /dev/null
+++ b/src/include/archive/shell_archive.h
@@ -0,0 +1,22 @@
+/*-------------------------------------------------------------------------
+ *
+ * shell_archive.h
+ *		Exports for archiving via shell.
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * src/include/archive/shell_archive.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef _SHELL_ARCHIVE_H
+#define _SHELL_ARCHIVE_H
+
+/*
+ * Since the logic for archiving via a shell command is in the core server
+ * and does not need to be loaded via a shared library, it has a special
+ * initialization function.
+ */
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
+
+#endif							/* _SHELL_ARCHIVE_H */
diff --git a/src/include/postmaster/pgarch.h b/src/include/postmaster/pgarch.h
index bcd51dfad6..3bd4fac71e 100644
--- a/src/include/postmaster/pgarch.h
+++ b/src/include/postmaster/pgarch.h
@@ -33,43 +33,4 @@ extern void PgArchiverMain(void) pg_attribute_noreturn();
 extern void PgArchWakeup(void);
 extern void PgArchForceDirScan(void);
 
-/*
- * The value of the archive_library GUC.
- */
-extern PGDLLIMPORT char *XLogArchiveLibrary;
-
-/*
- * Archive module callbacks
- *
- * These callback functions should be defined by archive libraries and returned
- * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
- * For more information about the purpose of each callback, refer to the
- * archive modules documentation.
- */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
-
-typedef struct ArchiveModuleCallbacks
-{
-	ArchiveCheckConfiguredCB check_configured_cb;
-	ArchiveFileCB archive_file_cb;
-	ArchiveShutdownCB shutdown_cb;
-} ArchiveModuleCallbacks;
-
-/*
- * Type of the shared library symbol _PG_archive_module_init that is looked
- * up when loading an archive library.
- */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
-
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
-
-/*
- * Since the logic for archiving via a shell command is in the core server
- * and does not need to be loaded via a shared library, it has a special
- * initialization function.
- */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
-
 #endif							/* _PGARCH_H */
-- 
2.25.1


--k+w/mQv8wyuph6w0--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v15 1/1] restructure archive modules API
@ 2023-02-09 21:49  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-02-09 21:49 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c         | 86 ++++++++++++++++---
 doc/src/sgml/archive-modules.sgml             | 35 ++++++--
 src/backend/Makefile                          |  2 +-
 src/backend/archive/Makefile                  | 18 ++++
 src/backend/archive/meson.build               |  5 ++
 .../{postmaster => archive}/shell_archive.c   | 32 ++++---
 src/backend/meson.build                       |  1 +
 src/backend/postmaster/Makefile               |  1 -
 src/backend/postmaster/meson.build            |  1 -
 src/backend/postmaster/pgarch.c               | 27 +++---
 src/backend/utils/misc/guc_tables.c           |  1 +
 src/include/archive/archive_module.h          | 58 +++++++++++++
 src/include/archive/shell_archive.h           | 24 ++++++
 src/include/postmaster/pgarch.h               | 39 ---------
 14 files changed, 243 insertions(+), 87 deletions(-)
 create mode 100644 src/backend/archive/Makefile
 create mode 100644 src/backend/archive/meson.build
 rename src/backend/{postmaster => archive}/shell_archive.c (77%)
 create mode 100644 src/include/archive/archive_module.h
 create mode 100644 src/include/archive/shell_archive.h

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 36b7a4814a..cd852888ce 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -30,9 +30,9 @@
 #include <sys/time.h>
 #include <unistd.h>
 
+#include "archive/archive_module.h"
 #include "common/int.h"
 #include "miscadmin.h"
-#include "postmaster/pgarch.h"
 #include "storage/copydir.h"
 #include "storage/fd.h"
 #include "utils/guc.h"
@@ -40,14 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+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,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = basic_archive_shutdown
+};
 
 /*
  * _PG_init
@@ -67,10 +80,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,11 +87,28 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
+{
+	return &basic_archive_callbacks;
+}
+
+/*
+ * basic_archive_startup
+ *
+ * Creates the module's memory context.
+ */
 void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+basic_archive_startup(ArchiveModuleState *state)
 {
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	BasicArchiveData *data;
+
+	data = (BasicArchiveData *) MemoryContextAllocZero(TopMemoryContext,
+													   sizeof(BasicArchiveData));
+	data->context = AllocSetContextCreate(TopMemoryContext,
+										  "basic_archive",
+										  ALLOCSET_DEFAULT_SIZES);
+	state->private_data = (void *) data;
 }
 
 /*
@@ -133,7 +159,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -144,10 +170,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+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
@@ -366,3 +394,35 @@ 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 ef02051f7f..668eb34991 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,18 +47,22 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
-   individual actions.
+   <function>_PG_archive_module_init</function>.  The result of the function
+   must be a pointer to a struct of type
+   <structname>ArchiveModuleCallbacks</structname>, which contains everything
+   that the core code needs to know how to make use of the archive module.  The
+   return value needs to be of server lifetime, which is typically achieved by
+   defining it as a <literal>static const</literal> variable in global scope.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
@@ -73,6 +77,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module has state, it can use
+    <structfield>state->private_data</structfield> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +101,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +123,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -125,10 +143,11 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     process exits (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.
+    these situations.  If the archive module has state, this callback should
+    free it to avoid leaks.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 86e6dcb792..e4bf0fe9c0 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -17,7 +17,7 @@ subdir = src/backend
 top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 
-SUBDIRS = access backup bootstrap catalog parser commands executor \
+SUBDIRS = access archive backup bootstrap catalog parser commands executor \
 	foreign lib libpq \
 	main nodes optimizer partitioning port postmaster \
 	regex replication rewrite \
diff --git a/src/backend/archive/Makefile b/src/backend/archive/Makefile
new file mode 100644
index 0000000000..8d2860d0df
--- /dev/null
+++ b/src/backend/archive/Makefile
@@ -0,0 +1,18 @@
+#-------------------------------------------------------------------------
+#
+# Makefile--
+#    Makefile for src/backend/archive
+#
+# IDENTIFICATION
+#    src/backend/archive/Makefile
+#
+#-------------------------------------------------------------------------
+
+subdir = src/backend/archive
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
+
+OBJS = \
+	shell_archive.o
+
+include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/archive/meson.build b/src/backend/archive/meson.build
new file mode 100644
index 0000000000..e194282931
--- /dev/null
+++ b/src/backend/archive/meson.build
@@ -0,0 +1,5 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+backend_sources += files(
+  'shell_archive.c'
+)
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/archive/shell_archive.c
similarity index 77%
rename from src/backend/postmaster/shell_archive.c
rename to src/backend/archive/shell_archive.c
index 7771b951b7..837da543f9 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/archive/shell_archive.c
@@ -9,7 +9,7 @@
  * Copyright (c) 2022-2023, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
- *	  src/backend/postmaster/shell_archive.c
+ *	  src/backend/archive/shell_archive.c
  *
  *-------------------------------------------------------------------------
  */
@@ -18,30 +18,36 @@
 #include <sys/wait.h>
 
 #include "access/xlog.h"
+#include "archive/archive_module.h"
+#include "archive/shell_archive.h"
 #include "common/percentrepl.h"
 #include "pgstat.h"
-#include "postmaster/pgarch.h"
 
-static bool shell_archive_configured(void);
-static bool shell_archive_file(const char *file, const char *path);
-static void shell_archive_shutdown(void);
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static void shell_archive_shutdown(ArchiveModuleState *state);
 
-void
-shell_archive_init(ArchiveModuleCallbacks *cb)
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(ArchiveModuleState *state, const char *file, const char *path)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -123,7 +129,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/backend/meson.build b/src/backend/meson.build
index b1db3ba75b..4fdd209b82 100644
--- a/src/backend/meson.build
+++ b/src/backend/meson.build
@@ -7,6 +7,7 @@ backend_link_with = [pgport_srv, common_srv]
 generated_backend_sources = []
 
 subdir('access')
+subdir('archive')
 subdir('backup')
 subdir('bootstrap')
 subdir('catalog')
diff --git a/src/backend/postmaster/Makefile b/src/backend/postmaster/Makefile
index 3a794e54d6..047448b34e 100644
--- a/src/backend/postmaster/Makefile
+++ b/src/backend/postmaster/Makefile
@@ -22,7 +22,6 @@ OBJS = \
 	interrupt.o \
 	pgarch.o \
 	postmaster.o \
-	shell_archive.o \
 	startup.o \
 	syslogger.o \
 	walwriter.o
diff --git a/src/backend/postmaster/meson.build b/src/backend/postmaster/meson.build
index 9079922de7..cda921fd10 100644
--- a/src/backend/postmaster/meson.build
+++ b/src/backend/postmaster/meson.build
@@ -10,7 +10,6 @@ backend_sources += files(
   'interrupt.c',
   'pgarch.c',
   'postmaster.c',
-  'shell_archive.c',
   'startup.c',
   'syslogger.c',
   'walwriter.c',
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index e551af2905..46af349564 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -31,6 +31,8 @@
 
 #include "access/xlog.h"
 #include "access/xlog_internal.h"
+#include "archive/archive_module.h"
+#include "archive/shell_archive.h"
 #include "lib/binaryheap.h"
 #include "libpq/pqsignal.h"
 #include "pgstat.h"
@@ -97,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -406,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveContext.check_configured_cb != NULL &&
-				!ArchiveContext.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveContext.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -814,7 +817,7 @@ HandlePgArchInterrupts(void)
 /*
  * LoadArchiveLibrary
  *
- * Loads the archiving callbacks into our local ArchiveContext.
+ * Loads the archiving callbacks into our local ArchiveCallbacks.
  */
 static void
 LoadArchiveLibrary(void)
@@ -827,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveContext, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -844,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveContext);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveContext.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -859,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveContext.shutdown_cb != NULL)
-		ArchiveContext.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 43b9d92660..1c0583fe26 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -33,6 +33,7 @@
 #include "access/xlog_internal.h"
 #include "access/xlogprefetcher.h"
 #include "access/xlogrecovery.h"
+#include "archive/archive_module.h"
 #include "catalog/namespace.h"
 #include "catalog/storage.h"
 #include "commands/async.h"
diff --git a/src/include/archive/archive_module.h b/src/include/archive/archive_module.h
new file mode 100644
index 0000000000..92ced4b222
--- /dev/null
+++ b/src/include/archive/archive_module.h
@@ -0,0 +1,58 @@
+/*-------------------------------------------------------------------------
+ *
+ * archive_module.h
+ *		Exports for archive modules.
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * src/include/archive/archive_module.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef _ARCHIVE_MODULE_H
+#define _ARCHIVE_MODULE_H
+
+/*
+ * The value of the archive_library GUC.
+ */
+extern PGDLLIMPORT char *XLogArchiveLibrary;
+
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
+/*
+ * Archive module callbacks
+ *
+ * These callback functions should be defined by archive libraries and returned
+ * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
+ * For more information about the purpose of each callback, refer to the
+ * archive modules documentation.
+ */
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
+
+typedef struct ArchiveModuleCallbacks
+{
+	ArchiveStartupCB startup_cb;
+	ArchiveCheckConfiguredCB check_configured_cb;
+	ArchiveFileCB archive_file_cb;
+	ArchiveShutdownCB shutdown_cb;
+} ArchiveModuleCallbacks;
+
+/*
+ * Type of the shared library symbol _PG_archive_module_init that is looked
+ * up when loading an archive library.
+ */
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
+
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
+
+#endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/archive/shell_archive.h b/src/include/archive/shell_archive.h
new file mode 100644
index 0000000000..9de6f769f1
--- /dev/null
+++ b/src/include/archive/shell_archive.h
@@ -0,0 +1,24 @@
+/*-------------------------------------------------------------------------
+ *
+ * shell_archive.h
+ *		Exports for archiving via shell.
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * src/include/archive/shell_archive.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef _SHELL_ARCHIVE_H
+#define _SHELL_ARCHIVE_H
+
+#include "archive/archive_module.h"
+
+/*
+ * Since the logic for archiving via a shell command is in the core server
+ * and does not need to be loaded via a shared library, it has a special
+ * initialization function.
+ */
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
+
+#endif							/* _SHELL_ARCHIVE_H */
diff --git a/src/include/postmaster/pgarch.h b/src/include/postmaster/pgarch.h
index bcd51dfad6..3bd4fac71e 100644
--- a/src/include/postmaster/pgarch.h
+++ b/src/include/postmaster/pgarch.h
@@ -33,43 +33,4 @@ extern void PgArchiverMain(void) pg_attribute_noreturn();
 extern void PgArchWakeup(void);
 extern void PgArchForceDirScan(void);
 
-/*
- * The value of the archive_library GUC.
- */
-extern PGDLLIMPORT char *XLogArchiveLibrary;
-
-/*
- * Archive module callbacks
- *
- * These callback functions should be defined by archive libraries and returned
- * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
- * For more information about the purpose of each callback, refer to the
- * archive modules documentation.
- */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
-
-typedef struct ArchiveModuleCallbacks
-{
-	ArchiveCheckConfiguredCB check_configured_cb;
-	ArchiveFileCB archive_file_cb;
-	ArchiveShutdownCB shutdown_cb;
-} ArchiveModuleCallbacks;
-
-/*
- * Type of the shared library symbol _PG_archive_module_init that is looked
- * up when loading an archive library.
- */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
-
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
-
-/*
- * Since the logic for archiving via a shell command is in the core server
- * and does not need to be loaded via a shared library, it has a special
- * initialization function.
- */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
-
 #endif							/* _PGARCH_H */
-- 
2.25.1


--HcAYCG3uE/tztfnV--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v14 1/1] restructure archive modules API
@ 2023-02-09 21:49  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-02-09 21:49 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c         | 85 ++++++++++++++++---
 doc/src/sgml/archive-modules.sgml             | 35 ++++++--
 src/backend/Makefile                          |  2 +-
 src/backend/archive/Makefile                  | 18 ++++
 src/backend/archive/meson.build               |  5 ++
 .../{postmaster => archive}/shell_archive.c   | 32 ++++---
 src/backend/meson.build                       |  1 +
 src/backend/postmaster/Makefile               |  1 -
 src/backend/postmaster/meson.build            |  1 -
 src/backend/postmaster/pgarch.c               | 27 +++---
 src/backend/utils/misc/guc_tables.c           |  1 +
 src/include/archive/archive_module.h          | 58 +++++++++++++
 src/include/archive/shell_archive.h           | 24 ++++++
 src/include/postmaster/pgarch.h               | 39 ---------
 14 files changed, 242 insertions(+), 87 deletions(-)
 create mode 100644 src/backend/archive/Makefile
 create mode 100644 src/backend/archive/meson.build
 rename src/backend/{postmaster => archive}/shell_archive.c (77%)
 create mode 100644 src/include/archive/archive_module.h
 create mode 100644 src/include/archive/shell_archive.h

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 36b7a4814a..d7c227a10b 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -30,9 +30,9 @@
 #include <sys/time.h>
 #include <unistd.h>
 
+#include "archive/archive_module.h"
 #include "common/int.h"
 #include "miscadmin.h"
-#include "postmaster/pgarch.h"
 #include "storage/copydir.h"
 #include "storage/fd.h"
 #include "utils/guc.h"
@@ -40,14 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+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,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = basic_archive_shutdown
+};
 
 /*
  * _PG_init
@@ -67,10 +80,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,11 +87,28 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
+{
+	return &basic_archive_callbacks;
+}
+
+/*
+ * basic_archive_startup
+ *
+ * Creates the module's memory context.
+ */
 void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+basic_archive_startup(ArchiveModuleState *state)
 {
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	BasicArchiveData *data;
+
+	data = (BasicArchiveData *) MemoryContextAllocZero(TopMemoryContext,
+													   sizeof(BasicArchiveData));
+	data->context = AllocSetContextCreate(TopMemoryContext,
+										  "basic_archive",
+										  ALLOCSET_DEFAULT_SIZES);
+	state->private_data = (void *) data;
 }
 
 /*
@@ -133,7 +159,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -144,10 +170,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+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
@@ -366,3 +394,34 @@ 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);
+
+	/*
+	 * 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 ef02051f7f..668eb34991 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,18 +47,22 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
-   individual actions.
+   <function>_PG_archive_module_init</function>.  The result of the function
+   must be a pointer to a struct of type
+   <structname>ArchiveModuleCallbacks</structname>, which contains everything
+   that the core code needs to know how to make use of the archive module.  The
+   return value needs to be of server lifetime, which is typically achieved by
+   defining it as a <literal>static const</literal> variable in global scope.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
@@ -73,6 +77,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module has state, it can use
+    <structfield>state->private_data</structfield> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +101,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +123,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -125,10 +143,11 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     process exits (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.
+    these situations.  If the archive module has state, this callback should
+    free it to avoid leaks.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 86e6dcb792..e4bf0fe9c0 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -17,7 +17,7 @@ subdir = src/backend
 top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 
-SUBDIRS = access backup bootstrap catalog parser commands executor \
+SUBDIRS = access archive backup bootstrap catalog parser commands executor \
 	foreign lib libpq \
 	main nodes optimizer partitioning port postmaster \
 	regex replication rewrite \
diff --git a/src/backend/archive/Makefile b/src/backend/archive/Makefile
new file mode 100644
index 0000000000..8d2860d0df
--- /dev/null
+++ b/src/backend/archive/Makefile
@@ -0,0 +1,18 @@
+#-------------------------------------------------------------------------
+#
+# Makefile--
+#    Makefile for src/backend/archive
+#
+# IDENTIFICATION
+#    src/backend/archive/Makefile
+#
+#-------------------------------------------------------------------------
+
+subdir = src/backend/archive
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
+
+OBJS = \
+	shell_archive.o
+
+include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/archive/meson.build b/src/backend/archive/meson.build
new file mode 100644
index 0000000000..e194282931
--- /dev/null
+++ b/src/backend/archive/meson.build
@@ -0,0 +1,5 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+backend_sources += files(
+  'shell_archive.c'
+)
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/archive/shell_archive.c
similarity index 77%
rename from src/backend/postmaster/shell_archive.c
rename to src/backend/archive/shell_archive.c
index 7771b951b7..837da543f9 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/archive/shell_archive.c
@@ -9,7 +9,7 @@
  * Copyright (c) 2022-2023, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
- *	  src/backend/postmaster/shell_archive.c
+ *	  src/backend/archive/shell_archive.c
  *
  *-------------------------------------------------------------------------
  */
@@ -18,30 +18,36 @@
 #include <sys/wait.h>
 
 #include "access/xlog.h"
+#include "archive/archive_module.h"
+#include "archive/shell_archive.h"
 #include "common/percentrepl.h"
 #include "pgstat.h"
-#include "postmaster/pgarch.h"
 
-static bool shell_archive_configured(void);
-static bool shell_archive_file(const char *file, const char *path);
-static void shell_archive_shutdown(void);
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static void shell_archive_shutdown(ArchiveModuleState *state);
 
-void
-shell_archive_init(ArchiveModuleCallbacks *cb)
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(ArchiveModuleState *state, const char *file, const char *path)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -123,7 +129,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/backend/meson.build b/src/backend/meson.build
index b1db3ba75b..4fdd209b82 100644
--- a/src/backend/meson.build
+++ b/src/backend/meson.build
@@ -7,6 +7,7 @@ backend_link_with = [pgport_srv, common_srv]
 generated_backend_sources = []
 
 subdir('access')
+subdir('archive')
 subdir('backup')
 subdir('bootstrap')
 subdir('catalog')
diff --git a/src/backend/postmaster/Makefile b/src/backend/postmaster/Makefile
index 3a794e54d6..047448b34e 100644
--- a/src/backend/postmaster/Makefile
+++ b/src/backend/postmaster/Makefile
@@ -22,7 +22,6 @@ OBJS = \
 	interrupt.o \
 	pgarch.o \
 	postmaster.o \
-	shell_archive.o \
 	startup.o \
 	syslogger.o \
 	walwriter.o
diff --git a/src/backend/postmaster/meson.build b/src/backend/postmaster/meson.build
index 9079922de7..cda921fd10 100644
--- a/src/backend/postmaster/meson.build
+++ b/src/backend/postmaster/meson.build
@@ -10,7 +10,6 @@ backend_sources += files(
   'interrupt.c',
   'pgarch.c',
   'postmaster.c',
-  'shell_archive.c',
   'startup.c',
   'syslogger.c',
   'walwriter.c',
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index e551af2905..46af349564 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -31,6 +31,8 @@
 
 #include "access/xlog.h"
 #include "access/xlog_internal.h"
+#include "archive/archive_module.h"
+#include "archive/shell_archive.h"
 #include "lib/binaryheap.h"
 #include "libpq/pqsignal.h"
 #include "pgstat.h"
@@ -97,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -406,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveContext.check_configured_cb != NULL &&
-				!ArchiveContext.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveContext.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -814,7 +817,7 @@ HandlePgArchInterrupts(void)
 /*
  * LoadArchiveLibrary
  *
- * Loads the archiving callbacks into our local ArchiveContext.
+ * Loads the archiving callbacks into our local ArchiveCallbacks.
  */
 static void
 LoadArchiveLibrary(void)
@@ -827,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveContext, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -844,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveContext);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveContext.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -859,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveContext.shutdown_cb != NULL)
-		ArchiveContext.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 43b9d92660..1c0583fe26 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -33,6 +33,7 @@
 #include "access/xlog_internal.h"
 #include "access/xlogprefetcher.h"
 #include "access/xlogrecovery.h"
+#include "archive/archive_module.h"
 #include "catalog/namespace.h"
 #include "catalog/storage.h"
 #include "commands/async.h"
diff --git a/src/include/archive/archive_module.h b/src/include/archive/archive_module.h
new file mode 100644
index 0000000000..92ced4b222
--- /dev/null
+++ b/src/include/archive/archive_module.h
@@ -0,0 +1,58 @@
+/*-------------------------------------------------------------------------
+ *
+ * archive_module.h
+ *		Exports for archive modules.
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * src/include/archive/archive_module.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef _ARCHIVE_MODULE_H
+#define _ARCHIVE_MODULE_H
+
+/*
+ * The value of the archive_library GUC.
+ */
+extern PGDLLIMPORT char *XLogArchiveLibrary;
+
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
+/*
+ * Archive module callbacks
+ *
+ * These callback functions should be defined by archive libraries and returned
+ * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
+ * For more information about the purpose of each callback, refer to the
+ * archive modules documentation.
+ */
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
+
+typedef struct ArchiveModuleCallbacks
+{
+	ArchiveStartupCB startup_cb;
+	ArchiveCheckConfiguredCB check_configured_cb;
+	ArchiveFileCB archive_file_cb;
+	ArchiveShutdownCB shutdown_cb;
+} ArchiveModuleCallbacks;
+
+/*
+ * Type of the shared library symbol _PG_archive_module_init that is looked
+ * up when loading an archive library.
+ */
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
+
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
+
+#endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/archive/shell_archive.h b/src/include/archive/shell_archive.h
new file mode 100644
index 0000000000..9de6f769f1
--- /dev/null
+++ b/src/include/archive/shell_archive.h
@@ -0,0 +1,24 @@
+/*-------------------------------------------------------------------------
+ *
+ * shell_archive.h
+ *		Exports for archiving via shell.
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * src/include/archive/shell_archive.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef _SHELL_ARCHIVE_H
+#define _SHELL_ARCHIVE_H
+
+#include "archive/archive_module.h"
+
+/*
+ * Since the logic for archiving via a shell command is in the core server
+ * and does not need to be loaded via a shared library, it has a special
+ * initialization function.
+ */
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
+
+#endif							/* _SHELL_ARCHIVE_H */
diff --git a/src/include/postmaster/pgarch.h b/src/include/postmaster/pgarch.h
index bcd51dfad6..3bd4fac71e 100644
--- a/src/include/postmaster/pgarch.h
+++ b/src/include/postmaster/pgarch.h
@@ -33,43 +33,4 @@ extern void PgArchiverMain(void) pg_attribute_noreturn();
 extern void PgArchWakeup(void);
 extern void PgArchForceDirScan(void);
 
-/*
- * The value of the archive_library GUC.
- */
-extern PGDLLIMPORT char *XLogArchiveLibrary;
-
-/*
- * Archive module callbacks
- *
- * These callback functions should be defined by archive libraries and returned
- * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
- * For more information about the purpose of each callback, refer to the
- * archive modules documentation.
- */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
-
-typedef struct ArchiveModuleCallbacks
-{
-	ArchiveCheckConfiguredCB check_configured_cb;
-	ArchiveFileCB archive_file_cb;
-	ArchiveShutdownCB shutdown_cb;
-} ArchiveModuleCallbacks;
-
-/*
- * Type of the shared library symbol _PG_archive_module_init that is looked
- * up when loading an archive library.
- */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
-
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
-
-/*
- * Since the logic for archiving via a shell command is in the core server
- * and does not need to be loaded via a shared library, it has a special
- * initialization function.
- */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
-
 #endif							/* _PGARCH_H */
-- 
2.25.1


--DocE+STaALJfprDB--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v12 1/1] restructure archive modules API
@ 2023-02-09 21:49  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-02-09 21:49 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c         | 85 ++++++++++++++++---
 doc/src/sgml/archive-modules.sgml             | 35 ++++++--
 src/backend/Makefile                          |  2 +-
 src/backend/archive/Makefile                  | 18 ++++
 src/backend/archive/meson.build               |  5 ++
 .../{postmaster => archive}/shell_archive.c   | 30 ++++---
 src/backend/meson.build                       |  1 +
 src/backend/postmaster/Makefile               |  1 -
 src/backend/postmaster/meson.build            |  1 -
 src/backend/postmaster/pgarch.c               | 27 +++---
 src/backend/utils/misc/guc_tables.c           |  1 +
 src/include/archive/archive_module.h          | 58 +++++++++++++
 src/include/archive/shell_archive.h           | 22 +++++
 src/include/postmaster/pgarch.h               | 39 ---------
 14 files changed, 239 insertions(+), 86 deletions(-)
 create mode 100644 src/backend/archive/Makefile
 create mode 100644 src/backend/archive/meson.build
 rename src/backend/{postmaster => archive}/shell_archive.c (78%)
 create mode 100644 src/include/archive/archive_module.h
 create mode 100644 src/include/archive/shell_archive.h

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 36b7a4814a..d7c227a10b 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -30,9 +30,9 @@
 #include <sys/time.h>
 #include <unistd.h>
 
+#include "archive/archive_module.h"
 #include "common/int.h"
 #include "miscadmin.h"
-#include "postmaster/pgarch.h"
 #include "storage/copydir.h"
 #include "storage/fd.h"
 #include "utils/guc.h"
@@ -40,14 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+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,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = basic_archive_shutdown
+};
 
 /*
  * _PG_init
@@ -67,10 +80,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,11 +87,28 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
+{
+	return &basic_archive_callbacks;
+}
+
+/*
+ * basic_archive_startup
+ *
+ * Creates the module's memory context.
+ */
 void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+basic_archive_startup(ArchiveModuleState *state)
 {
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	BasicArchiveData *data;
+
+	data = (BasicArchiveData *) MemoryContextAllocZero(TopMemoryContext,
+													   sizeof(BasicArchiveData));
+	data->context = AllocSetContextCreate(TopMemoryContext,
+										  "basic_archive",
+										  ALLOCSET_DEFAULT_SIZES);
+	state->private_data = (void *) data;
 }
 
 /*
@@ -133,7 +159,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -144,10 +170,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+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
@@ -366,3 +394,34 @@ 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);
+
+	/*
+	 * 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 ef02051f7f..668eb34991 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,18 +47,22 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
-   individual actions.
+   <function>_PG_archive_module_init</function>.  The result of the function
+   must be a pointer to a struct of type
+   <structname>ArchiveModuleCallbacks</structname>, which contains everything
+   that the core code needs to know how to make use of the archive module.  The
+   return value needs to be of server lifetime, which is typically achieved by
+   defining it as a <literal>static const</literal> variable in global scope.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
@@ -73,6 +77,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module has state, it can use
+    <structfield>state->private_data</structfield> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +101,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +123,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -125,10 +143,11 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     process exits (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.
+    these situations.  If the archive module has state, this callback should
+    free it to avoid leaks.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 86e6dcb792..e4bf0fe9c0 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -17,7 +17,7 @@ subdir = src/backend
 top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 
-SUBDIRS = access backup bootstrap catalog parser commands executor \
+SUBDIRS = access archive backup bootstrap catalog parser commands executor \
 	foreign lib libpq \
 	main nodes optimizer partitioning port postmaster \
 	regex replication rewrite \
diff --git a/src/backend/archive/Makefile b/src/backend/archive/Makefile
new file mode 100644
index 0000000000..8d2860d0df
--- /dev/null
+++ b/src/backend/archive/Makefile
@@ -0,0 +1,18 @@
+#-------------------------------------------------------------------------
+#
+# Makefile--
+#    Makefile for src/backend/archive
+#
+# IDENTIFICATION
+#    src/backend/archive/Makefile
+#
+#-------------------------------------------------------------------------
+
+subdir = src/backend/archive
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
+
+OBJS = \
+	shell_archive.o
+
+include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/archive/meson.build b/src/backend/archive/meson.build
new file mode 100644
index 0000000000..e194282931
--- /dev/null
+++ b/src/backend/archive/meson.build
@@ -0,0 +1,5 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+backend_sources += files(
+  'shell_archive.c'
+)
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/archive/shell_archive.c
similarity index 78%
rename from src/backend/postmaster/shell_archive.c
rename to src/backend/archive/shell_archive.c
index 7771b951b7..48e8952c0c 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/archive/shell_archive.c
@@ -18,30 +18,36 @@
 #include <sys/wait.h>
 
 #include "access/xlog.h"
+#include "archive/archive_module.h"
+#include "archive/shell_archive.h"
 #include "common/percentrepl.h"
 #include "pgstat.h"
-#include "postmaster/pgarch.h"
 
-static bool shell_archive_configured(void);
-static bool shell_archive_file(const char *file, const char *path);
-static void shell_archive_shutdown(void);
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static void shell_archive_shutdown(ArchiveModuleState *state);
 
-void
-shell_archive_init(ArchiveModuleCallbacks *cb)
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(ArchiveModuleState *state, const char *file, const char *path)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -123,7 +129,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/backend/meson.build b/src/backend/meson.build
index b1db3ba75b..4fdd209b82 100644
--- a/src/backend/meson.build
+++ b/src/backend/meson.build
@@ -7,6 +7,7 @@ backend_link_with = [pgport_srv, common_srv]
 generated_backend_sources = []
 
 subdir('access')
+subdir('archive')
 subdir('backup')
 subdir('bootstrap')
 subdir('catalog')
diff --git a/src/backend/postmaster/Makefile b/src/backend/postmaster/Makefile
index 3a794e54d6..047448b34e 100644
--- a/src/backend/postmaster/Makefile
+++ b/src/backend/postmaster/Makefile
@@ -22,7 +22,6 @@ OBJS = \
 	interrupt.o \
 	pgarch.o \
 	postmaster.o \
-	shell_archive.o \
 	startup.o \
 	syslogger.o \
 	walwriter.o
diff --git a/src/backend/postmaster/meson.build b/src/backend/postmaster/meson.build
index 9079922de7..cda921fd10 100644
--- a/src/backend/postmaster/meson.build
+++ b/src/backend/postmaster/meson.build
@@ -10,7 +10,6 @@ backend_sources += files(
   'interrupt.c',
   'pgarch.c',
   'postmaster.c',
-  'shell_archive.c',
   'startup.c',
   'syslogger.c',
   'walwriter.c',
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index e551af2905..46af349564 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -31,6 +31,8 @@
 
 #include "access/xlog.h"
 #include "access/xlog_internal.h"
+#include "archive/archive_module.h"
+#include "archive/shell_archive.h"
 #include "lib/binaryheap.h"
 #include "libpq/pqsignal.h"
 #include "pgstat.h"
@@ -97,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -406,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveContext.check_configured_cb != NULL &&
-				!ArchiveContext.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveContext.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -814,7 +817,7 @@ HandlePgArchInterrupts(void)
 /*
  * LoadArchiveLibrary
  *
- * Loads the archiving callbacks into our local ArchiveContext.
+ * Loads the archiving callbacks into our local ArchiveCallbacks.
  */
 static void
 LoadArchiveLibrary(void)
@@ -827,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveContext, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -844,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveContext);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveContext.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -859,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveContext.shutdown_cb != NULL)
-		ArchiveContext.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index b46e3b8c55..91cb64602b 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -33,6 +33,7 @@
 #include "access/xlog_internal.h"
 #include "access/xlogprefetcher.h"
 #include "access/xlogrecovery.h"
+#include "archive/archive_module.h"
 #include "catalog/namespace.h"
 #include "catalog/storage.h"
 #include "commands/async.h"
diff --git a/src/include/archive/archive_module.h b/src/include/archive/archive_module.h
new file mode 100644
index 0000000000..92ced4b222
--- /dev/null
+++ b/src/include/archive/archive_module.h
@@ -0,0 +1,58 @@
+/*-------------------------------------------------------------------------
+ *
+ * archive_module.h
+ *		Exports for archive modules.
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * src/include/archive/archive_module.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef _ARCHIVE_MODULE_H
+#define _ARCHIVE_MODULE_H
+
+/*
+ * The value of the archive_library GUC.
+ */
+extern PGDLLIMPORT char *XLogArchiveLibrary;
+
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
+/*
+ * Archive module callbacks
+ *
+ * These callback functions should be defined by archive libraries and returned
+ * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
+ * For more information about the purpose of each callback, refer to the
+ * archive modules documentation.
+ */
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
+
+typedef struct ArchiveModuleCallbacks
+{
+	ArchiveStartupCB startup_cb;
+	ArchiveCheckConfiguredCB check_configured_cb;
+	ArchiveFileCB archive_file_cb;
+	ArchiveShutdownCB shutdown_cb;
+} ArchiveModuleCallbacks;
+
+/*
+ * Type of the shared library symbol _PG_archive_module_init that is looked
+ * up when loading an archive library.
+ */
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
+
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
+
+#endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/archive/shell_archive.h b/src/include/archive/shell_archive.h
new file mode 100644
index 0000000000..1c843c13c5
--- /dev/null
+++ b/src/include/archive/shell_archive.h
@@ -0,0 +1,22 @@
+/*-------------------------------------------------------------------------
+ *
+ * shell_archive.h
+ *		Exports for archiving via shell.
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * src/include/archive/shell_archive.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef _SHELL_ARCHIVE_H
+#define _SHELL_ARCHIVE_H
+
+/*
+ * Since the logic for archiving via a shell command is in the core server
+ * and does not need to be loaded via a shared library, it has a special
+ * initialization function.
+ */
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
+
+#endif							/* _SHELL_ARCHIVE_H */
diff --git a/src/include/postmaster/pgarch.h b/src/include/postmaster/pgarch.h
index bcd51dfad6..3bd4fac71e 100644
--- a/src/include/postmaster/pgarch.h
+++ b/src/include/postmaster/pgarch.h
@@ -33,43 +33,4 @@ extern void PgArchiverMain(void) pg_attribute_noreturn();
 extern void PgArchWakeup(void);
 extern void PgArchForceDirScan(void);
 
-/*
- * The value of the archive_library GUC.
- */
-extern PGDLLIMPORT char *XLogArchiveLibrary;
-
-/*
- * Archive module callbacks
- *
- * These callback functions should be defined by archive libraries and returned
- * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
- * For more information about the purpose of each callback, refer to the
- * archive modules documentation.
- */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
-
-typedef struct ArchiveModuleCallbacks
-{
-	ArchiveCheckConfiguredCB check_configured_cb;
-	ArchiveFileCB archive_file_cb;
-	ArchiveShutdownCB shutdown_cb;
-} ArchiveModuleCallbacks;
-
-/*
- * Type of the shared library symbol _PG_archive_module_init that is looked
- * up when loading an archive library.
- */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
-
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
-
-/*
- * Since the logic for archiving via a shell command is in the core server
- * and does not need to be loaded via a shared library, it has a special
- * initialization function.
- */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
-
 #endif							/* _PGARCH_H */
-- 
2.25.1


--tThc/1wpZn/ma/RB--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v13 1/1] restructure archive modules API
@ 2023-02-09 21:49  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-02-09 21:49 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c         | 85 ++++++++++++++++---
 doc/src/sgml/archive-modules.sgml             | 35 ++++++--
 src/backend/Makefile                          |  2 +-
 src/backend/archive/Makefile                  | 18 ++++
 src/backend/archive/meson.build               |  5 ++
 .../{postmaster => archive}/shell_archive.c   | 30 ++++---
 src/backend/meson.build                       |  1 +
 src/backend/postmaster/Makefile               |  1 -
 src/backend/postmaster/meson.build            |  1 -
 src/backend/postmaster/pgarch.c               | 27 +++---
 src/backend/utils/misc/guc_tables.c           |  1 +
 src/include/archive/archive_module.h          | 58 +++++++++++++
 src/include/archive/shell_archive.h           | 24 ++++++
 src/include/postmaster/pgarch.h               | 39 ---------
 14 files changed, 241 insertions(+), 86 deletions(-)
 create mode 100644 src/backend/archive/Makefile
 create mode 100644 src/backend/archive/meson.build
 rename src/backend/{postmaster => archive}/shell_archive.c (78%)
 create mode 100644 src/include/archive/archive_module.h
 create mode 100644 src/include/archive/shell_archive.h

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 36b7a4814a..d7c227a10b 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -30,9 +30,9 @@
 #include <sys/time.h>
 #include <unistd.h>
 
+#include "archive/archive_module.h"
 #include "common/int.h"
 #include "miscadmin.h"
-#include "postmaster/pgarch.h"
 #include "storage/copydir.h"
 #include "storage/fd.h"
 #include "utils/guc.h"
@@ -40,14 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+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,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = basic_archive_shutdown
+};
 
 /*
  * _PG_init
@@ -67,10 +80,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,11 +87,28 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
+{
+	return &basic_archive_callbacks;
+}
+
+/*
+ * basic_archive_startup
+ *
+ * Creates the module's memory context.
+ */
 void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+basic_archive_startup(ArchiveModuleState *state)
 {
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	BasicArchiveData *data;
+
+	data = (BasicArchiveData *) MemoryContextAllocZero(TopMemoryContext,
+													   sizeof(BasicArchiveData));
+	data->context = AllocSetContextCreate(TopMemoryContext,
+										  "basic_archive",
+										  ALLOCSET_DEFAULT_SIZES);
+	state->private_data = (void *) data;
 }
 
 /*
@@ -133,7 +159,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -144,10 +170,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+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
@@ -366,3 +394,34 @@ 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);
+
+	/*
+	 * 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 ef02051f7f..668eb34991 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,18 +47,22 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
-   individual actions.
+   <function>_PG_archive_module_init</function>.  The result of the function
+   must be a pointer to a struct of type
+   <structname>ArchiveModuleCallbacks</structname>, which contains everything
+   that the core code needs to know how to make use of the archive module.  The
+   return value needs to be of server lifetime, which is typically achieved by
+   defining it as a <literal>static const</literal> variable in global scope.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
@@ -73,6 +77,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module has state, it can use
+    <structfield>state->private_data</structfield> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +101,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +123,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -125,10 +143,11 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     process exits (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.
+    these situations.  If the archive module has state, this callback should
+    free it to avoid leaks.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 86e6dcb792..e4bf0fe9c0 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -17,7 +17,7 @@ subdir = src/backend
 top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 
-SUBDIRS = access backup bootstrap catalog parser commands executor \
+SUBDIRS = access archive backup bootstrap catalog parser commands executor \
 	foreign lib libpq \
 	main nodes optimizer partitioning port postmaster \
 	regex replication rewrite \
diff --git a/src/backend/archive/Makefile b/src/backend/archive/Makefile
new file mode 100644
index 0000000000..8d2860d0df
--- /dev/null
+++ b/src/backend/archive/Makefile
@@ -0,0 +1,18 @@
+#-------------------------------------------------------------------------
+#
+# Makefile--
+#    Makefile for src/backend/archive
+#
+# IDENTIFICATION
+#    src/backend/archive/Makefile
+#
+#-------------------------------------------------------------------------
+
+subdir = src/backend/archive
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
+
+OBJS = \
+	shell_archive.o
+
+include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/archive/meson.build b/src/backend/archive/meson.build
new file mode 100644
index 0000000000..e194282931
--- /dev/null
+++ b/src/backend/archive/meson.build
@@ -0,0 +1,5 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+backend_sources += files(
+  'shell_archive.c'
+)
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/archive/shell_archive.c
similarity index 78%
rename from src/backend/postmaster/shell_archive.c
rename to src/backend/archive/shell_archive.c
index 7771b951b7..48e8952c0c 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/archive/shell_archive.c
@@ -18,30 +18,36 @@
 #include <sys/wait.h>
 
 #include "access/xlog.h"
+#include "archive/archive_module.h"
+#include "archive/shell_archive.h"
 #include "common/percentrepl.h"
 #include "pgstat.h"
-#include "postmaster/pgarch.h"
 
-static bool shell_archive_configured(void);
-static bool shell_archive_file(const char *file, const char *path);
-static void shell_archive_shutdown(void);
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static void shell_archive_shutdown(ArchiveModuleState *state);
 
-void
-shell_archive_init(ArchiveModuleCallbacks *cb)
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(ArchiveModuleState *state, const char *file, const char *path)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -123,7 +129,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/backend/meson.build b/src/backend/meson.build
index b1db3ba75b..4fdd209b82 100644
--- a/src/backend/meson.build
+++ b/src/backend/meson.build
@@ -7,6 +7,7 @@ backend_link_with = [pgport_srv, common_srv]
 generated_backend_sources = []
 
 subdir('access')
+subdir('archive')
 subdir('backup')
 subdir('bootstrap')
 subdir('catalog')
diff --git a/src/backend/postmaster/Makefile b/src/backend/postmaster/Makefile
index 3a794e54d6..047448b34e 100644
--- a/src/backend/postmaster/Makefile
+++ b/src/backend/postmaster/Makefile
@@ -22,7 +22,6 @@ OBJS = \
 	interrupt.o \
 	pgarch.o \
 	postmaster.o \
-	shell_archive.o \
 	startup.o \
 	syslogger.o \
 	walwriter.o
diff --git a/src/backend/postmaster/meson.build b/src/backend/postmaster/meson.build
index 9079922de7..cda921fd10 100644
--- a/src/backend/postmaster/meson.build
+++ b/src/backend/postmaster/meson.build
@@ -10,7 +10,6 @@ backend_sources += files(
   'interrupt.c',
   'pgarch.c',
   'postmaster.c',
-  'shell_archive.c',
   'startup.c',
   'syslogger.c',
   'walwriter.c',
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index e551af2905..46af349564 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -31,6 +31,8 @@
 
 #include "access/xlog.h"
 #include "access/xlog_internal.h"
+#include "archive/archive_module.h"
+#include "archive/shell_archive.h"
 #include "lib/binaryheap.h"
 #include "libpq/pqsignal.h"
 #include "pgstat.h"
@@ -97,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -406,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveContext.check_configured_cb != NULL &&
-				!ArchiveContext.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveContext.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -814,7 +817,7 @@ HandlePgArchInterrupts(void)
 /*
  * LoadArchiveLibrary
  *
- * Loads the archiving callbacks into our local ArchiveContext.
+ * Loads the archiving callbacks into our local ArchiveCallbacks.
  */
 static void
 LoadArchiveLibrary(void)
@@ -827,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveContext, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -844,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveContext);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveContext.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -859,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveContext.shutdown_cb != NULL)
-		ArchiveContext.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index b46e3b8c55..91cb64602b 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -33,6 +33,7 @@
 #include "access/xlog_internal.h"
 #include "access/xlogprefetcher.h"
 #include "access/xlogrecovery.h"
+#include "archive/archive_module.h"
 #include "catalog/namespace.h"
 #include "catalog/storage.h"
 #include "commands/async.h"
diff --git a/src/include/archive/archive_module.h b/src/include/archive/archive_module.h
new file mode 100644
index 0000000000..92ced4b222
--- /dev/null
+++ b/src/include/archive/archive_module.h
@@ -0,0 +1,58 @@
+/*-------------------------------------------------------------------------
+ *
+ * archive_module.h
+ *		Exports for archive modules.
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * src/include/archive/archive_module.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef _ARCHIVE_MODULE_H
+#define _ARCHIVE_MODULE_H
+
+/*
+ * The value of the archive_library GUC.
+ */
+extern PGDLLIMPORT char *XLogArchiveLibrary;
+
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
+/*
+ * Archive module callbacks
+ *
+ * These callback functions should be defined by archive libraries and returned
+ * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
+ * For more information about the purpose of each callback, refer to the
+ * archive modules documentation.
+ */
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
+
+typedef struct ArchiveModuleCallbacks
+{
+	ArchiveStartupCB startup_cb;
+	ArchiveCheckConfiguredCB check_configured_cb;
+	ArchiveFileCB archive_file_cb;
+	ArchiveShutdownCB shutdown_cb;
+} ArchiveModuleCallbacks;
+
+/*
+ * Type of the shared library symbol _PG_archive_module_init that is looked
+ * up when loading an archive library.
+ */
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
+
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
+
+#endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/archive/shell_archive.h b/src/include/archive/shell_archive.h
new file mode 100644
index 0000000000..9de6f769f1
--- /dev/null
+++ b/src/include/archive/shell_archive.h
@@ -0,0 +1,24 @@
+/*-------------------------------------------------------------------------
+ *
+ * shell_archive.h
+ *		Exports for archiving via shell.
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * src/include/archive/shell_archive.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef _SHELL_ARCHIVE_H
+#define _SHELL_ARCHIVE_H
+
+#include "archive/archive_module.h"
+
+/*
+ * Since the logic for archiving via a shell command is in the core server
+ * and does not need to be loaded via a shared library, it has a special
+ * initialization function.
+ */
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
+
+#endif							/* _SHELL_ARCHIVE_H */
diff --git a/src/include/postmaster/pgarch.h b/src/include/postmaster/pgarch.h
index bcd51dfad6..3bd4fac71e 100644
--- a/src/include/postmaster/pgarch.h
+++ b/src/include/postmaster/pgarch.h
@@ -33,43 +33,4 @@ extern void PgArchiverMain(void) pg_attribute_noreturn();
 extern void PgArchWakeup(void);
 extern void PgArchForceDirScan(void);
 
-/*
- * The value of the archive_library GUC.
- */
-extern PGDLLIMPORT char *XLogArchiveLibrary;
-
-/*
- * Archive module callbacks
- *
- * These callback functions should be defined by archive libraries and returned
- * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
- * For more information about the purpose of each callback, refer to the
- * archive modules documentation.
- */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
-
-typedef struct ArchiveModuleCallbacks
-{
-	ArchiveCheckConfiguredCB check_configured_cb;
-	ArchiveFileCB archive_file_cb;
-	ArchiveShutdownCB shutdown_cb;
-} ArchiveModuleCallbacks;
-
-/*
- * Type of the shared library symbol _PG_archive_module_init that is looked
- * up when loading an archive library.
- */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
-
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
-
-/*
- * Since the logic for archiving via a shell command is in the core server
- * and does not need to be loaded via a shared library, it has a special
- * initialization function.
- */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
-
 #endif							/* _PGARCH_H */
-- 
2.25.1


--7AUc2qLy4jB3hD7Z--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* [PATCH v10 1/1] restructure archive modules API
@ 2023-02-09 21:49  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nathan Bossart @ 2023-02-09 21:49 UTC (permalink / raw)

---
 contrib/basic_archive/basic_archive.c   | 91 +++++++++++++++++++++----
 doc/src/sgml/archive-modules.sgml       | 35 +++++++---
 src/backend/postmaster/pgarch.c         | 27 +++++---
 src/backend/postmaster/shell_archive.c  | 34 +++++----
 src/backend/utils/misc/guc_tables.c     |  1 +
 src/include/postmaster/archive_module.h | 58 ++++++++++++++++
 src/include/postmaster/pgarch.h         | 39 -----------
 src/include/postmaster/shell_archive.h  | 24 +++++++
 8 files changed, 224 insertions(+), 85 deletions(-)
 create mode 100644 src/include/postmaster/archive_module.h
 create mode 100644 src/include/postmaster/shell_archive.h

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 36b7a4814a..e4742c9c94 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -32,7 +32,7 @@
 
 #include "common/int.h"
 #include "miscadmin.h"
-#include "postmaster/pgarch.h"
+#include "postmaster/archive_module.h"
 #include "storage/copydir.h"
 #include "storage/fd.h"
 #include "utils/guc.h"
@@ -40,14 +40,27 @@
 
 PG_MODULE_MAGIC;
 
+typedef struct BasicArchiveData
+{
+	MemoryContext context;
+} BasicArchiveData;
+
 static char *archive_directory = NULL;
-static MemoryContext basic_archive_context;
 
-static bool basic_archive_configured(void);
-static bool basic_archive_file(const char *file, const char *path);
+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,
+	.check_configured_cb = basic_archive_configured,
+	.archive_file_cb = basic_archive_file,
+	.shutdown_cb = basic_archive_shutdown
+};
 
 /*
  * _PG_init
@@ -67,10 +80,6 @@ _PG_init(void)
 							   check_archive_directory, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
-
-	basic_archive_context = AllocSetContextCreate(TopMemoryContext,
-												  "basic_archive",
-												  ALLOCSET_DEFAULT_SIZES);
 }
 
 /*
@@ -78,11 +87,28 @@ _PG_init(void)
  *
  * Returns the module's archiving callbacks.
  */
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void)
+{
+	return &basic_archive_callbacks;
+}
+
+/*
+ * basic_archive_startup
+ *
+ * Creates the module's memory context.
+ */
 void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+basic_archive_startup(ArchiveModuleState *state)
 {
-	cb->check_configured_cb = basic_archive_configured;
-	cb->archive_file_cb = basic_archive_file;
+	BasicArchiveData *data;
+
+	data = (BasicArchiveData *) MemoryContextAllocZero(TopMemoryContext,
+													   sizeof(BasicArchiveData));
+	data->context = AllocSetContextCreate(TopMemoryContext,
+										  "basic_archive",
+										  ALLOCSET_DEFAULT_SIZES);
+	state->private_data = (void *) data;
 }
 
 /*
@@ -133,7 +159,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(void)
+basic_archive_configured(ArchiveModuleState *state)
 {
 	return archive_directory != NULL && archive_directory[0] != '\0';
 }
@@ -144,10 +170,12 @@ basic_archive_configured(void)
  * Archives one file.
  */
 static bool
-basic_archive_file(const char *file, const char *path)
+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
@@ -366,3 +394,40 @@ 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;
+	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.
+	 */
+	data = (BasicArchiveData *) (state->private_data);
+	if (data == NULL)
+		return;
+
+	/*
+	 * If called within basic_archive_context, switch out of it before we
+	 * delete it.
+	 */
+	basic_archive_context = data->context;
+	if (CurrentMemoryContext == basic_archive_context)
+		MemoryContextSwitchTo(TopMemoryContext);
+
+	if (MemoryContextIsValid(basic_archive_context))
+		MemoryContextDelete(basic_archive_context);
+
+	/*
+	 * 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 ef02051f7f..668eb34991 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -47,18 +47,22 @@
    normal library search path is used to locate the library.  To provide the
    required archive module callbacks and to indicate that the library is
    actually an archive module, it needs to provide a function named
-   <function>_PG_archive_module_init</function>.  This function is passed a
-   struct that needs to be filled with the callback function pointers for
-   individual actions.
+   <function>_PG_archive_module_init</function>.  The result of the function
+   must be a pointer to a struct of type
+   <structname>ArchiveModuleCallbacks</structname>, which contains everything
+   that the core code needs to know how to make use of the archive module.  The
+   return value needs to be of server lifetime, which is typically achieved by
+   defining it as a <literal>static const</literal> variable in global scope.
 
 <programlisting>
 typedef struct ArchiveModuleCallbacks
 {
+    ArchiveStartupCB startup_cb;
     ArchiveCheckConfiguredCB check_configured_cb;
     ArchiveFileCB archive_file_cb;
     ArchiveShutdownCB shutdown_cb;
 } ArchiveModuleCallbacks;
-typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 </programlisting>
 
    Only the <function>archive_file_cb</function> callback is required.  The
@@ -73,6 +77,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
    The server will call them as required to process each individual WAL file.
   </para>
 
+  <sect2 id="archive-module-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is called shortly after the
+    module is loaded.  This callback can be used to perform any additional
+    initialization required.  If the archive module has state, it can use
+    <structfield>state->private_data</structfield> to store it.
+
+<programlisting>
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
   <sect2 id="archive-module-check">
    <title>Check Callback</title>
    <para>
@@ -83,7 +101,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (void);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -105,7 +123,7 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     single WAL file.
 
 <programlisting>
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 </programlisting>
 
     If <literal>true</literal> is returned, the server proceeds as if the file
@@ -125,10 +143,11 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
     process exits (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.
+    these situations.  If the archive module has state, this callback should
+    free it to avoid leaks.
 
 <programlisting>
-typedef void (*ArchiveShutdownCB) (void);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
 </programlisting>
    </para>
   </sect2>
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index e551af2905..99a07996f7 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -34,8 +34,10 @@
 #include "lib/binaryheap.h"
 #include "libpq/pqsignal.h"
 #include "pgstat.h"
+#include "postmaster/archive_module.h"
 #include "postmaster/interrupt.h"
 #include "postmaster/pgarch.h"
+#include "postmaster/shell_archive.h"
 #include "storage/fd.h"
 #include "storage/ipc.h"
 #include "storage/latch.h"
@@ -97,7 +99,8 @@ char	   *XLogArchiveLibrary = "";
  */
 static time_t last_sigterm_time = 0;
 static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static const ArchiveModuleCallbacks *ArchiveCallbacks;
+static ArchiveModuleState *archive_module_state;
 
 
 /*
@@ -406,8 +409,8 @@ pgarch_ArchiverCopyLoop(void)
 			HandlePgArchInterrupts();
 
 			/* can't do anything if not configured ... */
-			if (ArchiveContext.check_configured_cb != NULL &&
-				!ArchiveContext.check_configured_cb())
+			if (ArchiveCallbacks->check_configured_cb != NULL &&
+				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
 						(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +511,7 @@ pgarch_archiveXlog(char *xlog)
 	snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
 	set_ps_display(activitymsg);
 
-	ret = ArchiveContext.archive_file_cb(xlog, pathname);
+	ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
 	if (ret)
 		snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
 	else
@@ -814,7 +817,7 @@ HandlePgArchInterrupts(void)
 /*
  * LoadArchiveLibrary
  *
- * Loads the archiving callbacks into our local ArchiveContext.
+ * Loads the archiving callbacks into our local ArchiveCallbacks.
  */
 static void
 LoadArchiveLibrary(void)
@@ -827,8 +830,6 @@ LoadArchiveLibrary(void)
 				 errmsg("both archive_command and archive_library set"),
 				 errdetail("Only one of archive_command, archive_library may be set.")));
 
-	memset(&ArchiveContext, 0, sizeof(ArchiveModuleCallbacks));
-
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().
@@ -844,12 +845,16 @@ LoadArchiveLibrary(void)
 		ereport(ERROR,
 				(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
 
-	(*archive_init) (&ArchiveContext);
+	ArchiveCallbacks = (*archive_init) ();
 
-	if (ArchiveContext.archive_file_cb == NULL)
+	if (ArchiveCallbacks->archive_file_cb == NULL)
 		ereport(ERROR,
 				(errmsg("archive modules must register an archive callback")));
 
+	archive_module_state = (ArchiveModuleState *) palloc0(sizeof(ArchiveModuleState));
+	if (ArchiveCallbacks->startup_cb != NULL)
+		ArchiveCallbacks->startup_cb(archive_module_state);
+
 	before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
@@ -859,6 +864,6 @@ LoadArchiveLibrary(void)
 static void
 pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
-	if (ArchiveContext.shutdown_cb != NULL)
-		ArchiveContext.shutdown_cb();
+	if (ArchiveCallbacks->shutdown_cb != NULL)
+		ArchiveCallbacks->shutdown_cb(archive_module_state);
 }
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 7771b951b7..54b02ca152 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -20,28 +20,34 @@
 #include "access/xlog.h"
 #include "common/percentrepl.h"
 #include "pgstat.h"
-#include "postmaster/pgarch.h"
-
-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)
+#include "postmaster/archive_module.h"
+#include "postmaster/shell_archive.h"
+
+static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_file(ArchiveModuleState *state, const char *file, const char *path);
+static void shell_archive_shutdown(ArchiveModuleState *state);
+
+static const ArchiveModuleCallbacks shell_archive_callbacks = {
+	.startup_cb = NULL,
+	.check_configured_cb = shell_archive_configured,
+	.archive_file_cb = shell_archive_file,
+	.shutdown_cb = shell_archive_shutdown
+};
+
+const ArchiveModuleCallbacks *
+shell_archive_init(void)
 {
-	cb->check_configured_cb = shell_archive_configured;
-	cb->archive_file_cb = shell_archive_file;
-	cb->shutdown_cb = shell_archive_shutdown;
+	return &shell_archive_callbacks;
 }
 
 static bool
-shell_archive_configured(void)
+shell_archive_configured(ArchiveModuleState *state)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
 
 static bool
-shell_archive_file(const char *file, const char *path)
+shell_archive_file(ArchiveModuleState *state, const char *file, const char *path)
 {
 	char	   *xlogarchcmd;
 	char	   *nativePath = NULL;
@@ -123,7 +129,7 @@ shell_archive_file(const char *file, const char *path)
 }
 
 static void
-shell_archive_shutdown(void)
+shell_archive_shutdown(ArchiveModuleState *state)
 {
 	elog(DEBUG1, "archiver process shutting down");
 }
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index b46e3b8c55..c3ee83b691 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -52,6 +52,7 @@
 #include "parser/parse_expr.h"
 #include "parser/parser.h"
 #include "pgstat.h"
+#include "postmaster/archive_module.h"
 #include "postmaster/autovacuum.h"
 #include "postmaster/bgworker_internals.h"
 #include "postmaster/bgwriter.h"
diff --git a/src/include/postmaster/archive_module.h b/src/include/postmaster/archive_module.h
new file mode 100644
index 0000000000..79e9d53534
--- /dev/null
+++ b/src/include/postmaster/archive_module.h
@@ -0,0 +1,58 @@
+/*-------------------------------------------------------------------------
+ *
+ * archive_module.h
+ *		Exports for archive modules.
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * src/include/postmaster/archive_module.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef _ARCHIVE_MODULE_H
+#define _ARCHIVE_MODULE_H
+
+/*
+ * The value of the archive_library GUC.
+ */
+extern PGDLLIMPORT char *XLogArchiveLibrary;
+
+typedef struct ArchiveModuleState
+{
+	/*
+	 * Private data pointer for use by an archive module.  This can be used to
+	 * store state for the module that will be passed to each of its callbacks.
+	 */
+	void	   *private_data;
+} ArchiveModuleState;
+
+/*
+ * Archive module callbacks
+ *
+ * These callback functions should be defined by archive libraries and returned
+ * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
+ * For more information about the purpose of each callback, refer to the
+ * archive modules documentation.
+ */
+typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
+typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
+
+typedef struct ArchiveModuleCallbacks
+{
+	ArchiveStartupCB startup_cb;
+	ArchiveCheckConfiguredCB check_configured_cb;
+	ArchiveFileCB archive_file_cb;
+	ArchiveShutdownCB shutdown_cb;
+} ArchiveModuleCallbacks;
+
+/*
+ * Type of the shared library symbol _PG_archive_module_init that is looked
+ * up when loading an archive library.
+ */
+typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
+
+extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
+
+#endif							/* _ARCHIVE_MODULE_H */
diff --git a/src/include/postmaster/pgarch.h b/src/include/postmaster/pgarch.h
index bcd51dfad6..3bd4fac71e 100644
--- a/src/include/postmaster/pgarch.h
+++ b/src/include/postmaster/pgarch.h
@@ -33,43 +33,4 @@ extern void PgArchiverMain(void) pg_attribute_noreturn();
 extern void PgArchWakeup(void);
 extern void PgArchForceDirScan(void);
 
-/*
- * The value of the archive_library GUC.
- */
-extern PGDLLIMPORT char *XLogArchiveLibrary;
-
-/*
- * Archive module callbacks
- *
- * These callback functions should be defined by archive libraries and returned
- * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
- * For more information about the purpose of each callback, refer to the
- * archive modules documentation.
- */
-typedef bool (*ArchiveCheckConfiguredCB) (void);
-typedef bool (*ArchiveFileCB) (const char *file, const char *path);
-typedef void (*ArchiveShutdownCB) (void);
-
-typedef struct ArchiveModuleCallbacks
-{
-	ArchiveCheckConfiguredCB check_configured_cb;
-	ArchiveFileCB archive_file_cb;
-	ArchiveShutdownCB shutdown_cb;
-} ArchiveModuleCallbacks;
-
-/*
- * Type of the shared library symbol _PG_archive_module_init that is looked
- * up when loading an archive library.
- */
-typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb);
-
-extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb);
-
-/*
- * Since the logic for archiving via a shell command is in the core server
- * and does not need to be loaded via a shared library, it has a special
- * initialization function.
- */
-extern void shell_archive_init(ArchiveModuleCallbacks *cb);
-
 #endif							/* _PGARCH_H */
diff --git a/src/include/postmaster/shell_archive.h b/src/include/postmaster/shell_archive.h
new file mode 100644
index 0000000000..be83c7715a
--- /dev/null
+++ b/src/include/postmaster/shell_archive.h
@@ -0,0 +1,24 @@
+/*-------------------------------------------------------------------------
+ *
+ * shell_archive.h
+ *		Exports for archiving via shell.
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * src/include/postmaster/shell_archive.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef _SHELL_ARCHIVE_H
+#define _SHELL_ARCHIVE_H
+
+#include "postmaster/archive_module.h"
+
+/*
+ * Since the logic for archiving via a shell command is in the core server
+ * and does not need to be loaded via a shared library, it has a special
+ * initialization function.
+ */
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
+
+#endif							/* _SHELL_ARCHIVE_H */
-- 
2.25.1


--4Ckj6UjgE2iN1+kY--





^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: meson: Non-feature feature options
@ 2023-02-20 18:53  Peter Eisentraut <[email protected]>
  0 siblings, 3 replies; 33+ messages in thread

From: Peter Eisentraut @ 2023-02-20 18:53 UTC (permalink / raw)
  To: Nazir Bilal Yavuz <[email protected]>; Andres Freund <[email protected]>; +Cc: pgsql-hackers

On 20.02.23 13:33, Nazir Bilal Yavuz wrote:
> I added SSL and UUID patches. UUID patch has two different fixes:
> 
> 1 - v1-0002-meson-Refactor-UUID-option.patch: Adding 'auto' choice to 
> 'uuid' combo option.
> 
> 2 - v1-0002-meson-Refactor-UUID-option-with-uuid_library.patch: Making 
> 'uuid' feature option and adding new 'uuid_library' combo option with 
> ['auto', 'bsd', 'e2fs', 'ossp'] choices. If 'uuid_library' is set other 
> than 'auto' and it can't be found, build throws an error.
> 
> What do you think?

I like the second approach, with a 'uuid' feature option.  As you wrote 
earlier, adding an 'auto' choice to a combo option doesn't work fully 
like a real feature option.

But what does uuid_library=auto do?  Which one does it pick?  This is 
not a behavior we currently have, is it?

I would rename the ssl_type variable to ssl_library, so that if we ever 
expose that as an option, it would be consistent with uuid_library.







^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: meson: Non-feature feature options
@ 2023-02-20 19:10  Nazir Bilal Yavuz <[email protected]>
  parent: Peter Eisentraut <[email protected]>
  2 siblings, 0 replies; 33+ messages in thread

From: Nazir Bilal Yavuz @ 2023-02-20 19:10 UTC (permalink / raw)
  To: Peter Eisentraut <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers

Hi,

On Mon, 20 Feb 2023 at 21:53, Peter Eisentraut
<[email protected]> wrote:
>
> But what does uuid_library=auto do?  Which one does it pick?  This is
> not a behavior we currently have, is it?

Yes, we didn't have that behavior before. It checks uuid libs by the
order of 'e2fs', 'bsd' and 'ossp'. It uses the first one it finds and
doesn't try to find the rest but the build doesn't fail if it can't
find any library.

Regards,
Nazir Bilal Yavuz
Microsoft






^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: meson: Non-feature feature options
@ 2023-02-20 19:39  Daniel Gustafsson <[email protected]>
  parent: Peter Eisentraut <[email protected]>
  2 siblings, 0 replies; 33+ messages in thread

From: Daniel Gustafsson @ 2023-02-20 19:39 UTC (permalink / raw)
  To: Peter Eisentraut <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers

> On 20 Feb 2023, at 19:53, Peter Eisentraut <[email protected]> wrote:

> I would rename the ssl_type variable to ssl_library, so that if we ever expose that as an option, it would be consistent with uuid_library.

+1, ssl_library is a better name.

--
Daniel Gustafsson







^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: meson: Non-feature feature options
@ 2023-02-20 19:42  Andres Freund <[email protected]>
  parent: Peter Eisentraut <[email protected]>
  2 siblings, 1 reply; 33+ messages in thread

From: Andres Freund @ 2023-02-20 19:42 UTC (permalink / raw)
  To: Peter Eisentraut <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; pgsql-hackers

Hi,

On 2023-02-20 19:53:53 +0100, Peter Eisentraut wrote:
> On 20.02.23 13:33, Nazir Bilal Yavuz wrote:
> > I added SSL and UUID patches. UUID patch has two different fixes:
> > 
> > 1 - v1-0002-meson-Refactor-UUID-option.patch: Adding 'auto' choice to
> > 'uuid' combo option.
> > 
> > 2 - v1-0002-meson-Refactor-UUID-option-with-uuid_library.patch: Making
> > 'uuid' feature option and adding new 'uuid_library' combo option with
> > ['auto', 'bsd', 'e2fs', 'ossp'] choices. If 'uuid_library' is set other
> > than 'auto' and it can't be found, build throws an error.
> > 
> > What do you think?
> 
> I like the second approach, with a 'uuid' feature option.  As you wrote
> earlier, adding an 'auto' choice to a combo option doesn't work fully like a
> real feature option.

But we can make it behave exactly like one, by checking the auto_features
option.

Greetings,

Andres Freund






^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: meson: Non-feature feature options
@ 2023-02-21 16:32  Nazir Bilal Yavuz <[email protected]>
  parent: Andres Freund <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Nazir Bilal Yavuz @ 2023-02-21 16:32 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers

Hi,

On Mon, 20 Feb 2023 at 22:42, Andres Freund <[email protected]> wrote:
> On 2023-02-20 19:53:53 +0100, Peter Eisentraut wrote:
> > On 20.02.23 13:33, Nazir Bilal Yavuz wrote:
> > > I added SSL and UUID patches. UUID patch has two different fixes:
> > >
> > > 1 - v1-0002-meson-Refactor-UUID-option.patch: Adding 'auto' choice to
> > > 'uuid' combo option.
> > >
> > > 2 - v1-0002-meson-Refactor-UUID-option-with-uuid_library.patch: Making
> > > 'uuid' feature option and adding new 'uuid_library' combo option with
> > > ['auto', 'bsd', 'e2fs', 'ossp'] choices. If 'uuid_library' is set other
> > > than 'auto' and it can't be found, build throws an error.
> > >
> > > What do you think?
> >
> > I like the second approach, with a 'uuid' feature option.  As you wrote
> > earlier, adding an 'auto' choice to a combo option doesn't work fully like a
> > real feature option.
>
> But we can make it behave exactly like one, by checking the auto_features
> option.

Yes, we can set it like `uuidopt = get_option('auto_features')`.
However, if someone wants to set 'auto_features' to 'disabled' but
'uuid' to 'enabled'(to find at least one working uuid library); this
won't be possible. We can add 'enabled', 'disabled and 'auto' choices
to 'uuid' combo option to make all behaviours possible but adding
'uuid' feature option and 'uuid_library' combo option seems better to
me.

Regards,
Nazir Bilal Yavuz
Microsoft






^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: BF mamba failure
@ 2026-07-07 10:13  Kouber Saparev <[email protected]>
  0 siblings, 1 reply; 33+ messages in thread

From: Kouber Saparev @ 2026-07-07 10:13 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; [email protected]

We had this issue again, the database just stopped - PostgreSQL 17.6.

FATAL,XX000,"trying to drop stats entry already dropped: kind=relation
dboid=16420 objoid=3885511363 refcount=1 generation=0"

There is no such object present in the database.

Do you think the issue might be fixed in PostgreSQL 18.4?... Or PostgreSQL
19 eventually?

На ср, 17.09.2025 г. в 6:33 Michael Paquier <[email protected]> написа:

> On Tue, Sep 16, 2025 at 02:45:03PM +0300, Kouber Saparev wrote:
> > На пт, 12.09.2025 г. в 3:37 Michael Paquier <[email protected]>
> написа:
> >> With which process has this cascading standby been created?
> >> Does the workload of the primary involve a high consumption of OIDs
> >> for relations, say many temporary tables?
> >
> > Yes, we have around 150 entries added and deleted per second in pg_class,
> > and around 800 in pg_attribute. So something is actively creating and
> > dropping tables all the time.
>
> The number of relations would most likely matter, I doubt that
> pg_attribute contributes a lot in the frictions.  One of the test
> scenarios I am running is exactly that: a bunch of CREATE/DROP tables
> running concurrently with backends in a cascading standby accessing
> them, while tweaking the OID generated in the control file to force
> wraparounds.  I am failing to re-create your problem, for now at
> least.
>
> > > Another thing that may help is the WAL record history.  Are you for
> > > example seeing attempts to drop twice the same pgstats entry in WAL
> > > records?  Perhaps the origin of the problem is in this area.  A
> > > refcount of 2 is relevant, of course.
> > >
> >
> > How could we dig into this, i.e. inspecting such attempts in the WAL
> > records?
>
> Yeah, with pg_walinspect or pg_waldump, but I doubt that you retain
> enough WAL history to be able to ping at something specific.  One
> pattern I am looking for is duplicated drops initiated from the WAL
> records, when wraparound hits, or at least if there's a window small
> enough that two WAL records are generated and attempt to generate the
> same entry drop.  That's just an assumption at this stage.
>
>
> > Is it enough to upgrade the replicas or we need to upgrade the primary as
> > well?
>
> That would be both.  There are always reasons to update to the latest
> minor version of Postgres.  See the release notes for a bunch of them,
> that's usually convincing enough especially with CVEs addressed.  :)
> --
> Michael
>


^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: BF mamba failure
@ 2026-07-07 23:59  Michael Paquier <[email protected]>
  parent: Kouber Saparev <[email protected]>
  0 siblings, 1 reply; 33+ messages in thread

From: Michael Paquier @ 2026-07-07 23:59 UTC (permalink / raw)
  To: Kouber Saparev <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; [email protected]

On Tue, Jul 07, 2026 at 01:13:59PM +0300, Kouber Saparev wrote:
> We had this issue again, the database just stopped - PostgreSQL 17.6.
> 
> FATAL,XX000,"trying to drop stats entry already dropped: kind=relation
> dboid=16420 objoid=3885511363 refcount=1 generation=0"
> 
> There is no such object present in the database.
> 
> Do you think the issue might be fixed in PostgreSQL 18.4?... Or PostgreSQL
> 19 eventually?

Well, honestly, hard to say.  That's a case where a reproducible
scenario would be useful, as it seems to ve very workload dependent in
terms of handling of the relation entries..  (Aka I have not dug in
details how to do that.)

As one example, the buildfarm seems to be quite silent, but it would
help if we need something like a wraparound for better
reproducibility.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: BF mamba failure
@ 2026-07-08 10:53  Bertrand Drouvot <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 33+ messages in thread

From: Bertrand Drouvot @ 2026-07-08 10:53 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Kouber Saparev <[email protected]>; [email protected]

Hi,

On Wed, Jul 08, 2026 at 08:59:35AM +0900, Michael Paquier wrote:
> On Tue, Jul 07, 2026 at 01:13:59PM +0300, Kouber Saparev wrote:
> > We had this issue again, the database just stopped - PostgreSQL 17.6.
> > 
> > FATAL,XX000,"trying to drop stats entry already dropped: kind=relation
> > dboid=16420 objoid=3885511363 refcount=1 generation=0"
> > 
> > There is no such object present in the database.
> > 
> > Do you think the issue might be fixed in PostgreSQL 18.4?... Or PostgreSQL
> > 19 eventually?
> 
> Well, honestly, hard to say.  That's a case where a reproducible
> scenario would be useful, as it seems to ve very workload dependent in
> terms of handling of the relation entries..  (Aka I have not dug in
> details how to do that.)
> 
> As one example, the buildfarm seems to be quite silent, but it would
> help if we need something like a wraparound for better
> reproducibility.

So given that Kouber said that "we have around 150 entries added and deleted
per second in pg_class, and around 800 in pg_attribute. So something is actively
creating and dropping tables all the time.", OID reused is likely the culprit (
doable in months at this rate).

To try to simulate an OID reuse, I created an extension to manually set the next
OID and was able to reproduce the issue on 17.6, that way:

Primary: create table popo (a int);
Standby: select count(*) from popo; (keep this session open)
Primary: SELECT oid FROM pg_class WHERE relname = 'popo'; (17044 here)
Primary: drop table popo;
Primary: checkpoint;
Primary: vacuum;
Primary: SELECT set_next_oid(17044::oid);
Primary: create table popo2 (a int);
Primary: SELECT oid FROM pg_class WHERE relname = 'popo2'; (verify it reused 17044)
Primary: drop table popo2;

Produces:

2026-07-08 10:30:22.566 UTC [2096038] FATAL:  trying to drop stats entry already dropped: kind=relation dboid=5 objoid=17044 refcount=1 generation=0

So, the standby backend holds a local reference to the pgstat entry. When the
first DROP is replayed on the standby, the entry is marked dropped=true and refcount
goes from 2 to 1 so it can't be freed.
Then when the second DROP (for popo2, same OID) is replayed, pgstat_drop_entry_internal()
finds the entry with dropped=true and errors out.

The good news is that it does not reproduce on the 17 STABLE branch, I guess it's
due to 850b9218c8e being backpatched to stable branches. So a fix will be in the
next minor versions.

Michael, I wonder if that would make sense to add this "set_next_oid" kind of
thing to a contrib module to test this kind of OID reuse issue. Same idea as
xid_wraparound.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com






^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: BF mamba failure
@ 2026-07-08 22:45  Michael Paquier <[email protected]>
  parent: Bertrand Drouvot <[email protected]>
  0 siblings, 1 reply; 33+ messages in thread

From: Michael Paquier @ 2026-07-08 22:45 UTC (permalink / raw)
  To: Bertrand Drouvot <[email protected]>; +Cc: Kouber Saparev <[email protected]>; [email protected]

On Wed, Jul 08, 2026 at 10:53:47AM +0000, Bertrand Drouvot wrote:
> So given that Kouber said that "we have around 150 entries added and deleted
> per second in pg_class, and around 800 in pg_attribute. So something is actively
> creating and dropping tables all the time.", OID reused is likely the culprit (
> doable in months at this rate).

That would mean keeping around a session on the standby for long time.
With a connection pooler, why not.

> The good news is that it does not reproduce on the 17 STABLE branch, I guess it's
> due to 850b9218c8e being backpatched to stable branches. So a fix will be in the
> next minor versions.

Due to the decision of using missing_ok=true in the end-of-transaction
drops, that could indeed help.  I'd be curious to hear back after the
next minor release.  If I have nothing more to do, I'll take it.

> Michael, I wonder if that would make sense to add this "set_next_oid" kind of
> thing to a contrib module to test this kind of OID reuse issue. Same idea as
> xid_wraparound.

That's probably larger than just the OIDs.  Maybe something that's
able to do what pg_resetwal does for the control file could be useful.
Publishing another footgun sounds a bit annoying, but we also have
stuff like pg_surgery, that serve a similar purpose to shortcut the
backend.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: BF mamba failure
@ 2026-07-09 04:34  Bertrand Drouvot <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 33+ messages in thread

From: Bertrand Drouvot @ 2026-07-09 04:34 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Kouber Saparev <[email protected]>; [email protected]

Hi,

On Thu, Jul 09, 2026 at 07:45:54AM +0900, Michael Paquier wrote:
> On Wed, Jul 08, 2026 at 10:53:47AM +0000, Bertrand Drouvot wrote:
> Due to the decision of using missing_ok=true in the end-of-transaction
> drops, that could indeed help.  I'd be curious to hear back after the
> next minor release.

Yeah, Kouber do you think you could upgrade to 17.11 or 18.5? (current schedule
is August 13th, 2026 ([1])).

> > Michael, I wonder if that would make sense to add this "set_next_oid" kind of
> > thing to a contrib module to test this kind of OID reuse issue. Same idea as
> > xid_wraparound.
> 
> That's probably larger than just the OIDs.  Maybe something that's
> able to do what pg_resetwal does for the control file could be useful.
> Publishing another footgun sounds a bit annoying, but we also have
> stuff like pg_surgery, that serve a similar purpose to shortcut the
> backend.

Yeah. In addition to the OIDs I could think about the mxid (but I think they would
be better suited in xid_wraparound). The others look way more difficult (if doable)
to do outside pg_resetwal.

So I'd be tempted to just add an oid_wraparound one and we could add this test
case too. Thoughts?

[1]: https://www.postgresql.org/developer/roadmap/

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com






^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: BF mamba failure
@ 2026-07-10 12:08  Kouber Saparev <[email protected]>
  parent: Bertrand Drouvot <[email protected]>
  0 siblings, 1 reply; 33+ messages in thread

From: Kouber Saparev @ 2026-07-10 12:08 UTC (permalink / raw)
  To: Bertrand Drouvot <[email protected]>; +Cc: Michael Paquier <[email protected]>; [email protected]

На чт, 9.07.2026 г. в 7:34 Bertrand Drouvot <[email protected]>
написа:

>
> Yeah, Kouber do you think you could upgrade to 17.11 or 18.5? (current
> schedule
> is August 13th, 2026 ([1])).
>

We were planning an upgrade to 18 anyway, so we will just wait for the
release of 18.5 and give it a try, yes. And then hope for the best. The
last such sudden stop of the database we had was 10 months ago, so it would
be really hard to reproduce, but I will report (hopefully not) if it hits
us again after the upgrade.


^ permalink  raw  reply  [nested|flat] 33+ messages in thread

* Re: BF mamba failure
@ 2026-07-11 03:46  Michael Paquier <[email protected]>
  parent: Kouber Saparev <[email protected]>
  0 siblings, 0 replies; 33+ messages in thread

From: Michael Paquier @ 2026-07-11 03:46 UTC (permalink / raw)
  To: Kouber Saparev <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; [email protected]

On Fri, Jul 10, 2026 at 03:08:09PM +0300, Kouber Saparev wrote:
> We were planning an upgrade to 18 anyway, so we will just wait for the
> release of 18.5 and give it a try, yes. And then hope for the best. The
> last such sudden stop of the database we had was 10 months ago, so it would
> be really hard to reproduce, but I will report (hopefully not) if it hits
> us again after the upgrade.

Thanks a lot for that.  It may be possible that we are still missing
something, so a follow-up message if you see a failure would be
welcome.  Even if you see nothing, an update would also be nice but I
get that it may take a couple of weeks to manifest on your side based
on the symptoms you have described until now.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 33+ messages in thread


end of thread, other threads:[~2026-07-11 03:46 UTC | newest]

Thread overview: 33+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-12-19 03:43 [PATCH 3/6] Add primary_slot_name to init_from_backup in TAP test. Kyotaro Horiguchi <[email protected]>
2018-12-19 03:43 [PATCH 3/6] Add primary_slot_name to init_from_backup in TAP test. Kyotaro Horiguchi <[email protected]>
2019-06-07 09:07 Missing generated column in ALTER TABLE ADD COLUMN doc Masahiko Sawada <[email protected]>
2019-06-10 08:05 ` Re: Missing generated column in ALTER TABLE ADD COLUMN doc Michael Paquier <[email protected]>
2019-06-10 09:09   ` Re: Missing generated column in ALTER TABLE ADD COLUMN doc Masahiko Sawada <[email protected]>
2019-06-11 04:02     ` Re: Missing generated column in ALTER TABLE ADD COLUMN doc Michael Paquier <[email protected]>
2019-06-11 04:49       ` Re: Missing generated column in ALTER TABLE ADD COLUMN doc Masahiko Sawada <[email protected]>
2023-01-31 22:58 [PATCH v3 3/3] restructure archive modules API Nathan Bossart <[email protected]>
2023-01-31 22:58 [PATCH v5 3/3] restructure archive modules API Nathan Bossart <[email protected]>
2023-01-31 22:58 [PATCH v9 3/3] restructure archive modules API Nathan Bossart <[email protected]>
2023-01-31 22:58 [PATCH v2 3/3] restructure archive modules API Nathan Bossart <[email protected]>
2023-01-31 22:58 [PATCH v6 3/3] restructure archive modules API Nathan Bossart <[email protected]>
2023-01-31 22:58 [PATCH v7 3/4] restructure archive modules API Nathan Bossart <[email protected]>
2023-01-31 22:58 [PATCH v4 3/3] restructure archive modules API Nathan Bossart <[email protected]>
2023-01-31 22:58 [PATCH v8 4/4] restructure archive modules API Nathan Bossart <[email protected]>
2023-02-09 21:49 [PATCH v12 1/1] restructure archive modules API Nathan Bossart <[email protected]>
2023-02-09 21:49 [PATCH v13 1/1] restructure archive modules API Nathan Bossart <[email protected]>
2023-02-09 21:49 [PATCH v15 1/1] restructure archive modules API Nathan Bossart <[email protected]>
2023-02-09 21:49 [PATCH v11 1/1] restructure archive modules API Nathan Bossart <[email protected]>
2023-02-09 21:49 [PATCH v14 1/1] restructure archive modules API Nathan Bossart <[email protected]>
2023-02-09 21:49 [PATCH v10 1/1] restructure archive modules API Nathan Bossart <[email protected]>
2023-02-20 18:53 Re: meson: Non-feature feature options Peter Eisentraut <[email protected]>
2023-02-20 19:10 ` Re: meson: Non-feature feature options Nazir Bilal Yavuz <[email protected]>
2023-02-20 19:39 ` Re: meson: Non-feature feature options Daniel Gustafsson <[email protected]>
2023-02-20 19:42 ` Re: meson: Non-feature feature options Andres Freund <[email protected]>
2023-02-21 16:32   ` Re: meson: Non-feature feature options Nazir Bilal Yavuz <[email protected]>
2026-07-07 10:13 Re: BF mamba failure Kouber Saparev <[email protected]>
2026-07-07 23:59 ` Re: BF mamba failure Michael Paquier <[email protected]>
2026-07-08 10:53   ` Re: BF mamba failure Bertrand Drouvot <[email protected]>
2026-07-08 22:45     ` Re: BF mamba failure Michael Paquier <[email protected]>
2026-07-09 04:34       ` Re: BF mamba failure Bertrand Drouvot <[email protected]>
2026-07-10 12:08         ` Re: BF mamba failure Kouber Saparev <[email protected]>
2026-07-11 03:46           ` Re: BF mamba failure Michael Paquier <[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