agora inbox for pgsql-hackers@postgresql.org  
help / color / mirror / Atom feed
[PATCH v7 3/4] restructure archive modules API
15+ messages / 2 participants
[nested] [flat]

* [PATCH v5 3/3] restructure archive modules API
@ 2023-01-31 22:58 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v6 3/3] restructure archive modules API
@ 2023-01-31 22:58 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v4 3/3] restructure archive modules API
@ 2023-01-31 22:58 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v2 3/3] restructure archive modules API
@ 2023-01-31 22:58 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v3 3/3] restructure archive modules API
@ 2023-01-31 22:58 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v8 4/4] restructure archive modules API
@ 2023-01-31 22:58 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v7 3/4] restructure archive modules API
@ 2023-01-31 22:58 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v9 3/3] restructure archive modules API
@ 2023-01-31 22:58 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v11 1/1] restructure archive modules API
@ 2023-02-09 21:49 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v15 1/1] restructure archive modules API
@ 2023-02-09 21:49 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v13 1/1] restructure archive modules API
@ 2023-02-09 21:49 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v10 1/1] restructure archive modules API
@ 2023-02-09 21:49 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v12 1/1] restructure archive modules API
@ 2023-02-09 21:49 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v14 1/1] restructure archive modules API
@ 2023-02-09 21:49 Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* [PATCH v49 1/7] Make index_concurrently_create_copy more general
@ 2026-03-24 18:02 Álvaro Herrera <alvherre@kurilemu.de>
  0 siblings, 0 replies; 15+ messages in thread

From: Álvaro Herrera @ 2026-03-24 18:02 UTC (permalink / raw)

Add a 'boolean concurrent' option, and make it work for both cases.
Also rename it to index_create_copy.

This allows it to be reused for other purposes -- specifically, for
REPACK CONCURRENTLY.

With the CONCURRENTLY option, REPACK cannot simply swap the heap file and
rebuild its indexes. Instead, it needs to build a separate set of indexes
(including system catalog entries) *before* the actual swap, to reduce the
time AccessExclusiveLock needs to be held for.  This approach is
different from what CREATE INDEX CONCURRENTLY does.

Per a suggestion from Mihail Nikalayeu.

Author: Antonin Houska <ah@cybertec.at>
Discussion: https://postgr.es/m/41104.1754922120@localhost
---
 src/backend/catalog/index.c      | 39 +++++++++++++++++++-------------
 src/backend/commands/indexcmds.c | 15 +++++++-----
 src/backend/nodes/makefuncs.c    |  9 ++++----
 src/include/catalog/index.h      |  7 +++---
 src/include/nodes/makefuncs.h    |  4 +++-
 5 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1ccfa687f05..b86ad73c626 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1289,17 +1289,17 @@ index_create(Relation heapRelation,
 }
 
 /*
- * index_concurrently_create_copy
+ * index_create_copy
  *
- * Create concurrently an index based on the definition of the one provided by
- * caller.  The index is inserted into catalogs and needs to be built later
- * on.  This is called during concurrent reindex processing.
+ * Create an index based on the definition of the one provided by caller.  The
+ * index is inserted into catalogs. If 'concurrently' is TRUE, it needs to be
+ * built later on; otherwise it's built immediately.
  *
  * "tablespaceOid" is the tablespace to use for this index.
  */
 Oid
-index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
-							   Oid tablespaceOid, const char *newName)
+index_create_copy(Relation heapRelation, bool concurrently,
+				  Oid oldIndexId, Oid tablespaceOid, const char *newName)
 {
 	Relation	indexRelation;
 	IndexInfo  *oldInfo,
@@ -1318,6 +1318,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 	List	   *indexColNames = NIL;
 	List	   *indexExprs = NIL;
 	List	   *indexPreds = NIL;
+	int			flags = 0;
 
 	indexRelation = index_open(oldIndexId, RowExclusiveLock);
 
@@ -1328,7 +1329,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 	 * Concurrent build of an index with exclusion constraints is not
 	 * supported.
 	 */
-	if (oldInfo->ii_ExclusionOps != NULL)
+	if (oldInfo->ii_ExclusionOps != NULL && concurrently)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("concurrent index creation for exclusion constraints is not supported")));
@@ -1384,9 +1385,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 	}
 
 	/*
-	 * Build the index information for the new index.  Note that rebuild of
-	 * indexes with exclusion constraints is not supported, hence there is no
-	 * need to fill all the ii_Exclusion* fields.
+	 * Build the index information for the new index.
 	 */
 	newInfo = makeIndexInfo(oldInfo->ii_NumIndexAttrs,
 							oldInfo->ii_NumIndexKeyAttrs,
@@ -1395,10 +1394,13 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 							indexPreds,
 							oldInfo->ii_Unique,
 							oldInfo->ii_NullsNotDistinct,
-							false,	/* not ready for inserts */
-							true,
+							!concurrently,	/* isready */
+							concurrently,	/* concurrent */
 							indexRelation->rd_indam->amsummarizing,
-							oldInfo->ii_WithoutOverlaps);
+							oldInfo->ii_WithoutOverlaps,
+							oldInfo->ii_ExclusionOps,
+							oldInfo->ii_ExclusionProcs,
+							oldInfo->ii_ExclusionStrats);
 
 	/*
 	 * Extract the list of column names and the column numbers for the new
@@ -1436,6 +1438,9 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 		stattargets[i].isnull = isnull;
 	}
 
+	if (concurrently)
+		flags = INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT;
+
 	/*
 	 * Now create the new index.
 	 *
@@ -1459,7 +1464,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 							  indcoloptions->values,
 							  stattargets,
 							  reloptionsDatum,
-							  INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT,
+							  flags,
 							  0,
 							  true, /* allow table to be a system catalog? */
 							  false,	/* is_internal? */
@@ -2453,7 +2458,8 @@ BuildIndexInfo(Relation index)
 					   indexStruct->indisready,
 					   false,
 					   index->rd_indam->amsummarizing,
-					   indexStruct->indisexclusion && indexStruct->indisunique);
+					   indexStruct->indisexclusion && indexStruct->indisunique,
+					   NULL, NULL, NULL);
 
 	/* fill in attribute numbers */
 	for (i = 0; i < numAtts; i++)
@@ -2513,7 +2519,8 @@ BuildDummyIndexInfo(Relation index)
 					   indexStruct->indisready,
 					   false,
 					   index->rd_indam->amsummarizing,
-					   indexStruct->indisexclusion && indexStruct->indisunique);
+					   indexStruct->indisexclusion && indexStruct->indisunique,
+					   NULL, NULL, NULL);
 
 	/* fill in attribute numbers */
 	for (i = 0; i < numAtts; i++)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 373e8234794..4a2d21915b1 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -244,7 +244,8 @@ CheckIndexCompatible(Oid oldId,
 	 */
 	indexInfo = makeIndexInfo(numberOfAttributes, numberOfAttributes,
 							  accessMethodId, NIL, NIL, false, false,
-							  false, false, amsummarizing, isWithoutOverlaps);
+							  false, false, amsummarizing, isWithoutOverlaps,
+							  NULL, NULL, NULL);
 	typeIds = palloc_array(Oid, numberOfAttributes);
 	collationIds = palloc_array(Oid, numberOfAttributes);
 	opclassIds = palloc_array(Oid, numberOfAttributes);
@@ -931,7 +932,8 @@ DefineIndex(ParseState *pstate,
 							  !concurrent,
 							  concurrent,
 							  amissummarizing,
-							  stmt->iswithoutoverlaps);
+							  stmt->iswithoutoverlaps,
+							  NULL, NULL, NULL);
 
 	typeIds = palloc_array(Oid, numberOfAttributes);
 	collationIds = palloc_array(Oid, numberOfAttributes);
@@ -3989,10 +3991,11 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
 			tablespaceid = indexRel->rd_rel->reltablespace;
 
 		/* Create new index definition based on given index */
-		newIndexId = index_concurrently_create_copy(heapRel,
-													idx->indexId,
-													tablespaceid,
-													concurrentName);
+		newIndexId = index_create_copy(heapRel,
+									   true,
+									   idx->indexId,
+									   tablespaceid,
+									   concurrentName);
 
 		/*
 		 * Now open the relation of the new index, a session-level lock is
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 3cd35c5c457..8d23aa917e5 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -834,7 +834,8 @@ IndexInfo *
 makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions,
 			  List *predicates, bool unique, bool nulls_not_distinct,
 			  bool isready, bool concurrent, bool summarizing,
-			  bool withoutoverlaps)
+			  bool withoutoverlaps, Oid *exclusion_ops, Oid *exclusion_procs,
+			  uint16 *exclusion_strats)
 {
 	IndexInfo  *n = makeNode(IndexInfo);
 
@@ -863,9 +864,9 @@ makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions,
 	n->ii_PredicateState = NULL;
 
 	/* exclusion constraints */
-	n->ii_ExclusionOps = NULL;
-	n->ii_ExclusionProcs = NULL;
-	n->ii_ExclusionStrats = NULL;
+	n->ii_ExclusionOps = exclusion_ops;
+	n->ii_ExclusionProcs = exclusion_procs;
+	n->ii_ExclusionStrats = exclusion_strats;
 
 	/* speculative inserts */
 	n->ii_UniqueOps = NULL;
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index a38e95bc0eb..ed9e4c37d27 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -101,10 +101,9 @@ extern Oid	index_create(Relation heapRelation,
 #define	INDEX_CONSTR_CREATE_REMOVE_OLD_DEPS	(1 << 4)
 #define	INDEX_CONSTR_CREATE_WITHOUT_OVERLAPS (1 << 5)
 
-extern Oid	index_concurrently_create_copy(Relation heapRelation,
-										   Oid oldIndexId,
-										   Oid tablespaceOid,
-										   const char *newName);
+extern Oid	index_create_copy(Relation heapRelation, bool concurrently,
+							  Oid oldIndexId, Oid tablespaceOid,
+							  const char *newName);
 
 extern void index_concurrently_build(Oid heapRelationId,
 									 Oid indexRelationId);
diff --git a/src/include/nodes/makefuncs.h b/src/include/nodes/makefuncs.h
index bf54d39feb0..40ec249a7a1 100644
--- a/src/include/nodes/makefuncs.h
+++ b/src/include/nodes/makefuncs.h
@@ -99,7 +99,9 @@ extern IndexInfo *makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid,
 								List *expressions, List *predicates,
 								bool unique, bool nulls_not_distinct,
 								bool isready, bool concurrent,
-								bool summarizing, bool withoutoverlaps);
+								bool summarizing, bool withoutoverlaps,
+								Oid *exclusion_ops, Oid *exclusion_procs,
+								uint16 *exclusion_strats);
 
 extern Node *makeStringConst(char *str, int location);
 extern DefElem *makeDefElem(char *name, Node *arg, int location);
-- 
2.47.3


--3n3vqr5y2jhknrqj
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v49-0002-Rename-cluster.c-h-repack.c-h.patch"



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


end of thread, other threads:[~2026-03-24 18:02 UTC | newest]

Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-01-31 22:58 [PATCH v5 3/3] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-01-31 22:58 [PATCH v6 3/3] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-01-31 22:58 [PATCH v4 3/3] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-01-31 22:58 [PATCH v2 3/3] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-01-31 22:58 [PATCH v3 3/3] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-01-31 22:58 [PATCH v8 4/4] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-01-31 22:58 [PATCH v7 3/4] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-01-31 22:58 [PATCH v9 3/3] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-02-09 21:49 [PATCH v11 1/1] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-02-09 21:49 [PATCH v15 1/1] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-02-09 21:49 [PATCH v13 1/1] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-02-09 21:49 [PATCH v10 1/1] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-02-09 21:49 [PATCH v12 1/1] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2023-02-09 21:49 [PATCH v14 1/1] restructure archive modules API Nathan Bossart <nathandbossart@gmail.com>
2026-03-24 18:02 [PATCH v49 1/7] Make index_concurrently_create_copy more general Álvaro Herrera <alvherre@kurilemu.de>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox