public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v13 1/1] restructure archive modules API
6+ messages / 5 participants
[nested] [flat]
* [PATCH v13 1/1] restructure archive modules API
@ 2023-02-09 21:49 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 6+ 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] 6+ messages in thread
* Re: meson: Non-feature feature options
@ 2023-02-20 18:53 Peter Eisentraut <[email protected]>
0 siblings, 3 replies; 6+ messages in thread
From: Peter Eisentraut @ 2023-02-20 18:53 UTC (permalink / raw)
To: Nazir Bilal Yavuz <[email protected]>; Andres Freund <[email protected]>; +Cc: pgsql-hackers
On 20.02.23 13:33, Nazir Bilal Yavuz wrote:
> I added SSL and UUID patches. UUID patch has two different fixes:
>
> 1 - v1-0002-meson-Refactor-UUID-option.patch: Adding 'auto' choice to
> 'uuid' combo option.
>
> 2 - v1-0002-meson-Refactor-UUID-option-with-uuid_library.patch: Making
> 'uuid' feature option and adding new 'uuid_library' combo option with
> ['auto', 'bsd', 'e2fs', 'ossp'] choices. If 'uuid_library' is set other
> than 'auto' and it can't be found, build throws an error.
>
> What do you think?
I like the second approach, with a 'uuid' feature option. As you wrote
earlier, adding an 'auto' choice to a combo option doesn't work fully
like a real feature option.
But what does uuid_library=auto do? Which one does it pick? This is
not a behavior we currently have, is it?
I would rename the ssl_type variable to ssl_library, so that if we ever
expose that as an option, it would be consistent with uuid_library.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: meson: Non-feature feature options
@ 2023-02-20 19:10 Nazir Bilal Yavuz <[email protected]>
parent: Peter Eisentraut <[email protected]>
2 siblings, 0 replies; 6+ messages in thread
From: Nazir Bilal Yavuz @ 2023-02-20 19:10 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers
Hi,
On Mon, 20 Feb 2023 at 21:53, Peter Eisentraut
<[email protected]> wrote:
>
> But what does uuid_library=auto do? Which one does it pick? This is
> not a behavior we currently have, is it?
Yes, we didn't have that behavior before. It checks uuid libs by the
order of 'e2fs', 'bsd' and 'ossp'. It uses the first one it finds and
doesn't try to find the rest but the build doesn't fail if it can't
find any library.
Regards,
Nazir Bilal Yavuz
Microsoft
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: meson: Non-feature feature options
@ 2023-02-20 19:39 Daniel Gustafsson <[email protected]>
parent: Peter Eisentraut <[email protected]>
2 siblings, 0 replies; 6+ messages in thread
From: Daniel Gustafsson @ 2023-02-20 19:39 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
> On 20 Feb 2023, at 19:53, Peter Eisentraut <[email protected]> wrote:
> I would rename the ssl_type variable to ssl_library, so that if we ever expose that as an option, it would be consistent with uuid_library.
+1, ssl_library is a better name.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: meson: Non-feature feature options
@ 2023-02-20 19:42 Andres Freund <[email protected]>
parent: Peter Eisentraut <[email protected]>
2 siblings, 1 reply; 6+ messages in thread
From: Andres Freund @ 2023-02-20 19:42 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; pgsql-hackers
Hi,
On 2023-02-20 19:53:53 +0100, Peter Eisentraut wrote:
> On 20.02.23 13:33, Nazir Bilal Yavuz wrote:
> > I added SSL and UUID patches. UUID patch has two different fixes:
> >
> > 1 - v1-0002-meson-Refactor-UUID-option.patch: Adding 'auto' choice to
> > 'uuid' combo option.
> >
> > 2 - v1-0002-meson-Refactor-UUID-option-with-uuid_library.patch: Making
> > 'uuid' feature option and adding new 'uuid_library' combo option with
> > ['auto', 'bsd', 'e2fs', 'ossp'] choices. If 'uuid_library' is set other
> > than 'auto' and it can't be found, build throws an error.
> >
> > What do you think?
>
> I like the second approach, with a 'uuid' feature option. As you wrote
> earlier, adding an 'auto' choice to a combo option doesn't work fully like a
> real feature option.
But we can make it behave exactly like one, by checking the auto_features
option.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: meson: Non-feature feature options
@ 2023-02-21 16:32 Nazir Bilal Yavuz <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Nazir Bilal Yavuz @ 2023-02-21 16:32 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers
Hi,
On Mon, 20 Feb 2023 at 22:42, Andres Freund <[email protected]> wrote:
> On 2023-02-20 19:53:53 +0100, Peter Eisentraut wrote:
> > On 20.02.23 13:33, Nazir Bilal Yavuz wrote:
> > > I added SSL and UUID patches. UUID patch has two different fixes:
> > >
> > > 1 - v1-0002-meson-Refactor-UUID-option.patch: Adding 'auto' choice to
> > > 'uuid' combo option.
> > >
> > > 2 - v1-0002-meson-Refactor-UUID-option-with-uuid_library.patch: Making
> > > 'uuid' feature option and adding new 'uuid_library' combo option with
> > > ['auto', 'bsd', 'e2fs', 'ossp'] choices. If 'uuid_library' is set other
> > > than 'auto' and it can't be found, build throws an error.
> > >
> > > What do you think?
> >
> > I like the second approach, with a 'uuid' feature option. As you wrote
> > earlier, adding an 'auto' choice to a combo option doesn't work fully like a
> > real feature option.
>
> But we can make it behave exactly like one, by checking the auto_features
> option.
Yes, we can set it like `uuidopt = get_option('auto_features')`.
However, if someone wants to set 'auto_features' to 'disabled' but
'uuid' to 'enabled'(to find at least one working uuid library); this
won't be possible. We can add 'enabled', 'disabled and 'auto' choices
to 'uuid' combo option to make all behaviours possible but adding
'uuid' feature option and 'uuid_library' combo option seems better to
me.
Regards,
Nazir Bilal Yavuz
Microsoft
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-02-21 16:32 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-02-09 21:49 [PATCH v13 1/1] restructure archive modules API Nathan Bossart <[email protected]>
2023-02-20 18:53 Re: meson: Non-feature feature options Peter Eisentraut <[email protected]>
2023-02-20 19:10 ` Re: meson: Non-feature feature options Nazir Bilal Yavuz <[email protected]>
2023-02-20 19:39 ` Re: meson: Non-feature feature options Daniel Gustafsson <[email protected]>
2023-02-20 19:42 ` Re: meson: Non-feature feature options Andres Freund <[email protected]>
2023-02-21 16:32 ` Re: meson: Non-feature feature options Nazir Bilal Yavuz <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox