Re: recovery modules
rebased
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From c23ddbe1dac8b9a79db31ad67df423848e475905 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 14:28:53 -0800
Subject: [PATCH v23 1/5] introduce routine for checking mutually exclusive
string GUCs
---
src/backend/postmaster/pgarch.c | 8 +++-
src/backend/utils/misc/guc.c| 22 ++
src/include/utils/guc.h | 3 +++
3 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 02f91431f5..5f1a6f190d 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -912,11 +912,9 @@ LoadArchiveLibrary(void)
{
ArchiveModuleInit archive_init;
- if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-errmsg("both \"archive_command\" and
\"archive_library\" set"),
-errdetail("Only one of \"archive_command\",
\"archive_library\" may be set.")));
+ (void) CheckMutuallyExclusiveStringGUCs(XLogArchiveLibrary,
"archive_library",
+
XLogArchiveCommand, "archive_command",
+
ERROR);
/*
* If shell archiving is enabled, use our special initialization
function.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 547cecde24..05dc5303bc 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2659,6 +2659,28 @@ ReportGUCOption(struct config_generic *record)
pfree(val);
}
+/*
+ * If both parameters are set, emits a log message at 'elevel' and returns
+ * false. Otherwise, returns true.
+ */
+bool
+CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+const char
*p2val, const char *p2name,
+int elevel)
+{
+ if (p1val[0] != '\0' && p2val[0] != '\0')
+ {
+ ereport(elevel,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("both \"%s\" and \"%s\" set", p1name,
p2name),
+errdetail("Only one of \"%s\", \"%s\" may be
set.",
+ p1name, p2name)));
+ return false;
+ }
+
+ return true;
+}
+
/*
* Convert a value from one of the human-friendly units ("kB", "min" etc.)
* to the given base unit. 'value' and 'unit' are the input value and unit
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index e4a594b5e8..018bb7e55b 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -376,6 +376,9 @@ extern void RestrictSearchPath(void);
extern void AtEOXact_GUC(bool isCommit, int nestLevel);
extern void BeginReportingGUCOptions(void);
extern void ReportChangedGUCOptions(void);
+extern bool CheckMutuallyExclusiveStringGUCs(const char *p1val, const char
*p1name,
+
const char *p2val, const char *p2name,
+
int elevel);
extern void ParseLongOption(const char *string, char **name, char **value);
extern const char *get_config_unit_name(int flags);
extern bool parse_int(const char *value, int *result, int flags,
--
2.39.3 (Apple Git-146)
>From b35cd2dd0b360a50af7ab9175b2887646ba57f37 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 10:36:00 -0800
Subject: [PATCH v23 2/5] refactor code for restoring via shell
---
src/backend/Makefile | 2 +-
src/backend/access/transam/timeline.c | 12 +-
src/backend/access/transam/xlog.c | 50 -
src/backend/access/transam/xlogarchive.c | 167 ---
src/backend/access/transam/xlogrecovery.c | 3 +-
src/backend/meson.build | 1 +
src/backend/postmaster/startup.c | 16 +-
src/backend/restore/Makefile | 18 ++
src/backend/restore/meson.build | 5 +
src/backend/restore/shell_restore.c | 245 ++
src/include/Makefile | 2 +-
src/include/access/xlogarchive.h | 9 +-
src/include/meson.build | 1 +
src/include/postmaster/startup.h | 1 +
src/include/restore/shell_restore.h | 26 +++
src/tools/pgindent/typedefs.list | 1 +
16 files changed, 407 insertions(+), 152 deletions(-)
create mode 100644 src/backend/restore/Makefile
create mode 100644
Re: recovery modules
rebased
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 046d6e4b13d3a6b15df1245f3154969f7553594d Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 14:28:53 -0800
Subject: [PATCH v22 1/5] introduce routine for checking mutually exclusive
string GUCs
---
src/backend/postmaster/pgarch.c | 8 +++-
src/backend/utils/misc/guc.c| 22 ++
src/include/utils/guc.h | 3 +++
3 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index d82bcc2cfd..98a5aa3661 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -912,11 +912,9 @@ LoadArchiveLibrary(void)
{
ArchiveModuleInit archive_init;
- if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
- ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("both archive_command and archive_library set"),
- errdetail("Only one of archive_command, archive_library may be set.")));
+ (void) CheckMutuallyExclusiveStringGUCs(XLogArchiveLibrary, "archive_library",
+ XLogArchiveCommand, "archive_command",
+ ERROR);
/*
* If shell archiving is enabled, use our special initialization function.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 3fb6803998..02bc4d66e7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2659,6 +2659,28 @@ ReportGUCOption(struct config_generic *record)
pfree(val);
}
+/*
+ * If both parameters are set, emits a log message at 'elevel' and returns
+ * false. Otherwise, returns true.
+ */
+bool
+CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel)
+{
+ if (p1val[0] != '\0' && p2val[0] != '\0')
+ {
+ ereport(elevel,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot set both %s and %s", p1name, p2name),
+ errdetail("Only one of %s or %s may be set.",
+ p1name, p2name)));
+ return false;
+ }
+
+ return true;
+}
+
/*
* Convert a value from one of the human-friendly units ("kB", "min" etc.)
* to the given base unit. 'value' and 'unit' are the input value and unit
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index e4a594b5e8..018bb7e55b 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -376,6 +376,9 @@ extern void RestrictSearchPath(void);
extern void AtEOXact_GUC(bool isCommit, int nestLevel);
extern void BeginReportingGUCOptions(void);
extern void ReportChangedGUCOptions(void);
+extern bool CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel);
extern void ParseLongOption(const char *string, char **name, char **value);
extern const char *get_config_unit_name(int flags);
extern bool parse_int(const char *value, int *result, int flags,
--
2.25.1
>From 88e5e696792efdb62f7067400223c67357db6dba Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 10:36:00 -0800
Subject: [PATCH v22 2/5] refactor code for restoring via shell
---
src/backend/Makefile | 2 +-
src/backend/access/transam/timeline.c | 12 +-
src/backend/access/transam/xlog.c | 50 -
src/backend/access/transam/xlogarchive.c | 167 ---
src/backend/access/transam/xlogrecovery.c | 3 +-
src/backend/meson.build | 1 +
src/backend/postmaster/startup.c | 16 +-
src/backend/restore/Makefile | 18 ++
src/backend/restore/meson.build | 5 +
src/backend/restore/shell_restore.c | 245 ++
src/include/Makefile | 2 +-
src/include/access/xlogarchive.h | 9 +-
src/include/meson.build | 1 +
src/include/postmaster/startup.h | 1 +
src/include/restore/shell_restore.h | 26 +++
src/tools/pgindent/typedefs.list | 1 +
16 files changed, 407 insertions(+), 152 deletions(-)
create mode 100644 src/backend/restore/Makefile
create mode 100644 src/backend/restore/meson.build
create mode 100644 src/backend/restore/shell_restore.c
create mode 100644 src/include/restore/shell_restore.h
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 6700aec039..590b5002c0 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -19,7 +19,7 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = access archive backup bootstrap catalog parser commands executor \
foreign lib libpq \
main nodes optimizer partitioning port postmaster \
- regex replication rewrite \
+ regex replication restore rewrite \
statistics storage tcop tsearch utils $(top_builddir)/src/timezone \
jit
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index 146751ae37..3269547de7 100644
---
Re: recovery modules
rebased
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 5897631d5f09032565d92d5b8547baf3d24eef87 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 14:28:53 -0800
Subject: [PATCH v21 1/5] introduce routine for checking mutually exclusive
string GUCs
---
src/backend/postmaster/pgarch.c | 8 +++-
src/backend/utils/misc/guc.c| 22 ++
src/include/utils/guc.h | 3 +++
3 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 251f75e91d..d951089a79 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -913,11 +913,9 @@ LoadArchiveLibrary(void)
{
ArchiveModuleInit archive_init;
- if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
- ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("both archive_command and archive_library set"),
- errdetail("Only one of archive_command, archive_library may be set.")));
+ (void) CheckMutuallyExclusiveStringGUCs(XLogArchiveLibrary, "archive_library",
+ XLogArchiveCommand, "archive_command",
+ ERROR);
/*
* If shell archiving is enabled, use our special initialization function.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f51b3e0b50..b0118a516b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2659,6 +2659,28 @@ ReportGUCOption(struct config_generic *record)
pfree(val);
}
+/*
+ * If both parameters are set, emits a log message at 'elevel' and returns
+ * false. Otherwise, returns true.
+ */
+bool
+CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel)
+{
+ if (p1val[0] != '\0' && p2val[0] != '\0')
+ {
+ ereport(elevel,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot set both %s and %s", p1name, p2name),
+ errdetail("Only one of %s or %s may be set.",
+ p1name, p2name)));
+ return false;
+ }
+
+ return true;
+}
+
/*
* Convert a value from one of the human-friendly units ("kB", "min" etc.)
* to the given base unit. 'value' and 'unit' are the input value and unit
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 8d1fe04078..129484819c 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -377,6 +377,9 @@ extern void RestrictSearchPath(void);
extern void AtEOXact_GUC(bool isCommit, int nestLevel);
extern void BeginReportingGUCOptions(void);
extern void ReportChangedGUCOptions(void);
+extern bool CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel);
extern void ParseLongOption(const char *string, char **name, char **value);
extern const char *get_config_unit_name(int flags);
extern bool parse_int(const char *value, int *result, int flags,
--
2.25.1
>From a01d9bd2d6a93a16b38ed071066f6901dd968391 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 10:36:00 -0800
Subject: [PATCH v21 2/5] refactor code for restoring via shell
---
src/backend/Makefile | 2 +-
src/backend/access/transam/timeline.c | 12 +-
src/backend/access/transam/xlog.c | 50 -
src/backend/access/transam/xlogarchive.c | 167 ---
src/backend/access/transam/xlogrecovery.c | 3 +-
src/backend/meson.build | 1 +
src/backend/postmaster/startup.c | 16 +-
src/backend/restore/Makefile | 18 ++
src/backend/restore/meson.build | 5 +
src/backend/restore/shell_restore.c | 245 ++
src/include/Makefile | 2 +-
src/include/access/xlogarchive.h | 9 +-
src/include/meson.build | 1 +
src/include/postmaster/startup.h | 1 +
src/include/restore/shell_restore.h | 26 +++
src/tools/pgindent/typedefs.list | 1 +
16 files changed, 407 insertions(+), 152 deletions(-)
create mode 100644 src/backend/restore/Makefile
create mode 100644 src/backend/restore/meson.build
create mode 100644 src/backend/restore/shell_restore.c
create mode 100644 src/include/restore/shell_restore.h
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 6700aec039..590b5002c0 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -19,7 +19,7 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = access archive backup bootstrap catalog parser commands executor \
foreign lib libpq \
main nodes optimizer partitioning port postmaster \
- regex replication rewrite \
+ regex replication restore rewrite \
statistics storage tcop tsearch utils $(top_builddir)/src/timezone \
jit
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index 146751ae37..3269547de7 100644
---
Re: recovery modules
another rebase for cfbot
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 0ccdbb0a010830380e6ff4b7a052198d50f0680f Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 14:28:53 -0800
Subject: [PATCH v20 1/5] introduce routine for checking mutually exclusive
string GUCs
---
src/backend/postmaster/pgarch.c | 8 +++-
src/backend/utils/misc/guc.c| 22 ++
src/include/utils/guc.h | 3 +++
3 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index c266904b57..55e9a1796b 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -821,11 +821,9 @@ LoadArchiveLibrary(void)
{
ArchiveModuleInit archive_init;
- if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
- ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("both archive_command and archive_library set"),
- errdetail("Only one of archive_command, archive_library may be set.")));
+ (void) CheckMutuallyExclusiveStringGUCs(XLogArchiveLibrary, "archive_library",
+ XLogArchiveCommand, "archive_command",
+ ERROR);
/*
* If shell archiving is enabled, use our special initialization function.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 391866145e..ac507008ce 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2659,6 +2659,28 @@ ReportGUCOption(struct config_generic *record)
pfree(val);
}
+/*
+ * If both parameters are set, emits a log message at 'elevel' and returns
+ * false. Otherwise, returns true.
+ */
+bool
+CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel)
+{
+ if (p1val[0] != '\0' && p2val[0] != '\0')
+ {
+ ereport(elevel,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot set both %s and %s", p1name, p2name),
+ errdetail("Only one of %s or %s may be set.",
+ p1name, p2name)));
+ return false;
+ }
+
+ return true;
+}
+
/*
* Convert a value from one of the human-friendly units ("kB", "min" etc.)
* to the given base unit. 'value' and 'unit' are the input value and unit
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 3712aba09b..3088ada610 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -376,6 +376,9 @@ extern void RestrictSearchPath(void);
extern void AtEOXact_GUC(bool isCommit, int nestLevel);
extern void BeginReportingGUCOptions(void);
extern void ReportChangedGUCOptions(void);
+extern bool CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel);
extern void ParseLongOption(const char *string, char **name, char **value);
extern const char *get_config_unit_name(int flags);
extern bool parse_int(const char *value, int *result, int flags,
--
2.25.1
>From 9d36df03e0eece1a08b7e778e6d4c82b2228582a Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 10:36:00 -0800
Subject: [PATCH v20 2/5] refactor code for restoring via shell
---
src/backend/Makefile | 2 +-
src/backend/access/transam/timeline.c | 12 +-
src/backend/access/transam/xlog.c | 50 -
src/backend/access/transam/xlogarchive.c | 167 ---
src/backend/access/transam/xlogrecovery.c | 3 +-
src/backend/meson.build | 1 +
src/backend/postmaster/startup.c | 16 +-
src/backend/restore/Makefile | 18 ++
src/backend/restore/meson.build | 5 +
src/backend/restore/shell_restore.c | 245 ++
src/include/Makefile | 2 +-
src/include/access/xlogarchive.h | 9 +-
src/include/meson.build | 1 +
src/include/postmaster/startup.h | 1 +
src/include/restore/shell_restore.h | 26 +++
src/tools/pgindent/typedefs.list | 1 +
16 files changed, 407 insertions(+), 152 deletions(-)
create mode 100644 src/backend/restore/Makefile
create mode 100644 src/backend/restore/meson.build
create mode 100644 src/backend/restore/shell_restore.c
create mode 100644 src/include/restore/shell_restore.h
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 6700aec039..590b5002c0 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -19,7 +19,7 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = access archive backup bootstrap catalog parser commands executor \
foreign lib libpq \
main nodes optimizer partitioning port postmaster \
- regex replication rewrite \
+ regex replication restore rewrite \
statistics storage tcop tsearch utils $(top_builddir)/src/timezone \
jit
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index
Re: recovery modules
rebased for cfbot
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From d3f497906daf1c405059b2c292f1eeb5cfeb742b Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 14:28:53 -0800
Subject: [PATCH v19 1/5] introduce routine for checking mutually exclusive
string GUCs
---
src/backend/postmaster/pgarch.c | 8 +++-
src/backend/utils/misc/guc.c| 22 ++
src/include/utils/guc.h | 3 +++
3 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index f97035ca03..dcb5222bbe 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -815,11 +815,9 @@ LoadArchiveLibrary(void)
{
ArchiveModuleInit archive_init;
- if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
- ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("both archive_command and archive_library set"),
- errdetail("Only one of archive_command, archive_library may be set.")));
+ (void) CheckMutuallyExclusiveStringGUCs(XLogArchiveLibrary, "archive_library",
+ XLogArchiveCommand, "archive_command",
+ ERROR);
/*
* If shell archiving is enabled, use our special initialization function.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 391866145e..ac507008ce 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2659,6 +2659,28 @@ ReportGUCOption(struct config_generic *record)
pfree(val);
}
+/*
+ * If both parameters are set, emits a log message at 'elevel' and returns
+ * false. Otherwise, returns true.
+ */
+bool
+CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel)
+{
+ if (p1val[0] != '\0' && p2val[0] != '\0')
+ {
+ ereport(elevel,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot set both %s and %s", p1name, p2name),
+ errdetail("Only one of %s or %s may be set.",
+ p1name, p2name)));
+ return false;
+ }
+
+ return true;
+}
+
/*
* Convert a value from one of the human-friendly units ("kB", "min" etc.)
* to the given base unit. 'value' and 'unit' are the input value and unit
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 3712aba09b..3088ada610 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -376,6 +376,9 @@ extern void RestrictSearchPath(void);
extern void AtEOXact_GUC(bool isCommit, int nestLevel);
extern void BeginReportingGUCOptions(void);
extern void ReportChangedGUCOptions(void);
+extern bool CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel);
extern void ParseLongOption(const char *string, char **name, char **value);
extern const char *get_config_unit_name(int flags);
extern bool parse_int(const char *value, int *result, int flags,
--
2.25.1
>From a8e50ef80c91927d388b23b6e9eb034e046c02af Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 10:36:00 -0800
Subject: [PATCH v19 2/5] refactor code for restoring via shell
---
src/backend/Makefile | 2 +-
src/backend/access/transam/timeline.c | 12 +-
src/backend/access/transam/xlog.c | 50 -
src/backend/access/transam/xlogarchive.c | 167 ---
src/backend/access/transam/xlogrecovery.c | 3 +-
src/backend/meson.build | 1 +
src/backend/postmaster/startup.c | 16 +-
src/backend/restore/Makefile | 18 ++
src/backend/restore/meson.build | 5 +
src/backend/restore/shell_restore.c | 245 ++
src/include/Makefile | 2 +-
src/include/access/xlogarchive.h | 9 +-
src/include/meson.build | 1 +
src/include/postmaster/startup.h | 1 +
src/include/restore/shell_restore.h | 26 +++
src/tools/pgindent/typedefs.list | 1 +
16 files changed, 407 insertions(+), 152 deletions(-)
create mode 100644 src/backend/restore/Makefile
create mode 100644 src/backend/restore/meson.build
create mode 100644 src/backend/restore/shell_restore.c
create mode 100644 src/include/restore/shell_restore.h
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 3d7be09529..88e0e53f82 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -19,7 +19,7 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = access archive backup bootstrap catalog parser commands executor \
foreign lib libpq \
main nodes optimizer partitioning port postmaster \
- regex replication rewrite \
+ regex replication restore rewrite \
statistics storage tcop tsearch utils $(top_builddir)/src/timezone \
jit
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index 146751ae37..3269547de7
Re: recovery modules
Here is another rebase. Given the size of this patch set and the lack of
review, I am going to punt this one to v18.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From dd69e5987b477818f60b202af5fb9b2603dc8acb Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 14:28:53 -0800
Subject: [PATCH v18 1/5] introduce routine for checking mutually exclusive
string GUCs
---
src/backend/postmaster/pgarch.c | 8 +++-
src/backend/utils/misc/guc.c| 22 ++
src/include/utils/guc.h | 3 +++
3 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index f97035ca03..dcb5222bbe 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -815,11 +815,9 @@ LoadArchiveLibrary(void)
{
ArchiveModuleInit archive_init;
- if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
- ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("both archive_command and archive_library set"),
- errdetail("Only one of archive_command, archive_library may be set.")));
+ (void) CheckMutuallyExclusiveStringGUCs(XLogArchiveLibrary, "archive_library",
+ XLogArchiveCommand, "archive_command",
+ ERROR);
/*
* If shell archiving is enabled, use our special initialization function.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index dd5a46469a..b5c8e3702a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2660,6 +2660,28 @@ ReportGUCOption(struct config_generic *record)
pfree(val);
}
+/*
+ * If both parameters are set, emits a log message at 'elevel' and returns
+ * false. Otherwise, returns true.
+ */
+bool
+CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel)
+{
+ if (p1val[0] != '\0' && p2val[0] != '\0')
+ {
+ ereport(elevel,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot set both %s and %s", p1name, p2name),
+ errdetail("Only one of %s or %s may be set.",
+ p1name, p2name)));
+ return false;
+ }
+
+ return true;
+}
+
/*
* Convert a value from one of the human-friendly units ("kB", "min" etc.)
* to the given base unit. 'value' and 'unit' are the input value and unit
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 3712aba09b..3088ada610 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -376,6 +376,9 @@ extern void RestrictSearchPath(void);
extern void AtEOXact_GUC(bool isCommit, int nestLevel);
extern void BeginReportingGUCOptions(void);
extern void ReportChangedGUCOptions(void);
+extern bool CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel);
extern void ParseLongOption(const char *string, char **name, char **value);
extern const char *get_config_unit_name(int flags);
extern bool parse_int(const char *value, int *result, int flags,
--
2.25.1
>From 83a71392a83b7d04d50568e7c5466630528503a5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 10:36:00 -0800
Subject: [PATCH v18 2/5] refactor code for restoring via shell
---
src/backend/Makefile | 2 +-
src/backend/access/transam/timeline.c | 12 +-
src/backend/access/transam/xlog.c | 50 -
src/backend/access/transam/xlogarchive.c | 167 ---
src/backend/access/transam/xlogrecovery.c | 3 +-
src/backend/meson.build | 1 +
src/backend/postmaster/startup.c | 16 +-
src/backend/restore/Makefile | 18 ++
src/backend/restore/meson.build | 5 +
src/backend/restore/shell_restore.c | 245 ++
src/include/Makefile | 2 +-
src/include/access/xlogarchive.h | 9 +-
src/include/meson.build | 1 +
src/include/postmaster/startup.h | 1 +
src/include/restore/shell_restore.h | 26 +++
src/tools/pgindent/typedefs.list | 1 +
16 files changed, 407 insertions(+), 152 deletions(-)
create mode 100644 src/backend/restore/Makefile
create mode 100644 src/backend/restore/meson.build
create mode 100644 src/backend/restore/shell_restore.c
create mode 100644 src/include/restore/shell_restore.h
diff --git a/src/backend/Makefile b/src/backend/Makefile
index d66e2a4b9f..be00835995 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -19,7 +19,7 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = access archive backup bootstrap catalog parser commands executor \
foreign lib libpq \
main nodes optimizer partitioning port postmaster \
- regex replication rewrite \
+ regex replication restore rewrite \
statistics storage tcop tsearch utils $(top_builddir)/src/timezone \
jit
diff --git
Re: recovery modules
rebased
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 6cee7d220b886e9be309929da5274c5770e59845 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 14:28:53 -0800
Subject: [PATCH v17 1/5] introduce routine for checking mutually exclusive
string GUCs
---
src/backend/postmaster/pgarch.c | 8 +++-
src/backend/utils/misc/guc.c| 22 ++
src/include/utils/guc.h | 3 +++
3 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 67693b0580..6ae8bb53c4 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -824,11 +824,9 @@ LoadArchiveLibrary(void)
{
ArchiveModuleInit archive_init;
- if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
- ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("both archive_command and archive_library set"),
- errdetail("Only one of archive_command, archive_library may be set.")));
+ (void) CheckMutuallyExclusiveStringGUCs(XLogArchiveLibrary, "archive_library",
+ XLogArchiveCommand, "archive_command",
+ ERROR);
/*
* If shell archiving is enabled, use our special initialization function.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8f65ef3d89..9d21c16169 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2640,6 +2640,28 @@ ReportGUCOption(struct config_generic *record)
pfree(val);
}
+/*
+ * If both parameters are set, emits a log message at 'elevel' and returns
+ * false. Otherwise, returns true.
+ */
+bool
+CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel)
+{
+ if (p1val[0] != '\0' && p2val[0] != '\0')
+ {
+ ereport(elevel,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot set both %s and %s", p1name, p2name),
+ errdetail("Only one of %s or %s may be set.",
+ p1name, p2name)));
+ return false;
+ }
+
+ return true;
+}
+
/*
* Convert a value from one of the human-friendly units ("kB", "min" etc.)
* to the given base unit. 'value' and 'unit' are the input value and unit
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 471d53da8f..6543e61463 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -375,6 +375,9 @@ extern int NewGUCNestLevel(void);
extern void AtEOXact_GUC(bool isCommit, int nestLevel);
extern void BeginReportingGUCOptions(void);
extern void ReportChangedGUCOptions(void);
+extern bool CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel);
extern void ParseLongOption(const char *string, char **name, char **value);
extern const char *get_config_unit_name(int flags);
extern bool parse_int(const char *value, int *result, int flags,
--
2.25.1
>From 9f5c2602baa8e29058bc976c01516d04e3c1f901 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 10:36:00 -0800
Subject: [PATCH v17 2/5] refactor code for restoring via shell
---
src/backend/Makefile | 2 +-
src/backend/access/transam/timeline.c | 12 +-
src/backend/access/transam/xlog.c | 50 -
src/backend/access/transam/xlogarchive.c | 167 ---
src/backend/access/transam/xlogrecovery.c | 3 +-
src/backend/meson.build | 1 +
src/backend/postmaster/startup.c | 16 +-
src/backend/restore/Makefile | 18 ++
src/backend/restore/meson.build | 5 +
src/backend/restore/shell_restore.c | 245 ++
src/include/Makefile | 2 +-
src/include/access/xlogarchive.h | 9 +-
src/include/meson.build | 1 +
src/include/postmaster/startup.h | 1 +
src/include/restore/shell_restore.h | 26 +++
src/tools/pgindent/typedefs.list | 1 +
16 files changed, 407 insertions(+), 152 deletions(-)
create mode 100644 src/backend/restore/Makefile
create mode 100644 src/backend/restore/meson.build
create mode 100644 src/backend/restore/shell_restore.c
create mode 100644 src/include/restore/shell_restore.h
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 7d2ea7d54a..cbeb8ac93c 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -19,7 +19,7 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = access archive backup bootstrap catalog parser commands executor \
foreign lib libpq \
main nodes optimizer partitioning port postmaster \
- regex replication rewrite \
+ regex replication restore rewrite \
statistics storage tcop tsearch utils $(top_builddir)/src/timezone \
jit
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index 146751ae37..3269547de7 100644
---
Re: recovery modules
rebased
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 0934f23773a612051c36070ed1f3b8ab100c4c65 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 14:28:53 -0800
Subject: [PATCH v16 1/5] introduce routine for checking mutually exclusive
string GUCs
---
src/backend/postmaster/pgarch.c | 8 +++-
src/backend/utils/misc/guc.c| 22 ++
src/include/utils/guc.h | 3 +++
3 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 46af349564..d94730c50c 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -824,11 +824,9 @@ LoadArchiveLibrary(void)
{
ArchiveModuleInit archive_init;
- if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
- ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("both archive_command and archive_library set"),
- errdetail("Only one of archive_command, archive_library may be set.")));
+ (void) CheckMutuallyExclusiveStringGUCs(XLogArchiveLibrary, "archive_library",
+ XLogArchiveCommand, "archive_command",
+ ERROR);
/*
* If shell archiving is enabled, use our special initialization function.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 39d3775e80..847b62e161 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2638,6 +2638,28 @@ ReportGUCOption(struct config_generic *record)
pfree(val);
}
+/*
+ * If both parameters are set, emits a log message at 'elevel' and returns
+ * false. Otherwise, returns true.
+ */
+bool
+CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel)
+{
+ if (p1val[0] != '\0' && p2val[0] != '\0')
+ {
+ ereport(elevel,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot set both %s and %s", p1name, p2name),
+ errdetail("Only one of %s or %s may be set.",
+ p1name, p2name)));
+ return false;
+ }
+
+ return true;
+}
+
/*
* Convert a value from one of the human-friendly units ("kB", "min" etc.)
* to the given base unit. 'value' and 'unit' are the input value and unit
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 902e57c0ca..e4eef0da81 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -372,6 +372,9 @@ extern int NewGUCNestLevel(void);
extern void AtEOXact_GUC(bool isCommit, int nestLevel);
extern void BeginReportingGUCOptions(void);
extern void ReportChangedGUCOptions(void);
+extern bool CheckMutuallyExclusiveStringGUCs(const char *p1val, const char *p1name,
+ const char *p2val, const char *p2name,
+ int elevel);
extern void ParseLongOption(const char *string, char **name, char **value);
extern const char *get_config_unit_name(int flags);
extern bool parse_int(const char *value, int *result, int flags,
--
2.25.1
>From 7f6a8d792620b4d5ae097eb43f25be69f89f17c6 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 15 Feb 2023 10:36:00 -0800
Subject: [PATCH v16 2/5] refactor code for restoring via shell
---
src/backend/Makefile | 2 +-
src/backend/access/transam/timeline.c | 12 +-
src/backend/access/transam/xlog.c | 50 -
src/backend/access/transam/xlogarchive.c | 167 ---
src/backend/access/transam/xlogrecovery.c | 3 +-
src/backend/meson.build | 1 +
src/backend/postmaster/startup.c | 16 +-
src/backend/restore/Makefile | 18 ++
src/backend/restore/meson.build | 5 +
src/backend/restore/shell_restore.c | 245 ++
src/include/Makefile | 2 +-
src/include/access/xlogarchive.h | 9 +-
src/include/meson.build | 1 +
src/include/postmaster/startup.h | 1 +
src/include/restore/shell_restore.h | 26 +++
src/tools/pgindent/typedefs.list | 1 +
16 files changed, 407 insertions(+), 152 deletions(-)
create mode 100644 src/backend/restore/Makefile
create mode 100644 src/backend/restore/meson.build
create mode 100644 src/backend/restore/shell_restore.c
create mode 100644 src/include/restore/shell_restore.h
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 3e275ac759..6913601c36 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -19,7 +19,7 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = access archive backup bootstrap catalog parser commands executor \
foreign lib libpq \
main nodes optimizer partitioning port postmaster \
- regex replication rewrite \
+ regex replication restore rewrite \
statistics storage tcop tsearch utils $(top_builddir)/src/timezone \
jit
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index 94e152694e..b3e1c18c65 100644
---
Re: recovery modules
rebased
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From b8856e61d775bc248a292163facc0b227abdde97 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 23 Feb 2023 14:27:48 -0800
Subject: [PATCH v15 1/7] Move extra code out of the Pre/PostRestoreCommand()
block.
If SIGTERM is received within this block, the startup process will
immediately proc_exit() in the signal handler, so it is inadvisable
to include any more code than is required in this section. This
change moves the code recently added to this block (see 1b06d7b and
7fed801) to outside of the block. This ensures that only system()
is called while proc_exit() might be called in the SIGTERM handler,
which is how this code worked from v8.4 to v14.
---
src/backend/access/transam/xlogarchive.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index f3fb92c8f9..b998cd651c 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -159,20 +159,27 @@ RestoreArchivedFile(char *path, const char *xlogfname,
(errmsg_internal("executing restore command \"%s\"",
xlogRestoreCmd)));
+ fflush(NULL);
+ pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
+
/*
- * Check signals before restore command and reset afterwards.
+ * PreRestoreCommand() informs the SIGTERM handler for the startup process
+ * that it should proc_exit() right away. This is done for the duration of
+ * the system() call because there isn't a good way to break out while it
+ * is executing. Since we might call proc_exit() in a signal handler, it
+ * is best to put any additional logic before or after the
+ * PreRestoreCommand()/PostRestoreCommand() section.
*/
PreRestoreCommand();
/*
* Copy xlog from archival storage to XLOGDIR
*/
- fflush(NULL);
- pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
rc = system(xlogRestoreCmd);
- pgstat_report_wait_end();
PostRestoreCommand();
+
+ pgstat_report_wait_end();
pfree(xlogRestoreCmd);
if (rc == 0)
--
2.25.1
>From e8105f300a2e141c8b5db478a16bc29e029b30df Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 14 Feb 2023 09:44:53 -0800
Subject: [PATCH v15 2/7] Don't proc_exit() in startup's SIGTERM handler if
forked by system().
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 17 -
src/backend/storage/ipc/ipc.c| 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
src/backend/utils/error/elog.c | 28
src/include/utils/elog.h | 6 +-
5 files changed, 50 insertions(+), 6 deletions(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..0e7de26bc2 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,20 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ write_stderr_signal_safe("StartupProcShutdownHandler() called in child process\n");
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..d5097dc008 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == (int) getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..b9e2c3aafe 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
PGPROC *proc;
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc =
Re: recovery modules
On Fri, Feb 17, 2023 at 11:41:32AM -0800, Andres Freund wrote: > I don't fully now, it's not entirely clear to me what the goals here were. I > think you'd likely need to do a bit of infrastructure work to do this > sanely. So far we just didn't have the need to handle files being released in > a way like you want to do there. > > I suspect a good direction would be to use resource owners. Add a separate set > of functions that release files on resource owner release. Most of the > infrastructure is there already, for temporary files > (c.f. OpenTemporaryFile()). Yes, perhaps. I've had good experience with these when it comes to avoid leakages when releasing resources, particularly for resources allocated by external libraries (cough, OpenSSL, cough). And there was some work to make these more scalable, for example. At this stage of the CF, it seems pretty clear to me that this should be pushed to v17, so moved to next CF. -- Michael signature.asc Description: PGP signature
Re: recovery modules
On Tue, Mar 14, 2023 at 09:13:09PM -0700, Nathan Bossart wrote: > However, that's not what happens when hot_standby is off. In that case, > the postmaster.pid file is updated with PM_STATUS_STANDBY once recovery > starts, which wait_for_postmaster_start() interprets as "ready." I see > this was reported before [0], but that discussion fizzled out. IIUC it was > done this way to avoid infinite waits when hot_standby is off and standby > mode is enabled. I could be missing something obvious, but that doesn't > seem necessary when hot_standby is off and recovery mode is enabled because > recovery should end at some point (never mind the halting problem). I'm > still digging into this and may spin off a new thread if I can conjure up a > proposal. > > [0] > https://postgr.es/m/CAMkU%3D1wrMqPggnEfszE-c3PPLmKgRK17_qr7tmxBECYEbyV-4Q%40mail.gmail.com These days, knowing hot_standby is on by default, and that users would recover up to the end-of-backup record of just use read replicas, do we have a strong case for keeping this GUC parameter at all? It does not strike me that we really need to change a five-year-old behavior if there has been few complaints about it. I agree that it is confusing as it stands, but the long-term simplifications may be worth it in the recovery code (aka less booleans needed to track the flow of the startup process, and less confusion around that). -- Michael signature.asc Description: PGP signature
Re: recovery modules
I noticed that the new TAP test for basic_archive was failing
intermittently for cfbot. It looks like the query for checking that the
post-backup WAL is restored sometimes executes before archive recovery is
complete (because hot_standby is on). To fix this, I adjusted the test to
use poll_query_until instead. There are no other changes in v14.
I first tried to set hot_standby to off on the restored node so that the
query wouldn't run until archive recovery completed. This seemed like it
would work because start() useѕ "pg_ctl --wait", which has the following
note in the docs:
Startup is considered complete when the PID file indicates that the
server is ready to accept connections.
However, that's not what happens when hot_standby is off. In that case,
the postmaster.pid file is updated with PM_STATUS_STANDBY once recovery
starts, which wait_for_postmaster_start() interprets as "ready." I see
this was reported before [0], but that discussion fizzled out. IIUC it was
done this way to avoid infinite waits when hot_standby is off and standby
mode is enabled. I could be missing something obvious, but that doesn't
seem necessary when hot_standby is off and recovery mode is enabled because
recovery should end at some point (never mind the halting problem). I'm
still digging into this and may spin off a new thread if I can conjure up a
proposal.
[0]
https://postgr.es/m/CAMkU%3D1wrMqPggnEfszE-c3PPLmKgRK17_qr7tmxBECYEbyV-4Q%40mail.gmail.com
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From ef9aade7270d12104647439a99e3b1822393a318 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 23 Feb 2023 14:27:48 -0800
Subject: [PATCH v14 1/7] Move extra code out of the Pre/PostRestoreCommand()
block.
If SIGTERM is received within this block, the startup process will
immediately proc_exit() in the signal handler, so it is inadvisable
to include any more code than is required in this section. This
change moves the code recently added to this block (see 1b06d7b and
7fed801) to outside of the block. This ensures that only system()
is called while proc_exit() might be called in the SIGTERM handler,
which is how this code worked from v8.4 to v14.
---
src/backend/access/transam/xlogarchive.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index fcc87ff44f..41684418b6 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -159,20 +159,27 @@ RestoreArchivedFile(char *path, const char *xlogfname,
(errmsg_internal("executing restore command \"%s\"",
xlogRestoreCmd)));
+ fflush(NULL);
+ pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
+
/*
- * Check signals before restore command and reset afterwards.
+ * PreRestoreCommand() informs the SIGTERM handler for the startup process
+ * that it should proc_exit() right away. This is done for the duration of
+ * the system() call because there isn't a good way to break out while it
+ * is executing. Since we might call proc_exit() in a signal handler, it
+ * is best to put any additional logic before or after the
+ * PreRestoreCommand()/PostRestoreCommand() section.
*/
PreRestoreCommand();
/*
* Copy xlog from archival storage to XLOGDIR
*/
- fflush(NULL);
- pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
rc = system(xlogRestoreCmd);
- pgstat_report_wait_end();
PostRestoreCommand();
+
+ pgstat_report_wait_end();
pfree(xlogRestoreCmd);
if (rc == 0)
--
2.25.1
>From 78fdf06e4b1bbb37c8ae37937728aaeb4b2cf50b Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 14 Feb 2023 09:44:53 -0800
Subject: [PATCH v14 2/7] Don't proc_exit() in startup's SIGTERM handler if
forked by system().
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 17 -
src/backend/storage/ipc/ipc.c| 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
src/backend/utils/error/elog.c | 28
src/include/utils/elog.h | 6 +-
5 files changed, 50 insertions(+), 6 deletions(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..0e7de26bc2 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,20 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to
Re: recovery modules
Here is a rebased version of the restore modules patch set. I swapped the
patch for the stopgap fix for restore_command with the latest version [0],
and I marked the restore/ headers as installable (as was recently done for
archive/ [1]). There are no other changes.
[0] https://postgr.es/m/20230301224751.GA1823946%40nathanxps13
[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=6ad5793
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 1173c8b4e476575c3e4b410f3aa6220360c38503 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 23 Feb 2023 14:27:48 -0800
Subject: [PATCH v13 1/7] Move extra code out of the Pre/PostRestoreCommand()
block.
If SIGTERM is received within this block, the startup process will
immediately proc_exit() in the signal handler, so it is inadvisable
to include any more code than is required in this section. This
change moves the code recently added to this block (see 1b06d7b and
7fed801) to outside of the block. This ensures that only system()
is called while proc_exit() might be called in the SIGTERM handler,
which is how this code worked from v8.4 to v14.
---
src/backend/access/transam/xlogarchive.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index fcc87ff44f..41684418b6 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -159,20 +159,27 @@ RestoreArchivedFile(char *path, const char *xlogfname,
(errmsg_internal("executing restore command \"%s\"",
xlogRestoreCmd)));
+ fflush(NULL);
+ pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
+
/*
- * Check signals before restore command and reset afterwards.
+ * PreRestoreCommand() informs the SIGTERM handler for the startup process
+ * that it should proc_exit() right away. This is done for the duration of
+ * the system() call because there isn't a good way to break out while it
+ * is executing. Since we might call proc_exit() in a signal handler, it
+ * is best to put any additional logic before or after the
+ * PreRestoreCommand()/PostRestoreCommand() section.
*/
PreRestoreCommand();
/*
* Copy xlog from archival storage to XLOGDIR
*/
- fflush(NULL);
- pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
rc = system(xlogRestoreCmd);
- pgstat_report_wait_end();
PostRestoreCommand();
+
+ pgstat_report_wait_end();
pfree(xlogRestoreCmd);
if (rc == 0)
--
2.25.1
>From bdd7268075f150bd292050f74701568af8eb66ec Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 14 Feb 2023 09:44:53 -0800
Subject: [PATCH v13 2/7] Don't proc_exit() in startup's SIGTERM handler if
forked by system().
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 17 -
src/backend/storage/ipc/ipc.c| 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
src/backend/utils/error/elog.c | 28
src/include/utils/elog.h | 6 +-
5 files changed, 50 insertions(+), 6 deletions(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..0e7de26bc2 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,20 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ write_stderr_signal_safe("StartupProcShutdownHandler() called in child process\n");
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..d5097dc008 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == (int) getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..b9e2c3aafe 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+
Re: recovery modules
Here is a new revision of the restore modules patch set. In this patch
set, the interface looks similar to the recent archive modules redesign,
and there are separate callbacks for retrieving different types of files.
I've attempted to address all the feedback I've received, but there was a
lot scattered across different threads, so it's possible I've missed
something. Note that 0001 is the stopgap fix for restore_command that's
being tracked elsewhere [0]. I was careful to avoid repeating the recent
mistake with the SIGTERM handling.
This patch set is still a little rough around the edges, but I wanted to
post it in case folks had general thoughts about the structure, interface,
etc. This implementation restores files synchronously one-by-one just like
archive modules, but in the future, I would like to add
asynchronous/parallel/batching support. My intent is for this work to move
us closer to that.
[0] https://postgr.es/m/20230214174755.GA1348509%40nathanxps13
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 64651ae3c56160fea31d15a78f07c8c12950ec99 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 14 Feb 2023 09:44:53 -0800
Subject: [PATCH v12 1/6] stopgap fix for restore_command
---
src/backend/access/transam/xlogarchive.c | 15 +++
src/backend/postmaster/startup.c | 20 +++-
src/backend/storage/ipc/ipc.c| 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
4 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index fcc87ff44f..41684418b6 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -159,20 +159,27 @@ RestoreArchivedFile(char *path, const char *xlogfname,
(errmsg_internal("executing restore command \"%s\"",
xlogRestoreCmd)));
+ fflush(NULL);
+ pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
+
/*
- * Check signals before restore command and reset afterwards.
+ * PreRestoreCommand() informs the SIGTERM handler for the startup process
+ * that it should proc_exit() right away. This is done for the duration of
+ * the system() call because there isn't a good way to break out while it
+ * is executing. Since we might call proc_exit() in a signal handler, it
+ * is best to put any additional logic before or after the
+ * PreRestoreCommand()/PostRestoreCommand() section.
*/
PreRestoreCommand();
/*
* Copy xlog from archival storage to XLOGDIR
*/
- fflush(NULL);
- pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
rc = system(xlogRestoreCmd);
- pgstat_report_wait_end();
PostRestoreCommand();
+
+ pgstat_report_wait_end();
pfree(xlogRestoreCmd);
if (rc == 0)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..de2b56c2fa 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,23 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ const char msg[] = "StartupProcShutdownHandler() called in child process";
+ int rc pg_attribute_unused();
+
+ rc = write(STDERR_FILENO, msg, sizeof(msg));
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..6796cabc3e 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..ae845e8249 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+ Assert(MyProcPid == getpid()); /* not safe if forked by system(), etc. */
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
PGPROC *proc;
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ Assert(MyProcPid == getpid()); /* not safe if forked by system(), etc. */
auxproc = [proctype];
--
2.25.1
>From
Re: recovery modules
On Fri, Feb 17, 2023 at 05:01:47PM +0900, Michael Paquier wrote: > All that had better > be put into their own threads, IMO, to bring more visibility on these > subjects. I created a new thread for these [0]. > Saying that, I have spent more time on the revamped version of the > archive modules and it was already doing a lot, so I have applied > it as it covered all the points discussed.. Thanks! [0] https://postgr.es/m/20230217215624.GA3131134%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: recovery modules
Hi, On 2023-02-16 13:58:10 -0800, Nathan Bossart wrote: > On Thu, Feb 16, 2023 at 01:17:54PM -0800, Andres Freund wrote: > > I'm quite baffled by: > > /* Close any files left open by copy_file() or compare_files() > > */ > > AtEOSubXact_Files(false, InvalidSubTransactionId, > > InvalidSubTransactionId); > > > > in basic_archive_file(). It seems *really* off to call AtEOSubXact_Files() > > completely outside the context of a transaction environment. And it only > > does > > the thing you want because you pass parameters that aren't actually valid in > > the normal use in AtEOSubXact_Files(). I really don't understand how that's > > supposed to be ok. > > Hm. Should copy_file() and compare_files() have PG_FINALLY blocks that > attempt to close the files instead? What would you recommend? I don't fully now, it's not entirely clear to me what the goals here were. I think you'd likely need to do a bit of infrastructure work to do this sanely. So far we just didn't have the need to handle files being released in a way like you want to do there. I suspect a good direction would be to use resource owners. Add a separate set of functions that release files on resource owner release. Most of the infrastructure is there already, for temporary files (c.f. OpenTemporaryFile()). Then that resource owner could be reset in case of error. I'm not even sure that erroring out is a reasonable way to implement copy_file(), compare_files(), particularly because you want to return via a return code from basic_archive_files(). Greetings, Andres Freund
Re: recovery modules
On Thu, Feb 16, 2023 at 01:17:54PM -0800, Andres Freund wrote: > On 2023-02-16 12:15:12 -0800, Nathan Bossart wrote: >> On Thu, Feb 16, 2023 at 11:29:56AM -0800, Andres Freund wrote: >>> Not related the things changed here, but this should never have been pushed >>> down into individual archive modules. There's absolutely no way that we're >>> going to keep this up2date and working correctly in random archive >>> modules. And it would break if archive modules are ever called outside of >>> pgarch.c. Hmm, yes. That's a bad idea to copy the error handling stack. The maintenance of this code could quickly go wrong. All that had better be put into their own threads, IMO, to bring more visibility on these subjects. > I'm quite baffled by: > /* Close any files left open by copy_file() or compare_files() > */ > AtEOSubXact_Files(false, InvalidSubTransactionId, > InvalidSubTransactionId); > > in basic_archive_file(). It seems *really* off to call AtEOSubXact_Files() > completely outside the context of a transaction environment. And it only does > the thing you want because you pass parameters that aren't actually valid in > the normal use in AtEOSubXact_Files(). I really don't understand how that's > supposed to be ok. As does this part, probably with a backpatch.. Saying that, I have spent more time on the revamped version of the archive modules and it was already doing a lot, so I have applied it as it covered all the points discussed.. -- Michael signature.asc Description: PGP signature
Re: recovery modules
On Thu, Feb 16, 2023 at 01:17:54PM -0800, Andres Freund wrote:
> On 2023-02-16 12:15:12 -0800, Nathan Bossart wrote:
>> On Thu, Feb 16, 2023 at 11:29:56AM -0800, Andres Freund wrote:
>> > On 2023-02-15 10:44:07 -0800, Nathan Bossart wrote:
>> >> @@ -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;
>> >
>> > Not related the things changed here, but this should never have been pushed
>> > down into individual archive modules. There's absolutely no way that we're
>> > going to keep this up2date and working correctly in random archive
>> > modules. And it would break if archive modules are ever called outside of
>> > pgarch.c.
>>
>> Yeah. IIRC I did briefly try to avoid this, but the difficulty was that
>> each module will have its own custom cleanup logic.
>
> It can use PG_TRY/CATCH for that, if the top-level sigsetjmp is in pgarch.c.
> Or you could add a cleanup callback to the API, to be called after the
> top-level cleanup in pgarch.c.
Yeah, that seems workable.
> I'm quite baffled by:
> /* Close any files left open by copy_file() or compare_files()
> */
> AtEOSubXact_Files(false, InvalidSubTransactionId,
> InvalidSubTransactionId);
>
> in basic_archive_file(). It seems *really* off to call AtEOSubXact_Files()
> completely outside the context of a transaction environment. And it only does
> the thing you want because you pass parameters that aren't actually valid in
> the normal use in AtEOSubXact_Files(). I really don't understand how that's
> supposed to be ok.
Hm. Should copy_file() and compare_files() have PG_FINALLY blocks that
attempt to close the files instead? What would you recommend?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: recovery modules
Hi,
On 2023-02-16 12:15:12 -0800, Nathan Bossart wrote:
> Thanks for reviewing.
>
> On Thu, Feb 16, 2023 at 11:29:56AM -0800, Andres Freund wrote:
> > On 2023-02-15 10:44:07 -0800, Nathan Bossart wrote:
> >> @@ -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;
> >
> > Not related the things changed here, but this should never have been pushed
> > down into individual archive modules. There's absolutely no way that we're
> > going to keep this up2date and working correctly in random archive
> > modules. And it would break if archive modules are ever called outside of
> > pgarch.c.
>
> Yeah. IIRC I did briefly try to avoid this, but the difficulty was that
> each module will have its own custom cleanup logic.
It can use PG_TRY/CATCH for that, if the top-level sigsetjmp is in pgarch.c.
Or you could add a cleanup callback to the API, to be called after the
top-level cleanup in pgarch.c.
I'm quite baffled by:
/* Close any files left open by copy_file() or compare_files()
*/
AtEOSubXact_Files(false, InvalidSubTransactionId,
InvalidSubTransactionId);
in basic_archive_file(). It seems *really* off to call AtEOSubXact_Files()
completely outside the context of a transaction environment. And it only does
the thing you want because you pass parameters that aren't actually valid in
the normal use in AtEOSubXact_Files(). I really don't understand how that's
supposed to be ok.
Greetings,
Andres Freund
Re: recovery modules
Thanks for reviewing.
On Thu, Feb 16, 2023 at 11:29:56AM -0800, Andres Freund wrote:
> On 2023-02-15 10:44:07 -0800, Nathan Bossart wrote:
>> @@ -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;
>
> Not related the things changed here, but this should never have been pushed
> down into individual archive modules. There's absolutely no way that we're
> going to keep this up2date and working correctly in random archive
> modules. And it would break if archive modules are ever called outside of
> pgarch.c.
Yeah. IIRC I did briefly try to avoid this, but the difficulty was that
each module will have its own custom cleanup logic. There's no requirement
that a module creates an exception handler, but I imagine that any
sufficiently complex one will. In any case, I agree that it's worth trying
to pull this out of the individual modules.
>> +static void
>> +basic_archive_shutdown(ArchiveModuleState *state)
>> +{
>> +BasicArchiveData *data = (BasicArchiveData *) (state->private_data);
>
> The parens around (state->private_data) are imo odd.
Oops, removed.
>> +basic_archive_context = data->context;
>> +Assert(CurrentMemoryContext != basic_archive_context);
>> +
>> +if (MemoryContextIsValid(basic_archive_context))
>> +MemoryContextDelete(basic_archive_context);
>
> I guess I'd personally be paranoid and clean data->context after
> this. Obviously doesn't matter right now, but at some later date it could be
> that we'd error out after this point, and re-entered the shutdown callback.
Done.
>> +/*
>> + * 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;
>
> If you wanted you could just define the callback types in the struct now, as
> we don't need asserts for the types.
This crossed my mind. I thought it was nice to have a declaration for each
callback that we can copy into the docs, but I'm sure we could do without
it, too.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 5969db5576f9d475169f1b0e25fcb4d9d331c9f4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 9 Feb 2023 13:49:46 -0800
Subject: [PATCH v15 1/1] restructure archive modules API
---
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
#include
+#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
Re: recovery modules
Hi,
On 2023-02-15 10:44:07 -0800, Nathan Bossart wrote:
> On Wed, Feb 15, 2023 at 03:38:21PM +0900, Michael Paquier wrote:
> > I may tweak a bit the comments, but nothing more. And I don't think I
> > have more to add. Andres, do you have anything you would like to
> > mention?
Just some minor comments below. None of them needs to be addressed.
> @@ -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;
Not related the things changed here, but this should never have been pushed
down into individual archive modules. There's absolutely no way that we're
going to keep this up2date and working correctly in random archive
modules. And it would break if archive modules are ever called outside of
pgarch.c.
> +static void
> +basic_archive_shutdown(ArchiveModuleState *state)
> +{
> + BasicArchiveData *data = (BasicArchiveData *) (state->private_data);
The parens around (state->private_data) are imo odd.
> + basic_archive_context = data->context;
> + Assert(CurrentMemoryContext != basic_archive_context);
> +
> + if (MemoryContextIsValid(basic_archive_context))
> + MemoryContextDelete(basic_archive_context);
I guess I'd personally be paranoid and clean data->context after
this. Obviously doesn't matter right now, but at some later date it could be
that we'd error out after this point, and re-entered the shutdown callback.
> +
> +/*
> + * 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;
If you wanted you could just define the callback types in the struct now, as
we don't need asserts for the types.
Greetings,
Andres Freund
Re: recovery modules
On Wed, Feb 15, 2023 at 03:38:21PM +0900, Michael Paquier wrote:
> On Mon, Feb 13, 2023 at 05:02:37PM -0800, Nathan Bossart wrote:
>> Sorry for then noise, cfbot alerted me to a missing #include, which I've
>> added in v13.
Sorry for more noise. I noticed that I missed updating the IDENTIFICATION
line for shell_archive.c. That's the only change in v14.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 2f32fdd5cfd89600025000c1ddfee30b6f9103b4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 9 Feb 2023 13:49:46 -0800
Subject: [PATCH v14 1/1] restructure archive modules API
---
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
#include
+#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 _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;
Re: recovery modules
On Mon, Feb 13, 2023 at 05:02:37PM -0800, Nathan Bossart wrote: > Sorry for then noise, cfbot alerted me to a missing #include, which I've > added in v13. + basic_archive_context = data->context; + Assert(CurrentMemoryContext != basic_archive_context); So this is what it means to document that we are not in the memory context we are freeing here. That seems good enough to me in this context. Tracking if one of CurrentMemoryContext's parents is the memory context that would be deleted would be another thing, but this does not apply here. I may tweak a bit the comments, but nothing more. And I don't think I have more to add. Andres, do you have anything you would like to mention? -- Michael signature.asc Description: PGP signature
Re: recovery modules
On Mon, Feb 13, 2023 at 04:55:58PM -0800, Nathan Bossart wrote:
> Okay. I've added it back in v12 with the suggested adjustment for the
> memory context stuff.
Sorry for then noise, cfbot alerted me to a missing #include, which I've
added in v13.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 41619a415a5a544a8a5b1e16ccbadac69a997027 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 9 Feb 2023 13:49:46 -0800
Subject: [PATCH v13 1/1] restructure archive modules API
---
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
#include
+#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 _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
Re: recovery modules
On Mon, Feb 13, 2023 at 03:37:33PM -0800, Andres Freund wrote:
> On 2023-02-13 14:56:47 -0800, Nathan Bossart wrote:
>> On Mon, Feb 13, 2023 at 04:37:10PM +0900, Michael Paquier wrote:
>> > + basic_archive_context = data->context;
>> > + if (CurrentMemoryContext == basic_archive_context)
>> > + MemoryContextSwitchTo(TopMemoryContext);
>> > +
>> > + if (MemoryContextIsValid(basic_archive_context))
>> > + MemoryContextDelete(basic_archive_context);
>> >
>> > This is a bit confusing, because it means that we enter in the
>> > shutdown callback with one context, but exit it under
>> > TopMemoryContext. Are you sure that this will be OK when there could
>> > be multiple callbacks piled up with before_shmem_exit()? shmem_exit()
>> > has nothing specific to memory contexts.
>>
>> Well, we can't free the memory context while we are in it, so we have to
>> switch to another one. I agree that this is a bit confusing, though.
>
> Why would we be in that memory context? I'd just add an assert documenting
> we're not.
>
>
>> On second thought, I'm not sure it's important to make sure the state is
>> freed in the shutdown callback. It's only called just before the archiver
>> process exits, so we're not really at risk of leaking anything. I suppose
>> we might not always restart the archiver in this case, but I also don't
>> anticipate that behavior changing in the near future. I think this
>> callback is more useful for things like shutting down background workers.
>
> I think it's crucial. Otherwise we're just ossifying the design that there's
> just one archive module active at a time.
>
>
>> I went ahead and removed the shutdown callback from basic_archive and the
>> note about leaking from the documentation.
>
> -1
Okay. I've added it back in v12 with the suggested adjustment for the
memory context stuff.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 22e583fa5b41d820e3233d87ea1306ff81349574 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 9 Feb 2023 13:49:46 -0800
Subject: [PATCH v12 1/1] restructure archive modules API
---
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
#include
+#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 =
Re: recovery modules
Hi, On 2023-02-13 14:56:47 -0800, Nathan Bossart wrote: > On Mon, Feb 13, 2023 at 04:37:10PM +0900, Michael Paquier wrote: > > + basic_archive_context = data->context; > > + if (CurrentMemoryContext == basic_archive_context) > > + MemoryContextSwitchTo(TopMemoryContext); > > + > > + if (MemoryContextIsValid(basic_archive_context)) > > + MemoryContextDelete(basic_archive_context); > > > > This is a bit confusing, because it means that we enter in the > > shutdown callback with one context, but exit it under > > TopMemoryContext. Are you sure that this will be OK when there could > > be multiple callbacks piled up with before_shmem_exit()? shmem_exit() > > has nothing specific to memory contexts. > > Well, we can't free the memory context while we are in it, so we have to > switch to another one. I agree that this is a bit confusing, though. Why would we be in that memory context? I'd just add an assert documenting we're not. > On second thought, I'm not sure it's important to make sure the state is > freed in the shutdown callback. It's only called just before the archiver > process exits, so we're not really at risk of leaking anything. I suppose > we might not always restart the archiver in this case, but I also don't > anticipate that behavior changing in the near future. I think this > callback is more useful for things like shutting down background workers. I think it's crucial. Otherwise we're just ossifying the design that there's just one archive module active at a time. > I went ahead and removed the shutdown callback from basic_archive and the > note about leaking from the documentation. -1 Greetings, Andres Freund
Re: recovery modules
On Mon, Feb 13, 2023 at 04:37:10PM +0900, Michael Paquier wrote:
> + basic_archive_context = data->context;
> + if (CurrentMemoryContext == basic_archive_context)
> + MemoryContextSwitchTo(TopMemoryContext);
> +
> + if (MemoryContextIsValid(basic_archive_context))
> + MemoryContextDelete(basic_archive_context);
>
> This is a bit confusing, because it means that we enter in the
> shutdown callback with one context, but exit it under
> TopMemoryContext. Are you sure that this will be OK when there could
> be multiple callbacks piled up with before_shmem_exit()? shmem_exit()
> has nothing specific to memory contexts.
Well, we can't free the memory context while we are in it, so we have to
switch to another one. I agree that this is a bit confusing, though.
On second thought, I'm not sure it's important to make sure the state is
freed in the shutdown callback. It's only called just before the archiver
process exits, so we're not really at risk of leaking anything. I suppose
we might not always restart the archiver in this case, but I also don't
anticipate that behavior changing in the near future. I think this
callback is more useful for things like shutting down background workers.
I went ahead and removed the shutdown callback from basic_archive and the
note about leaking from the documentation.
> Is putting the new headers in src/include/postmaster/ the best move in
> the long term? Perhaps that's fine, but I was wondering whether a new
> location like archive/ would make more sense. pg_arch.h being in the
> postmaster section is fine.
I moved them to archive/.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 2d4231d40b47a01f46dc2d151661eba46b8ba3b6 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 9 Feb 2023 13:49:46 -0800
Subject: [PATCH v11 1/1] restructure archive modules API
---
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
#include
+#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 _archive_callbacks;
+}
Re: recovery modules
On Thu, Feb 09, 2023 at 02:48:26PM -0800, Nathan Bossart wrote: > On Thu, Feb 09, 2023 at 12:18:55PM -0800, Andres Freund wrote: >>> >>> -typedef bool (*ArchiveCheckConfiguredCB) (void); >>> +typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state); >>> >>> >>> If true is returned, the server will proceed with >> >> Hm. I wonder if ArchiveCheckConfiguredCB() should actually work without the >> state. We're not really doing anything yet, at that point, so it shouldn't >> really need state? >> >> The reason I'm wondering is that I think we should consider calling this from >> the GUC assignment hook, at least in postmaster. Which would make it more >> convenient to not have state, I think? > > I agree that this callback should typically not need the state, but I'm not > sure whether it fits into the assignment hook for archive_library. This > callback is primarily meant for situations when you have archiving enabled, > but your module isn't configured yet (e.g., archive_command is empty). In > this case, we keep the WAL around, but we don't try to archive it until > this hook returns true. It's up to each module to define that criteria. I > can imagine someone introducing a GUC in their archive module that > temporarily halts archiving via this callback. In that case, calling it > via a GUC assignment hook probably won't work. In fact, checking whether > archive_command is empty in that hook might not work either. Keeping the state in the configure check callback does not strike me as a problem, FWIW. >>> >>> -typedef void (*ArchiveShutdownCB) (void); >>> +typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state); >>> >>> >>> >> >> Perhaps mention that this needs to free state it allocated in the >> ArchiveModuleState, or it'll be leaked? > > done > > I left this out originally because the archiver exits shortly after calling > this. However, if you have DSM segments or something, it's probably wise > to make sure those are cleaned up. And I suppose we might not always exit > immediately after this callback, so establishing the habit of freeing the > state could be a good idea. In addition to updating this part of the docs, > I wrote a shutdown callback for basic_archive that frees its state. This makes sense to me. Still, DSM segments had better be cleaned up with dsm_backend_shutdown(). + basic_archive_context = data->context; + if (CurrentMemoryContext == basic_archive_context) + MemoryContextSwitchTo(TopMemoryContext); + + if (MemoryContextIsValid(basic_archive_context)) + MemoryContextDelete(basic_archive_context); This is a bit confusing, because it means that we enter in the shutdown callback with one context, but exit it under TopMemoryContext. Are you sure that this will be OK when there could be multiple callbacks piled up with before_shmem_exit()? shmem_exit() has nothing specific to memory contexts. Is putting the new headers in src/include/postmaster/ the best move in the long term? Perhaps that's fine, but I was wondering whether a new location like archive/ would make more sense. pg_arch.h being in the postmaster section is fine. -- Michael signature.asc Description: PGP signature
Re: recovery modules
On Thu, Feb 09, 2023 at 12:18:55PM -0800, Andres Freund wrote: > I think this nearly ready. Michael, are you planning to commit this? I could take a stab at it, now if you feel strongly about doing it yourselfof course feel free :) > Personally I'd probably squash these into a single commit. Same impression here. Agreed that all these had better be merged together, still keeping them separated made their review so much easier. -- Michael signature.asc Description: PGP signature
Re: recovery modules
On Thu, Feb 09, 2023 at 12:18:55PM -0800, Andres Freund wrote:
> On 2023-02-09 11:39:17 -0800, Nathan Bossart wrote:
> Personally I'd probably squash these into a single commit.
done
> I'd probably mention that this should typically be of server lifetime / a
> 'static const' struct. Tableam documents this as follows:
done
>> +
>> +
>> +archive_library is only loaded in the archiver
>> process.
>> +
>> +
>>
>
> That's not really related to any of the changes here, right?
>
> I'm not sure it's a good idea to document that. We e.g. probably should allow
> the library to check that the configuration is correct, at postmaster start,
> rather than later, at runtime.
removed
>> +
>> + Startup Callback
>> +
>> +The startup_cb 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 state->private_data to store it.
>
> s/needs to have a state/has state/?
done
>>
>> -typedef bool (*ArchiveCheckConfiguredCB) (void);
>> +typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
>>
>>
>> If true is returned, the server will proceed with
>
> Hm. I wonder if ArchiveCheckConfiguredCB() should actually work without the
> state. We're not really doing anything yet, at that point, so it shouldn't
> really need state?
>
> The reason I'm wondering is that I think we should consider calling this from
> the GUC assignment hook, at least in postmaster. Which would make it more
> convenient to not have state, I think?
I agree that this callback should typically not need the state, but I'm not
sure whether it fits into the assignment hook for archive_library. This
callback is primarily meant for situations when you have archiving enabled,
but your module isn't configured yet (e.g., archive_command is empty). In
this case, we keep the WAL around, but we don't try to archive it until
this hook returns true. It's up to each module to define that criteria. I
can imagine someone introducing a GUC in their archive module that
temporarily halts archiving via this callback. In that case, calling it
via a GUC assignment hook probably won't work. In fact, checking whether
archive_command is empty in that hook might not work either.
>>
>> -typedef void (*ArchiveShutdownCB) (void);
>> +typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
>>
>>
>>
>
> Perhaps mention that this needs to free state it allocated in the
> ArchiveModuleState, or it'll be leaked?
done
I left this out originally because the archiver exits shortly after calling
this. However, if you have DSM segments or something, it's probably wise
to make sure those are cleaned up. And I suppose we might not always exit
immediately after this callback, so establishing the habit of freeing the
state could be a good idea. In addition to updating this part of the docs,
I wrote a shutdown callback for basic_archive that frees its state.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From fab6305c344deb83c5b5c30ca2c09dbe1925a36c Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 9 Feb 2023 13:49:46 -0800
Subject: [PATCH v10 1/1] restructure archive modules API
---
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
Re: recovery modules
Hi, On 2023-02-09 11:39:17 -0800, Nathan Bossart wrote: > rebased for cfbot I think this nearly ready. Michael, are you planning to commit this? Personally I'd probably squash these into a single commit. > 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 > - _PG_archive_module_init. This function is passed a > - struct that needs to be filled with the callback function pointers for > - individual actions. > + _PG_archive_module_init. This function must return > an > + ArchiveModuleCallbacks struct filled with the > + callback function pointers for individual actions. I'd probably mention that this should typically be of server lifetime / a 'static const' struct. Tableam documents this as follows: The result of the function must be a pointer to a struct of type TableAmRoutine, which contains everything that the core code needs to know to make use of the table access method. The return value needs to be of server lifetime, which is typically achieved by defining it as a static const variable in global scope > + > + > + > +archive_library is only loaded in the archiver > process. > + > + > That's not really related to any of the changes here, right? I'm not sure it's a good idea to document that. We e.g. probably should allow the library to check that the configuration is correct, at postmaster start, rather than later, at runtime. > > @@ -73,6 +80,20 @@ typedef void (*ArchiveModuleInit) (struct > ArchiveModuleCallbacks *cb); > The server will call them as required to process each individual WAL file. > > > + > + Startup Callback > + > +The startup_cb 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 state->private_data to store it. s/needs to have a state/has state/? > @@ -83,7 +104,7 @@ typedef void (*ArchiveModuleInit) (struct > ArchiveModuleCallbacks *cb); > assumes the module is configured. > > > -typedef bool (*ArchiveCheckConfiguredCB) (void); > +typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state); > > > If true is returned, the server will proceed with Hm. I wonder if ArchiveCheckConfiguredCB() should actually work without the state. We're not really doing anything yet, at that point, so it shouldn't really need state? The reason I'm wondering is that I think we should consider calling this from the GUC assignment hook, at least in postmaster. Which would make it more convenient to not have state, I think? > @@ -128,7 +149,7 @@ typedef bool (*ArchiveFileCB) (const char *file, const > char *path); > these situations. > > > -typedef void (*ArchiveShutdownCB) (void); > +typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state); > > > Perhaps mention that this needs to free state it allocated in the ArchiveModuleState, or it'll be leaked? Greetings, Andres Freund
Re: recovery modules
rebased for cfbot
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From ea6339276c6863f260572dfc816f9dd27ac7b516 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:01:22 -0800
Subject: [PATCH v9 1/3] s/ArchiveContext/ArchiveCallbacks
---
src/backend/postmaster/pgarch.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index e551af2905..065d7d1313 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -97,7 +97,7 @@ char *XLogArchiveLibrary = "";
*/
static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static ArchiveModuleCallbacks ArchiveCallbacks;
/*
@@ -406,8 +406,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())
{
ereport(WARNING,
(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +508,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(xlog, pathname);
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
@@ -814,7 +814,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,7 +827,7 @@ LoadArchiveLibrary(void)
errmsg("both archive_command and archive_library set"),
errdetail("Only one of archive_command, archive_library may be set.")));
- memset(, 0, sizeof(ArchiveModuleCallbacks));
+ memset(, 0, sizeof(ArchiveModuleCallbacks));
/*
* If shell archiving is enabled, use our special initialization function.
@@ -844,9 +844,9 @@ LoadArchiveLibrary(void)
ereport(ERROR,
(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
- (*archive_init) ();
+ (*archive_init) ();
- if (ArchiveContext.archive_file_cb == NULL)
+ if (ArchiveCallbacks.archive_file_cb == NULL)
ereport(ERROR,
(errmsg("archive modules must register an archive callback")));
@@ -859,6 +859,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();
}
--
2.25.1
>From e4e28ed2aea395e5120389e92d6944c48468208e Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:16:34 -0800
Subject: [PATCH v9 2/3] move archive module exports to dedicated headers
---
contrib/basic_archive/basic_archive.c | 2 +-
src/backend/postmaster/pgarch.c | 2 ++
src/backend/postmaster/shell_archive.c | 3 +-
src/backend/utils/misc/guc_tables.c | 1 +
src/include/postmaster/archive_module.h | 47 +
src/include/postmaster/pgarch.h | 39
src/include/postmaster/shell_archive.h | 24 +
7 files changed, 77 insertions(+), 41 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..384336884d 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"
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 065d7d1313..281d9fd8b7 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"
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 7771b951b7..0f0558e155 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -20,7 +20,8 @@
#include "access/xlog.h"
#include "common/percentrepl.h"
#include "pgstat.h"
-#include "postmaster/pgarch.h"
+#include
Re: recovery modules
On Wed, Feb 08, 2023 at 09:16:19PM -0800, Andres Freund wrote: > Pushed. Thanks! Thanks! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: recovery modules
On 2023-02-08 10:55:44 -0800, Nathan Bossart wrote: > done Pushed. Thanks!
Re: recovery modules
On Wed, Feb 08, 2023 at 10:24:18AM -0800, Andres Freund wrote:
> If you'd reorder it so that 0004 applies independently from the other changes,
> I'd just push that now.
done
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 688566e23519f173a805696e4f00345f30d5d8e6 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 8 Feb 2023 09:51:46 -0800
Subject: [PATCH v8 1/4] remove unnecessary assertions for functions with
central declarations
---
contrib/basic_archive/basic_archive.c | 2 --
contrib/test_decoding/test_decoding.c | 2 --
src/backend/postmaster/shell_archive.c | 2 --
src/backend/replication/pgoutput/pgoutput.c | 2 --
4 files changed, 8 deletions(-)
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 3d29711a31..36b7a4814a 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -81,8 +81,6 @@ _PG_init(void)
void
_PG_archive_module_init(ArchiveModuleCallbacks *cb)
{
- AssertVariableIsOfType(&_PG_archive_module_init, ArchiveModuleInit);
-
cb->check_configured_cb = basic_archive_configured;
cb->archive_file_cb = basic_archive_file;
}
diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c
index e523d22eba..b7e6048647 100644
--- a/contrib/test_decoding/test_decoding.c
+++ b/contrib/test_decoding/test_decoding.c
@@ -127,8 +127,6 @@ _PG_init(void)
void
_PG_output_plugin_init(OutputPluginCallbacks *cb)
{
- AssertVariableIsOfType(&_PG_output_plugin_init, LogicalOutputPluginInit);
-
cb->startup_cb = pg_decode_startup;
cb->begin_cb = pg_decode_begin_txn;
cb->change_cb = pg_decode_change;
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 806b81c3f2..7771b951b7 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -29,8 +29,6 @@ static void shell_archive_shutdown(void);
void
shell_archive_init(ArchiveModuleCallbacks *cb)
{
- AssertVariableIsOfType(_archive_init, ArchiveModuleInit);
-
cb->check_configured_cb = shell_archive_configured;
cb->archive_file_cb = shell_archive_file;
cb->shutdown_cb = shell_archive_shutdown;
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 73b080060d..98377c094b 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -248,8 +248,6 @@ static void pgoutput_column_list_init(PGOutputData *data,
void
_PG_output_plugin_init(OutputPluginCallbacks *cb)
{
- AssertVariableIsOfType(&_PG_output_plugin_init, LogicalOutputPluginInit);
-
cb->startup_cb = pgoutput_startup;
cb->begin_cb = pgoutput_begin_txn;
cb->change_cb = pgoutput_change;
--
2.25.1
>From c09374708bdde1eab98fdcad320d5689c6764bc2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:01:22 -0800
Subject: [PATCH v8 2/4] s/ArchiveContext/ArchiveCallbacks
---
src/backend/postmaster/pgarch.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index e551af2905..065d7d1313 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -97,7 +97,7 @@ char *XLogArchiveLibrary = "";
*/
static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static ArchiveModuleCallbacks ArchiveCallbacks;
/*
@@ -406,8 +406,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())
{
ereport(WARNING,
(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +508,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(xlog, pathname);
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
@@ -814,7 +814,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,7 +827,7 @@ LoadArchiveLibrary(void)
errmsg("both archive_command and archive_library set"),
errdetail("Only one of archive_command, archive_library may be set.")));
- memset(, 0, sizeof(ArchiveModuleCallbacks));
+ memset(, 0, sizeof(ArchiveModuleCallbacks));
/*
* If shell archiving is enabled, use our special initialization function.
@@ -844,9 +844,9 @@ LoadArchiveLibrary(void)
Re: recovery modules
Hi, On 2023-02-08 09:57:56 -0800, Nathan Bossart wrote: > On Wed, Feb 08, 2023 at 09:33:44AM -0800, Andres Freund wrote: > > And yes, I'd be for a patch to remove all of those assertions. > > done If you'd reorder it so that 0004 applies independently from the other changes, I'd just push that now. I was remembering additional AssertVariableIsOfType(), but it looks like we actually did remember to take them out when redesigning the output plugin interface... Greetings, Andres Freund
Re: recovery modules
On Wed, Feb 08, 2023 at 09:33:44AM -0800, Andres Freund wrote:
> And yes, I'd be for a patch to remove all of those assertions.
done
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 09fcca03d4a91be5757201cc311d3dad08463cd0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:01:22 -0800
Subject: [PATCH v7 1/4] s/ArchiveContext/ArchiveCallbacks
---
src/backend/postmaster/pgarch.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index e551af2905..065d7d1313 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -97,7 +97,7 @@ char *XLogArchiveLibrary = "";
*/
static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static ArchiveModuleCallbacks ArchiveCallbacks;
/*
@@ -406,8 +406,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())
{
ereport(WARNING,
(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +508,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(xlog, pathname);
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
@@ -814,7 +814,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,7 +827,7 @@ LoadArchiveLibrary(void)
errmsg("both archive_command and archive_library set"),
errdetail("Only one of archive_command, archive_library may be set.")));
- memset(, 0, sizeof(ArchiveModuleCallbacks));
+ memset(, 0, sizeof(ArchiveModuleCallbacks));
/*
* If shell archiving is enabled, use our special initialization function.
@@ -844,9 +844,9 @@ LoadArchiveLibrary(void)
ereport(ERROR,
(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
- (*archive_init) ();
+ (*archive_init) ();
- if (ArchiveContext.archive_file_cb == NULL)
+ if (ArchiveCallbacks.archive_file_cb == NULL)
ereport(ERROR,
(errmsg("archive modules must register an archive callback")));
@@ -859,6 +859,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();
}
--
2.25.1
>From 870999a50d3372d9ed33583b4d6eac05a2accada Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:16:34 -0800
Subject: [PATCH v7 2/4] move archive module exports to dedicated headers
---
contrib/basic_archive/basic_archive.c | 2 +-
src/backend/postmaster/pgarch.c | 2 ++
src/backend/postmaster/shell_archive.c | 3 +-
src/backend/utils/misc/guc_tables.c | 1 +
src/include/postmaster/archive_module.h | 47 +
src/include/postmaster/pgarch.h | 39
src/include/postmaster/shell_archive.h | 24 +
7 files changed, 77 insertions(+), 41 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 3d29711a31..87bbb2174d 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"
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 065d7d1313..281d9fd8b7 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"
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 806b81c3f2..35f88c1222 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -20,7 +20,8 @@
#include "access/xlog.h"
Re: recovery modules
Hi,
On 2023-02-08 09:27:05 -0800, Nathan Bossart wrote:
> On Wed, Feb 08, 2023 at 08:27:13AM -0800, Andres Freund wrote:
> > One minor thing: I don't think we really need the AssertVariableIsOfType()
> > for
> > anything but the Init() one?
>
> This is another part that was borrowed from logical decoding output
> plugins.
I know :(. It was needed in an earlier version of the output plugin interface,
where all the different callbacks were looked up via dlsym(), but should have
been removed after that.
> I'm not sure this adds much since f2b73c8 ("Add central
> declarations for dlsym()ed symbols"). Perhaps we should remove all of
> these assertions for functions that now have central declarations.
Most of them weren't needed even before that.
And yes, I'd be for a patch to remove all of those assertions.
Greetings,
Andres Freund
Re: recovery modules
On Wed, Feb 08, 2023 at 08:27:13AM -0800, Andres Freund wrote:
> One minor thing: I don't think we really need the AssertVariableIsOfType() for
> anything but the Init() one?
This is another part that was borrowed from logical decoding output
plugins. I'm not sure this adds much since f2b73c8 ("Add central
declarations for dlsym()ed symbols"). Perhaps we should remove all of
these assertions for functions that now have central declarations.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: recovery modules
Hi, On 2023-02-08 16:23:34 +0900, Michael Paquier wrote: > On Sat, Feb 04, 2023 at 10:14:36AM -0800, Nathan Bossart wrote: > > On Sat, Feb 04, 2023 at 11:59:20AM +0900, Michael Paquier wrote: > >> + ArchiveModuleCallbacks struct filled with the callback function > >> pointers for > >> This needs a structname markup. > >> > >> + can use state->private_data to store it. > >> And here it would be structfield. > > > > fixed > > Andres, did you have the change to look at that? I did look at it, > but it may not address all the points you may have in mind. Yes, I think this looks pretty good now. One minor thing: I don't think we really need the AssertVariableIsOfType() for anything but the Init() one? Greetings, Andres Freund
Re: recovery modules
On Sat, Feb 04, 2023 at 10:14:36AM -0800, Nathan Bossart wrote: > On Sat, Feb 04, 2023 at 11:59:20AM +0900, Michael Paquier wrote: >> + ArchiveModuleCallbacks struct filled with the callback function pointers >> for >> This needs a structname markup. >> >> + can use state->private_data to store it. >> And here it would be structfield. > > fixed Andres, did you have the change to look at that? I did look at it, but it may not address all the points you may have in mind. -- Michael signature.asc Description: PGP signature
Re: recovery modules
On Sat, Feb 04, 2023 at 11:59:20AM +0900, Michael Paquier wrote:
> + ArchiveModuleCallbacks struct filled with the callback function pointers
> for
> This needs a structname markup.
>
> + can use state->private_data to store it.
> And here it would be structfield.
fixed
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From a0241aca9957b2edb0db151bc9d7f9a1505b207c Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:01:22 -0800
Subject: [PATCH v6 1/3] s/ArchiveContext/ArchiveCallbacks
---
src/backend/postmaster/pgarch.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index e551af2905..065d7d1313 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -97,7 +97,7 @@ char *XLogArchiveLibrary = "";
*/
static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static ArchiveModuleCallbacks ArchiveCallbacks;
/*
@@ -406,8 +406,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())
{
ereport(WARNING,
(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +508,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(xlog, pathname);
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
@@ -814,7 +814,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,7 +827,7 @@ LoadArchiveLibrary(void)
errmsg("both archive_command and archive_library set"),
errdetail("Only one of archive_command, archive_library may be set.")));
- memset(, 0, sizeof(ArchiveModuleCallbacks));
+ memset(, 0, sizeof(ArchiveModuleCallbacks));
/*
* If shell archiving is enabled, use our special initialization function.
@@ -844,9 +844,9 @@ LoadArchiveLibrary(void)
ereport(ERROR,
(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
- (*archive_init) ();
+ (*archive_init) ();
- if (ArchiveContext.archive_file_cb == NULL)
+ if (ArchiveCallbacks.archive_file_cb == NULL)
ereport(ERROR,
(errmsg("archive modules must register an archive callback")));
@@ -859,6 +859,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();
}
--
2.25.1
>From 3c0b4c7a55bf93a2e6a9da0ec33f67705cb75574 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:16:34 -0800
Subject: [PATCH v6 2/3] move archive module exports to dedicated headers
---
contrib/basic_archive/basic_archive.c | 2 +-
src/backend/postmaster/pgarch.c | 2 ++
src/backend/postmaster/shell_archive.c | 3 +-
src/backend/utils/misc/guc_tables.c | 1 +
src/include/postmaster/archive_module.h | 47 +
src/include/postmaster/pgarch.h | 39
src/include/postmaster/shell_archive.h | 24 +
7 files changed, 77 insertions(+), 41 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 3d29711a31..87bbb2174d 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"
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 065d7d1313..281d9fd8b7 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"
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 806b81c3f2..35f88c1222
Re: recovery modules
On Thu, Feb 02, 2023 at 11:37:00AM -0800, Nathan Bossart wrote:
> On Thu, Feb 02, 2023 at 02:34:17PM +0900, Michael Paquier wrote:
>> Okay, the changes done here look straight-forward seen from here. I
>> got one small-ish comment.
>>
>> +basic_archive_startup(ArchiveModuleState *state)
>> +{
>> + BasicArchiveData *data = palloc0(sizeof(BasicArchiveData));
>>
>> Perhaps this should use MemoryContextAlloc() rather than a plain
>> palloc(). This should not matter based on the position where the
>> startup callback is called, still that may be a pattern worth
>> encouraging.
>
> Good call.
+ ArchiveModuleCallbacks struct filled with the callback function pointers for
This needs a structname markup.
+ can use state->private_data to store it.
And here it would be structfield.
As far as I can see, all the points raised about this redesign seem to
have been addressed. Andres, any comments?
--
Michael
signature.asc
Description: PGP signature
Re: recovery modules
On Thu, Feb 02, 2023 at 02:34:17PM +0900, Michael Paquier wrote:
> Okay, the changes done here look straight-forward seen from here. I
> got one small-ish comment.
>
> +basic_archive_startup(ArchiveModuleState *state)
> +{
> + BasicArchiveData *data = palloc0(sizeof(BasicArchiveData));
>
> Perhaps this should use MemoryContextAlloc() rather than a plain
> palloc(). This should not matter based on the position where the
> startup callback is called, still that may be a pattern worth
> encouraging.
Good call.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 97585c26468a39b0f881a9cc8b81ea614da9a547 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:01:22 -0800
Subject: [PATCH v5 1/3] s/ArchiveContext/ArchiveCallbacks
---
src/backend/postmaster/pgarch.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 3c714a79c6..dda6698509 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -97,7 +97,7 @@ char *XLogArchiveLibrary = "";
*/
static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static ArchiveModuleCallbacks ArchiveCallbacks;
/*
@@ -406,8 +406,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())
{
ereport(WARNING,
(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +508,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(xlog, pathname);
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
@@ -814,7 +814,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,7 +827,7 @@ LoadArchiveLibrary(void)
errmsg("both archive_command and archive_library set"),
errdetail("Only one of archive_command, archive_library may be set.")));
- memset(, 0, sizeof(ArchiveModuleCallbacks));
+ memset(, 0, sizeof(ArchiveModuleCallbacks));
/*
* If shell archiving is enabled, use our special initialization function.
@@ -844,9 +844,9 @@ LoadArchiveLibrary(void)
ereport(ERROR,
(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
- (*archive_init) ();
+ (*archive_init) ();
- if (ArchiveContext.archive_file_cb == NULL)
+ if (ArchiveCallbacks.archive_file_cb == NULL)
ereport(ERROR,
(errmsg("archive modules must register an archive callback")));
@@ -859,6 +859,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();
}
--
2.25.1
>From 8f26acafe541abcdd9261c2668876ff288fc02f4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:16:34 -0800
Subject: [PATCH v5 2/3] move archive module exports to dedicated headers
---
contrib/basic_archive/basic_archive.c | 2 +-
src/backend/postmaster/pgarch.c | 2 ++
src/backend/postmaster/shell_archive.c | 3 +-
src/backend/utils/misc/guc_tables.c | 1 +
src/include/postmaster/archive_module.h | 47 +
src/include/postmaster/pgarch.h | 39
src/include/postmaster/shell_archive.h | 24 +
7 files changed, 77 insertions(+), 41 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 3d29711a31..87bbb2174d 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"
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index dda6698509..ec47b2cc20 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"
Re: recovery modules
On Wed, Feb 01, 2023 at 01:23:26PM -0800, Nathan Bossart wrote:
> Yeah, that's nicer. cfbot is complaining about a missing #include, so I
> need to send a new revision anyway.
Okay, the changes done here look straight-forward seen from here. I
got one small-ish comment.
+basic_archive_startup(ArchiveModuleState *state)
+{
+ BasicArchiveData *data = palloc0(sizeof(BasicArchiveData));
Perhaps this should use MemoryContextAlloc() rather than a plain
palloc(). This should not matter based on the position where the
startup callback is called, still that may be a pattern worth
encouraging.
--
Michael
signature.asc
Description: PGP signature
Re: recovery modules
On Wed, Feb 01, 2023 at 01:06:06PM -0800, Andres Freund wrote:
> On 2023-02-01 12:15:29 -0800, Nathan Bossart wrote:
>> Here's a new patch set in which I've attempted to address this feedback and
>> Michael's feedback.
>
> Looks better!
Thanks!
>> @@ -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);
>
> Personally I'd always pass ArchiveModuleState *state as the first arg,
> but it's not important.
Yeah, that's nicer. cfbot is complaining about a missing #include, so I
need to send a new revision anyway.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From e54ed061b772e7f0859a37612ed1effebc5c6ba1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:01:22 -0800
Subject: [PATCH v4 1/3] s/ArchiveContext/ArchiveCallbacks
---
src/backend/postmaster/pgarch.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 3c714a79c6..dda6698509 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -97,7 +97,7 @@ char *XLogArchiveLibrary = "";
*/
static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static ArchiveModuleCallbacks ArchiveCallbacks;
/*
@@ -406,8 +406,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())
{
ereport(WARNING,
(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +508,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(xlog, pathname);
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
@@ -814,7 +814,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,7 +827,7 @@ LoadArchiveLibrary(void)
errmsg("both archive_command and archive_library set"),
errdetail("Only one of archive_command, archive_library may be set.")));
- memset(, 0, sizeof(ArchiveModuleCallbacks));
+ memset(, 0, sizeof(ArchiveModuleCallbacks));
/*
* If shell archiving is enabled, use our special initialization function.
@@ -844,9 +844,9 @@ LoadArchiveLibrary(void)
ereport(ERROR,
(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
- (*archive_init) ();
+ (*archive_init) ();
- if (ArchiveContext.archive_file_cb == NULL)
+ if (ArchiveCallbacks.archive_file_cb == NULL)
ereport(ERROR,
(errmsg("archive modules must register an archive callback")));
@@ -859,6 +859,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();
}
--
2.25.1
>From 37e19c986b548f5d281190e196e01ce94f8c4391 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:16:34 -0800
Subject: [PATCH v4 2/3] move archive module exports to dedicated headers
---
contrib/basic_archive/basic_archive.c | 2 +-
src/backend/postmaster/pgarch.c | 2 ++
src/backend/postmaster/shell_archive.c | 3 +-
src/backend/utils/misc/guc_tables.c | 1 +
src/include/postmaster/archive_module.h | 47 +
src/include/postmaster/pgarch.h | 39
src/include/postmaster/shell_archive.h | 24 +
7 files changed, 77 insertions(+), 41 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 3d29711a31..87bbb2174d 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@
Re: recovery modules
Hi, On 2023-02-01 12:15:29 -0800, Nathan Bossart wrote: > Here's a new patch set in which I've attempted to address this feedback and > Michael's feedback. Looks better! > @@ -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); Personally I'd always pass ArchiveModuleState *state as the first arg, but it's not important. Greetings, Andres Freund
Re: recovery modules
On Wed, Feb 01, 2023 at 03:54:26AM -0800, Andres Freund wrote:
> I'd make basic_archive's private data a struct, with a member for the
> context, but it's not that important.
>
> I'd also be inclined to do the same for the private_state you're passing
> around for each module. Even if it's just to reduce the number of
> functions accepting void * - loosing compiler type checking isn't great.
>
> So maybe an ArchiveModuleState { void *private_data } that's passed to
> basic_archive_startup() and all the other callbacks.
Here's a new patch set in which I've attempted to address this feedback and
Michael's feedback.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From e54ed061b772e7f0859a37612ed1effebc5c6ba1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:01:22 -0800
Subject: [PATCH v3 1/3] s/ArchiveContext/ArchiveCallbacks
---
src/backend/postmaster/pgarch.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 3c714a79c6..dda6698509 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -97,7 +97,7 @@ char *XLogArchiveLibrary = "";
*/
static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static ArchiveModuleCallbacks ArchiveCallbacks;
/*
@@ -406,8 +406,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())
{
ereport(WARNING,
(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -508,7 +508,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(xlog, pathname);
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
@@ -814,7 +814,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,7 +827,7 @@ LoadArchiveLibrary(void)
errmsg("both archive_command and archive_library set"),
errdetail("Only one of archive_command, archive_library may be set.")));
- memset(, 0, sizeof(ArchiveModuleCallbacks));
+ memset(, 0, sizeof(ArchiveModuleCallbacks));
/*
* If shell archiving is enabled, use our special initialization function.
@@ -844,9 +844,9 @@ LoadArchiveLibrary(void)
ereport(ERROR,
(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
- (*archive_init) ();
+ (*archive_init) ();
- if (ArchiveContext.archive_file_cb == NULL)
+ if (ArchiveCallbacks.archive_file_cb == NULL)
ereport(ERROR,
(errmsg("archive modules must register an archive callback")));
@@ -859,6 +859,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();
}
--
2.25.1
>From 0bcfb2dc8ef1544dcd6a8cc07a784b36a38febad Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:16:34 -0800
Subject: [PATCH v3 2/3] move archive module exports to dedicated headers
---
contrib/basic_archive/basic_archive.c | 2 +-
src/backend/postmaster/pgarch.c | 2 ++
src/backend/postmaster/shell_archive.c | 3 +-
src/backend/utils/misc/guc_tables.c | 1 +
src/include/postmaster/archive_module.h | 47 +
src/include/postmaster/pgarch.h | 39
src/include/postmaster/shell_archive.h | 22
7 files changed, 75 insertions(+), 41 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 3d29711a31..87bbb2174d 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"
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index dda6698509..ec47b2cc20 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"
Re: recovery modules
Hi,
On 2023-01-31 15:30:13 -0800, Nathan Bossart wrote:
> +/*
> + * basic_archive_startup
> + *
> + * Creates the module's memory context.
> + */
> +void *
> +basic_archive_startup(void)
> +{
> + return (void *) AllocSetContextCreate(TopMemoryContext,
> +
> "basic_archive",
> +
> ALLOCSET_DEFAULT_SIZES);
> }
I'd make basic_archive's private data a struct, with a member for the
context, but it's not that important.
I'd also be inclined to do the same for the private_state you're passing
around for each module. Even if it's just to reduce the number of
functions accepting void * - loosing compiler type checking isn't great.
So maybe an ArchiveModuleState { void *private_data } that's passed to
basic_archive_startup() and all the other callbacks.
Greetings,
Andres Freund
Re: recovery modules
On Tue, Jan 31, 2023 at 03:30:13PM -0800, Nathan Bossart wrote:
> Okay, here is a new patch set with the aforementioned adjustments and
> documentation updates.
So, it looks like you have addressed the feedback received here, as
of:
- Rename of Context to Callback.
- Move of the definition into their own header.
- Introduction of a callback for the startup initialization.
- Pass down a private state to each callback.
I have a few minor comments.
+/*
+ * 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);
Storing that in archive_module.h is not incorrect, still feels a bit
unnatural. I would have used a separate header for clarity. It may
not sound like a big deal, but we may want this separation if
archive_module.h is used in some frontend code in the future. Perhaps
that will never be the case, but I've seen many fancy (as in useful)
proposals in the past when it comes to such things.
static bool
-shell_archive_configured(void)
+shell_archive_configured(void *private_state)
{
return XLogArchiveCommand[0] != '\0';
Maybe check that in this context private_state should be NULL? The
other two callbacks could use an assert, as well.
- _PG_archive_module_init. This function is passed a
- struct that needs to be filled with the callback function pointers for
- individual actions.
+ _PG_archive_module_init. This function must return a
+ struct filled with the callback function pointers for individual actions.
Worth mentioning the name of the structure, as of "This function must
return a structure ArchiveModuleCallbacks filled with.."
+The startup_cb 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 void *private_state
+argument.
Not sure about the complexity of two sentences here. This could
simply be:
This function can return a pointer to an area of memory dedicated to
the state of the archive module loaded. This pointer is passed to
each of the module's other callbacks as the argument
private_state.
Side note: it looks like there is nothing in archive-modules.sgml
telling that these modules are only loaded by the archiver process.
--
Michael
signature.asc
Description: PGP signature
Re: recovery modules
On Tue, Jan 31, 2023 at 08:13:11AM +0900, Michael Paquier wrote:
> On Mon, Jan 30, 2023 at 12:04:22PM -0800, Nathan Bossart wrote:
>> On Mon, Jan 30, 2023 at 11:48:10AM -0800, Andres Freund wrote:
>>> I don't think _PG_archive_module_init() should actually allocate a memory
>>> context and do other similar initializations. Instead it should just return
>>> 'const ArchiveModuleCallbacks*', typically a single line.
>>>
>>> Allocations etc should happen in one of the callbacks. That way we can
>>> actually have multiple instances of a module.
>>
>> I think we'd need to invent a startup callback for archive modules for this
>> to work, but that's easy enough.
>
> If you don't return some (void *) pointing to a private area that
> would be stored by the backend, allocated as part of the loading path,
> I agree that an extra callback is what makes the most sense,
> presumably called around the beginning of PgArchiverMain(). Doing
> this kind of one-time action in the file callback woud be weird..
Okay, here is a new patch set with the aforementioned adjustments and
documentation updates.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From f07a2c65439ed1f1b38741f8796563e62adef6bb Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:01:22 -0800
Subject: [PATCH v2 1/3] s/ArchiveContext/ArchiveCallbacks
---
src/backend/postmaster/pgarch.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 8ecdb9ca23..36800127e8 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -97,7 +97,7 @@ char *XLogArchiveLibrary = "";
*/
static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static ArchiveModuleCallbacks ArchiveCallbacks;
/*
@@ -416,8 +416,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())
{
ereport(WARNING,
(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -518,7 +518,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(xlog, pathname);
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
@@ -824,7 +824,7 @@ HandlePgArchInterrupts(void)
/*
* LoadArchiveLibrary
*
- * Loads the archiving callbacks into our local ArchiveContext.
+ * Loads the archiving callbacks into our local ArchiveCallbacks.
*/
static void
LoadArchiveLibrary(void)
@@ -837,7 +837,7 @@ LoadArchiveLibrary(void)
errmsg("both archive_command and archive_library set"),
errdetail("Only one of archive_command, archive_library may be set.")));
- memset(, 0, sizeof(ArchiveModuleCallbacks));
+ memset(, 0, sizeof(ArchiveModuleCallbacks));
/*
* If shell archiving is enabled, use our special initialization function.
@@ -854,9 +854,9 @@ LoadArchiveLibrary(void)
ereport(ERROR,
(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
- (*archive_init) ();
+ (*archive_init) ();
- if (ArchiveContext.archive_file_cb == NULL)
+ if (ArchiveCallbacks.archive_file_cb == NULL)
ereport(ERROR,
(errmsg("archive modules must register an archive callback")));
@@ -869,6 +869,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();
}
--
2.25.1
>From ea03c90a99ed07a4df8537dfa4916d7858decef9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:16:34 -0800
Subject: [PATCH v2 2/3] move archive module exports to dedicated header
---
contrib/basic_archive/basic_archive.c | 2 +-
src/backend/postmaster/pgarch.c | 1 +
src/backend/postmaster/shell_archive.c | 2 +-
src/backend/utils/misc/guc_tables.c | 1 +
src/include/postmaster/archive_module.h | 54 +
src/include/postmaster/pgarch.h | 39 --
6 files changed, 58 insertions(+), 41 deletions(-)
create mode 100644 src/include/postmaster/archive_module.h
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 3d29711a31..87bbb2174d 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
Re: recovery modules
On Mon, Jan 30, 2023 at 12:04:22PM -0800, Nathan Bossart wrote: > On Mon, Jan 30, 2023 at 11:48:10AM -0800, Andres Freund wrote: >> I don't think _PG_archive_module_init() should actually allocate a memory >> context and do other similar initializations. Instead it should just return >> 'const ArchiveModuleCallbacks*', typically a single line. >> >> Allocations etc should happen in one of the callbacks. That way we can >> actually have multiple instances of a module. > > I think we'd need to invent a startup callback for archive modules for this > to work, but that's easy enough. If you don't return some (void *) pointing to a private area that would be stored by the backend, allocated as part of the loading path, I agree that an extra callback is what makes the most sense, presumably called around the beginning of PgArchiverMain(). Doing this kind of one-time action in the file callback woud be weird.. -- Michael signature.asc Description: PGP signature
Re: recovery modules
On Mon, Jan 30, 2023 at 11:48:10AM -0800, Andres Freund wrote: > I don't think _PG_archive_module_init() should actually allocate a memory > context and do other similar initializations. Instead it should just return > 'const ArchiveModuleCallbacks*', typically a single line. > > Allocations etc should happen in one of the callbacks. That way we can > actually have multiple instances of a module. I think we'd need to invent a startup callback for archive modules for this to work, but that's easy enough. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: recovery modules
Hi,
On 2023-01-30 16:51:38 +0900, Michael Paquier wrote:
> On Fri, Jan 27, 2023 at 10:27:29PM -0800, Nathan Bossart wrote:
> > Here is a work-in-progress patch set for adjusting the archive modules
> > interface. Is this roughly what you had in mind?
>
> I have been catching up with what is happening here. I can get
> behind the idea to use the term "callbacks" vs "context" for clarity,
> to move the callback definitions into their own header, and to add
> extra arguments to the callback functions for some private data.
>
> -void
> -_PG_archive_module_init(ArchiveModuleCallbacks *cb)
> +const ArchiveModuleCallbacks *
> +_PG_archive_module_init(void **arg)
> {
> AssertVariableIsOfType(&_PG_archive_module_init, ArchiveModuleInit);
>
> - cb->check_configured_cb = basic_archive_configured;
> - cb->archive_file_cb = basic_archive_file;
> + (*arg) = (void *) AllocSetContextCreate(TopMemoryContext,
> + "basic_archive",
> + ALLOCSET_DEFAULT_SIZES);
> +
> + return _archive_callbacks;
> Now, I find this part, where we use a double pointer to allow the
> module initialization to create and give back a private area, rather
> confusing, and I think that this could be bug-prone, as well.
I don't think _PG_archive_module_init() should actually allocate a memory
context and do other similar initializations. Instead it should just return
'const ArchiveModuleCallbacks*', typically a single line.
Allocations etc should happen in one of the callbacks. That way we can
actually have multiple instances of a module.
> Once
> you incorporate some data within the set of callbacks, isn't this
> stuff close to a "state" data, or just something that we could call
> only an "ArchiveModule"? Could it make more sense to have
> _PG_archive_module_init return a structure with everything rather than
> a separate in/out argument? Here is the idea, simply:
> typedef struct ArchiveModule {
> ArchiveCallbacks *routines;
> void *private_data;
> /* Potentially more here, like some flags? */
> } ArchiveModule;
I don't like this much. This still basically ends up with the module callbacks
not being sufficient to instantiate an archive module.
Greetings,
Andres Freund
Re: recovery modules
On Mon, Jan 30, 2023 at 04:51:38PM +0900, Michael Paquier wrote:
> Now, I find this part, where we use a double pointer to allow the
> module initialization to create and give back a private area, rather
> confusing, and I think that this could be bug-prone, as well. Once
> you incorporate some data within the set of callbacks, isn't this
> stuff close to a "state" data, or just something that we could call
> only an "ArchiveModule"? Could it make more sense to have
> _PG_archive_module_init return a structure with everything rather than
> a separate in/out argument? Here is the idea, simply:
> typedef struct ArchiveModule {
> ArchiveCallbacks *routines;
> void *private_data;
> /* Potentially more here, like some flags? */
> } ArchiveModule;
Yeah, we could probably invent an ArchiveModuleContext struct. I think
this is similar to how LogicalDecodingContext is used.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: recovery modules
On Fri, Jan 27, 2023 at 10:27:29PM -0800, Nathan Bossart wrote:
> Here is a work-in-progress patch set for adjusting the archive modules
> interface. Is this roughly what you had in mind?
I have been catching up with what is happening here. I can get
behind the idea to use the term "callbacks" vs "context" for clarity,
to move the callback definitions into their own header, and to add
extra arguments to the callback functions for some private data.
-void
-_PG_archive_module_init(ArchiveModuleCallbacks *cb)
+const ArchiveModuleCallbacks *
+_PG_archive_module_init(void **arg)
{
AssertVariableIsOfType(&_PG_archive_module_init, ArchiveModuleInit);
- cb->check_configured_cb = basic_archive_configured;
- cb->archive_file_cb = basic_archive_file;
+ (*arg) = (void *) AllocSetContextCreate(TopMemoryContext,
+ "basic_archive",
+ ALLOCSET_DEFAULT_SIZES);
+
+ return _archive_callbacks;
Now, I find this part, where we use a double pointer to allow the
module initialization to create and give back a private area, rather
confusing, and I think that this could be bug-prone, as well. Once
you incorporate some data within the set of callbacks, isn't this
stuff close to a "state" data, or just something that we could call
only an "ArchiveModule"? Could it make more sense to have
_PG_archive_module_init return a structure with everything rather than
a separate in/out argument? Here is the idea, simply:
typedef struct ArchiveModule {
ArchiveCallbacks *routines;
void *private_data;
/* Potentially more here, like some flags? */
} ArchiveModule;
That would be closer to the style of xlogreader.h, for example.
All these choices should be documented in archive_module.h, at the
end.
--
Michael
signature.asc
Description: PGP signature
Re: recovery modules
On Fri, Jan 27, 2023 at 08:17:46PM -0800, Nathan Bossart wrote:
> On Fri, Jan 27, 2023 at 05:55:42PM -0800, Andres Freund wrote:
>> On 2023-01-27 16:59:10 -0800, Nathan Bossart wrote:
>>> I think it would be weird for the archive module and
>>> recovery module interfaces to look so different, but if that's okay, I can
>>> change it.
>>
>> I'm a bit sad about the archive module case - I wonder if we should change it
>> now, there can't be many users of it out there. And I think it's more likely
>> that we'll eventually want multiple archiving scripts to run concurrently -
>> which will be quite hard with the current interface (no private state).
>
> I'm open to that. IIUC it wouldn't require too many changes to existing
> archive modules, and if it gets us closer to batching or parallelism, it's
> probably worth doing sooner than later.
Here is a work-in-progress patch set for adjusting the archive modules
interface. Is this roughly what you had in mind?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From f865032283d0c937ff1c47441d70a2b11e89d3f4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:01:22 -0800
Subject: [PATCH 1/4] s/ArchiveContext/ArchiveCallbacks
---
src/backend/postmaster/pgarch.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 8ecdb9ca23..36800127e8 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -97,7 +97,7 @@ char *XLogArchiveLibrary = "";
*/
static time_t last_sigterm_time = 0;
static PgArchData *PgArch = NULL;
-static ArchiveModuleCallbacks ArchiveContext;
+static ArchiveModuleCallbacks ArchiveCallbacks;
/*
@@ -416,8 +416,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())
{
ereport(WARNING,
(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -518,7 +518,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(xlog, pathname);
if (ret)
snprintf(activitymsg, sizeof(activitymsg), "last was %s", xlog);
else
@@ -824,7 +824,7 @@ HandlePgArchInterrupts(void)
/*
* LoadArchiveLibrary
*
- * Loads the archiving callbacks into our local ArchiveContext.
+ * Loads the archiving callbacks into our local ArchiveCallbacks.
*/
static void
LoadArchiveLibrary(void)
@@ -837,7 +837,7 @@ LoadArchiveLibrary(void)
errmsg("both archive_command and archive_library set"),
errdetail("Only one of archive_command, archive_library may be set.")));
- memset(, 0, sizeof(ArchiveModuleCallbacks));
+ memset(, 0, sizeof(ArchiveModuleCallbacks));
/*
* If shell archiving is enabled, use our special initialization function.
@@ -854,9 +854,9 @@ LoadArchiveLibrary(void)
ereport(ERROR,
(errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")));
- (*archive_init) ();
+ (*archive_init) ();
- if (ArchiveContext.archive_file_cb == NULL)
+ if (ArchiveCallbacks.archive_file_cb == NULL)
ereport(ERROR,
(errmsg("archive modules must register an archive callback")));
@@ -869,6 +869,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();
}
--
2.25.1
>From 1ea6fb293a1e69e6fdd39e1d6b96a0af601d8542 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 27 Jan 2023 21:16:34 -0800
Subject: [PATCH 2/4] move archive module exports to dedicated header
---
contrib/basic_archive/basic_archive.c | 2 +-
src/backend/postmaster/pgarch.c | 1 +
src/backend/postmaster/shell_archive.c | 2 +-
src/backend/utils/misc/guc_tables.c | 1 +
src/include/postmaster/archive_module.h | 54 +
src/include/postmaster/pgarch.h | 39 --
6 files changed, 58 insertions(+), 41 deletions(-)
create mode 100644 src/include/postmaster/archive_module.h
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 3d29711a31..87bbb2174d 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"
diff --git a/src/backend/postmaster/pgarch.c
Re: recovery modules
On Fri, Jan 27, 2023 at 05:55:42PM -0800, Andres Freund wrote: > On 2023-01-27 16:59:10 -0800, Nathan Bossart wrote: >> I think it would be weird for the archive module and >> recovery module interfaces to look so different, but if that's okay, I can >> change it. > > I'm a bit sad about the archive module case - I wonder if we should change it > now, there can't be many users of it out there. And I think it's more likely > that we'll eventually want multiple archiving scripts to run concurrently - > which will be quite hard with the current interface (no private state). I'm open to that. IIUC it wouldn't require too many changes to existing archive modules, and if it gets us closer to batching or parallelism, it's probably worth doing sooner than later. > I was wondering why we implement "shell" via a separate mechanism from > restore_library. I.e. a) why doesn't restore_library default to 'shell', > instead of an empty string, b) why aren't restore_command et al implemented > using a restore module. I think that's the long-term idea. For archive modules, there were concerns about backward compatibility [0]. [0] https://postgr.es/m/CABUevEx8cKy%3D%2BYQU_3NaeXnZV2bSB7Lk6EE%2B-FEcmE4JO4V1hg%40mail.gmail.com -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: recovery modules
Hi,
On 2023-01-27 16:59:10 -0800, Nathan Bossart wrote:
> On Fri, Jan 27, 2023 at 04:23:19PM -0800, Andres Freund wrote:
> >> +typedef bool (*RecoveryRestoreCB) (const char *file, const char *path,
> >> + const char
> >> *lastRestartPointFileName);
> >> +typedef void (*RecoveryArchiveCleanupCB) (const char
> >> *lastRestartPointFileName);
> >> +typedef void (*RecoveryEndCB) (const char *lastRestartPointFileName);
> >> +typedef void (*RecoveryShutdownCB) (void);
> >
> > I think the signature of these forces bad coding practices, because there's
> > no
> > way to have state within a recovery module (as there's no parameter for it).
> >
> > It's possible we would eventually support multiple modules, e.g. restoring
> > from shorter term file based archiving and from longer term archiving in
> > some
> > blob store. Then we'll regret not having a varible for this.
>
> Are you suggesting that we add a "void *arg" to each one of these?
Yes. Or pass a pointer to a struct with a "private_data" style field to all
of them.
> >> +extern RecoveryModuleCallbacks RecoveryContext;
> >
> > I think that'll typically be interpreteted as a MemoryContext by readers.
>
> How about RecoveryCallbacks?
Callbacks is better than Context here imo, but I think 'Recovery' makes it
sound like this actually performs WAL replay or such. Seems like it should be
RestoreCallbacks at the very least?
> > Also, why is this a global var? Exported too?
>
> It's needed in xlog.c, xlogarchive.c, and xlogrecovery.c. Would you rather
> it be static to xlogarchive.c and provide accessors for the others?
Maybe? Something about this feels wrong to me, but I can't entirely put my
finger on it.
> >> +/*
> >> + * Type of the shared library symbol _PG_recovery_module_init that is
> >> looked up
> >> + * when loading a recovery library.
> >> + */
> >> +typedef void (*RecoveryModuleInit) (RecoveryModuleCallbacks *cb);
> >
> > I think this is a bad way to return callbacks. This way the
> > RecoveryModuleCallbacks needs to be modifiable, which makes the job for the
> > compiler harder (and isn't the greatest for security).
> >
> > I strongly encourage you to follow the model used e.g. by tableam. The init
> > function should return a pointer to a *constant* struct. Which is
> > compile-time
> > initialized with the function pointers.
> >
> > See the bottom of heapam_handler.c for how that ends up looking.
>
> Hm. I used the existing strategy for archive modules and logical decoding
> output plugins here.
Unfortunately I didn't realize the problem when I was designing the output
plugin interface. But there's probably too many users of it out there now to
change it.
The interface does at least provide a way to have its own "per instance"
state, via the startup callback and
LogicalDecodingContext->output_plugin_private.
The worst interface in this area is index AMs - the handler returns a pointer
to a palloced struct with callbacks. That then is copied into a new allocation
in the relcache entry. We have hundreds to thousands of copies of what
bthandler() sets up in memory. Without any sort of benefit.
> I think it would be weird for the archive module and
> recovery module interfaces to look so different, but if that's okay, I can
> change it.
I'm a bit sad about the archive module case - I wonder if we should change it
now, there can't be many users of it out there. And I think it's more likely
that we'll eventually want multiple archiving scripts to run concurrently -
which will be quite hard with the current interface (no private state).
Just btw: It's imo a bit awkward for the definition of the archiving plugin
interface to be in pgarch.h: "Exports from postmaster/pgarch.c" doesn't
describe that well. A dedicated header seems cleaner.
>
> >> +void
> >> +LoadRecoveryCallbacks(void)
> >> +{
> >> + RecoveryModuleInit init;
> >> +
> >> + /*
> >> + * If the shell command is enabled, use our special initialization
> >> + * function. Otherwise, load the library and call its
> >> + * _PG_recovery_module_init().
> >> + */
> >> + if (restoreLibrary[0] == '\0')
> >> + init = shell_restore_init;
> >> + else
> >> + init = (RecoveryModuleInit)
> >> + load_external_function(restoreLibrary,
> >> "_PG_recovery_module_init",
> >> + false, NULL);
> >
> > Why a special rule for shell, instead of just defaulting the GUC to it?
>
> I'm not following this one. The default value of the restore_library GUC
> is an empty string, which means that the shell commands should be used.
I was wondering why we implement "shell" via a separate mechanism from
restore_library. I.e. a) why doesn't restore_library default to 'shell',
instead of an empty string, b) why aren't restore_command et al implemented
using a restore module.
> >> + /*
> >> + * If using shell commands,
Re: recovery modules
On Fri, Jan 27, 2023 at 04:23:19PM -0800, Andres Freund wrote:
>> +typedef bool (*RecoveryRestoreCB) (const char *file, const char *path,
>> + const char
>> *lastRestartPointFileName);
>> +typedef void (*RecoveryArchiveCleanupCB) (const char
>> *lastRestartPointFileName);
>> +typedef void (*RecoveryEndCB) (const char *lastRestartPointFileName);
>> +typedef void (*RecoveryShutdownCB) (void);
>
> I think the signature of these forces bad coding practices, because there's no
> way to have state within a recovery module (as there's no parameter for it).
>
> It's possible we would eventually support multiple modules, e.g. restoring
> from shorter term file based archiving and from longer term archiving in some
> blob store. Then we'll regret not having a varible for this.
Are you suggesting that we add a "void *arg" to each one of these? Or put
the arguments into a struct? Or something else?
>> +extern RecoveryModuleCallbacks RecoveryContext;
>
> I think that'll typically be interpreteted as a MemoryContext by readers.
How about RecoveryCallbacks?
> Also, why is this a global var? Exported too?
It's needed in xlog.c, xlogarchive.c, and xlogrecovery.c. Would you rather
it be static to xlogarchive.c and provide accessors for the others?
>> +/*
>> + * Type of the shared library symbol _PG_recovery_module_init that is
>> looked up
>> + * when loading a recovery library.
>> + */
>> +typedef void (*RecoveryModuleInit) (RecoveryModuleCallbacks *cb);
>
> I think this is a bad way to return callbacks. This way the
> RecoveryModuleCallbacks needs to be modifiable, which makes the job for the
> compiler harder (and isn't the greatest for security).
>
> I strongly encourage you to follow the model used e.g. by tableam. The init
> function should return a pointer to a *constant* struct. Which is compile-time
> initialized with the function pointers.
>
> See the bottom of heapam_handler.c for how that ends up looking.
Hm. I used the existing strategy for archive modules and logical decoding
output plugins here. I think it would be weird for the archive module and
recovery module interfaces to look so different, but if that's okay, I can
change it.
>> +void
>> +LoadRecoveryCallbacks(void)
>> +{
>> +RecoveryModuleInit init;
>> +
>> +/*
>> + * If the shell command is enabled, use our special initialization
>> + * function. Otherwise, load the library and call its
>> + * _PG_recovery_module_init().
>> + */
>> +if (restoreLibrary[0] == '\0')
>> +init = shell_restore_init;
>> +else
>> +init = (RecoveryModuleInit)
>> +load_external_function(restoreLibrary,
>> "_PG_recovery_module_init",
>> + false, NULL);
>
> Why a special rule for shell, instead of just defaulting the GUC to it?
I'm not following this one. The default value of the restore_library GUC
is an empty string, which means that the shell commands should be used.
>> +/*
>> + * If using shell commands, remove callbacks for any commands that are
>> not
>> + * set.
>> + */
>> +if (restoreLibrary[0] == '\0')
>> +{
>> +if (recoveryRestoreCommand[0] == '\0')
>> +RecoveryContext.restore_cb = NULL;
>> +if (archiveCleanupCommand[0] == '\0')
>> +RecoveryContext.archive_cleanup_cb = NULL;
>> +if (recoveryEndCommand[0] == '\0')
>> +RecoveryContext.recovery_end_cb = NULL;
>
> I'd just mandate that these are implemented and that the module has to handle
> if it doesn't need to do anything.
Wouldn't this just force module authors to write empty functions for the
parts they don't need?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: recovery modules
On Fri, Jan 27, 2023 at 04:09:39PM -0800, Andres Freund wrote: > I think it would be hard to write a good module that isn't just implementing > the existing commands without it. Needing to clean up archives and reacting to > the end of recovery seems a pretty core task. FWIW, recovery_end_command is straight-forward as it is done by the startup process, so that's an easy take. You could split the cake into two parts, as well, aka first focus on restore_command and recovery_end_command as a first step, then we could try to figure out how archive_cleanup_command would fit in this picture with the checkpointer or a different process. There are a bunch of deployments where WAL archive retention is controlled by the age of the backups, not by the backend deciding when these should be removed as a checkpoint runs depending on the computed redo LSN, so recovery modules would still be useful with just coverage for restore_command and recovery_end_command. > I was briefly wondering whether it'd be worth trying to offload things like > archive_cleanup_command from checkpointer to a different process, for > robustness. But given that it's pretty much required for performance that the > module runs in the startup process, that ship probably has sailed. Yeah, agreed that this could be interesting. That could leverage the work of the checkpointer. Nathan has proposed a patch for that recently, as far as I recall, to offload some tasks from the startup and checkpointer processes: https://commitfest.postgresql.org/41/3448/ So that pretty much goes into the same area? -- Michael signature.asc Description: PGP signature
Re: recovery modules
Hi,
On 2023-01-25 16:34:21 +0900, Michael Paquier wrote:
> diff --git a/src/include/access/xlogarchive.h
> b/src/include/access/xlogarchive.h
> index 299304703e..71c9b88165 100644
> --- a/src/include/access/xlogarchive.h
> +++ b/src/include/access/xlogarchive.h
> @@ -30,9 +30,45 @@ extern bool XLogArchiveIsReady(const char *xlog);
> extern bool XLogArchiveIsReadyOrDone(const char *xlog);
> extern void XLogArchiveCleanup(const char *xlog);
>
> -extern bool shell_restore(const char *file, const char *path,
> - const char
> *lastRestartPointFileName);
> -extern void shell_archive_cleanup(const char *lastRestartPointFileName);
> -extern void shell_recovery_end(const char *lastRestartPointFileName);
> +/*
> + * Recovery module callbacks
> + *
> + * These callback functions should be defined by recovery libraries and
> + * returned via _PG_recovery_module_init(). For more information about the
> + * purpose of each callback, refer to the recovery modules documentation.
> + */
> +typedef bool (*RecoveryRestoreCB) (const char *file, const char *path,
> +const char
> *lastRestartPointFileName);
> +typedef void (*RecoveryArchiveCleanupCB) (const char
> *lastRestartPointFileName);
> +typedef void (*RecoveryEndCB) (const char *lastRestartPointFileName);
> +typedef void (*RecoveryShutdownCB) (void);
I think the signature of these forces bad coding practices, because there's no
way to have state within a recovery module (as there's no parameter for it).
It's possible we would eventually support multiple modules, e.g. restoring
from shorter term file based archiving and from longer term archiving in some
blob store. Then we'll regret not having a varible for this.
> +typedef struct RecoveryModuleCallbacks
> +{
> + RecoveryRestoreCB restore_cb;
> + RecoveryArchiveCleanupCB archive_cleanup_cb;
> + RecoveryEndCB recovery_end_cb;
> + RecoveryShutdownCB shutdown_cb;
> +} RecoveryModuleCallbacks;
> +
> +extern RecoveryModuleCallbacks RecoveryContext;
I think that'll typically be interpreteted as a MemoryContext by readers.
Also, why is this a global var? Exported too?
> +/*
> + * Type of the shared library symbol _PG_recovery_module_init that is looked
> up
> + * when loading a recovery library.
> + */
> +typedef void (*RecoveryModuleInit) (RecoveryModuleCallbacks *cb);
I think this is a bad way to return callbacks. This way the
RecoveryModuleCallbacks needs to be modifiable, which makes the job for the
compiler harder (and isn't the greatest for security).
I strongly encourage you to follow the model used e.g. by tableam. The init
function should return a pointer to a *constant* struct. Which is compile-time
initialized with the function pointers.
See the bottom of heapam_handler.c for how that ends up looking.
> +void
> +LoadRecoveryCallbacks(void)
> +{
> + RecoveryModuleInit init;
> +
> + /*
> + * If the shell command is enabled, use our special initialization
> + * function. Otherwise, load the library and call its
> + * _PG_recovery_module_init().
> + */
> + if (restoreLibrary[0] == '\0')
> + init = shell_restore_init;
> + else
> + init = (RecoveryModuleInit)
> + load_external_function(restoreLibrary,
> "_PG_recovery_module_init",
> +false, NULL);
Why a special rule for shell, instead of just defaulting the GUC to it?
> + /*
> + * If using shell commands, remove callbacks for any commands that are
> not
> + * set.
> + */
> + if (restoreLibrary[0] == '\0')
> + {
> + if (recoveryRestoreCommand[0] == '\0')
> + RecoveryContext.restore_cb = NULL;
> + if (archiveCleanupCommand[0] == '\0')
> + RecoveryContext.archive_cleanup_cb = NULL;
> + if (recoveryEndCommand[0] == '\0')
> + RecoveryContext.recovery_end_cb = NULL;
I'd just mandate that these are implemented and that the module has to handle
if it doesn't need to do anything.
> + /*
> + * Check for invalid combinations of the command/library parameters and
> + * load the callbacks.
> + */
> + CheckMutuallyExclusiveGUCs(restoreLibrary, "restore_library",
> +
> recoveryRestoreCommand, "restore_command");
> + CheckMutuallyExclusiveGUCs(restoreLibrary, "restore_library",
> +recoveryEndCommand,
> "recovery_end_command");
> + before_shmem_exit(call_recovery_module_shutdown_cb, 0);
> + LoadRecoveryCallbacks();
This kind of sequence is duplicated into several places.
Greetings,
Andres Freund
Re: recovery modules
Hi, On 2023-01-27 15:28:21 -0800, Nathan Bossart wrote: > The more I think about this, the more I wonder whether we really need to > include archive_cleanup_command and recovery_end_command in recovery > modules. I think it would be hard to write a good module that isn't just implementing the existing commands without it. Needing to clean up archives and reacting to the end of recovery seems a pretty core task. > Another weird thing with the checkpointer is that the restore_library will > stay loaded long after recovery is finished, and it'll be loaded regardless > of whether recovery is required in the first place. I don't see a problem with that. And I suspect we might even end up there for other reasons. I was briefly wondering whether it'd be worth trying to offload things like archive_cleanup_command from checkpointer to a different process, for robustness. But given that it's pretty much required for performance that the module runs in the startup process, that ship probably has sailed. Greetings, Andres Freund
Re: recovery modules
On Thu, Jan 26, 2023 at 09:40:58PM -0800, Nathan Bossart wrote: > Yeah, there are some problems here. If we ERROR, we'll just bounce back to > the sigsetjmp() block once a second, and we'll never pick up configuration > reloads, shutdown signals, etc. If we FATAL, we'll just rapidly restart > over and over. Given the dicussion about misconfigured archiving > parameters [0], I doubt folks will be okay with giving priority to one or > the other. > > I'm currently thinking that the checkpointer should set a flag and clear > the recovery callbacks when a misconfiguration is detected. Anytime the > checkpointer tries to use the archive-cleanup callback, a WARNING would be > emitted. This is similar to an approach I proposed for archiving > misconfigurations (that we didn't proceed with) [1]. Given the > aformentioned problems, this approach might be more suitable for the > checkpointer than it is for the archiver. So, by doing that, archive_library would be ignored. What should be the checkpointer do when a aconfiguration error is found on misconfiguration? Would archive_cleanup_command be equally ignored or could there be a risk to see it used by the checkpointer? Ignoring it would be fine as the user intended the use of a library, rather than enforcing its use via a value one did not really want. So, on top of cleaning the callbacks, archive_cleanup_command should also be cleaned up in the checkpointer? Issuing one WARNING per checkpoint would be indeed much better than looping over and over, impacting the system reliability. Thoughts or comments from anyone? -- Michael signature.asc Description: PGP signature
Re: recovery modules
On Thu, Jan 26, 2023 at 09:40:58PM -0800, Nathan Bossart wrote: > On Wed, Jan 25, 2023 at 04:34:21PM +0900, Michael Paquier wrote: >> The loop part is annoying.. I've never been a fan of adding this >> cross-value checks for the archiver modules in the first place, and it >> would make things much simpler in the checkpointer if we need to think >> about that as we want these values to be reloadable. Perhaps this >> could just be an exception where we just give priority on one over the >> other archive_cleanup_command? The startup process has a well-defined >> sequence after a failure, while the checkpointer is designed to remain >> robust. > > Yeah, there are some problems here. If we ERROR, we'll just bounce back to > the sigsetjmp() block once a second, and we'll never pick up configuration > reloads, shutdown signals, etc. If we FATAL, we'll just rapidly restart > over and over. Given the dicussion about misconfigured archiving > parameters [0], I doubt folks will be okay with giving priority to one or > the other. > > I'm currently thinking that the checkpointer should set a flag and clear > the recovery callbacks when a misconfiguration is detected. Anytime the > checkpointer tries to use the archive-cleanup callback, a WARNING would be > emitted. This is similar to an approach I proposed for archiving > misconfigurations (that we didn't proceed with) [1]. Given the > aformentioned problems, this approach might be more suitable for the > checkpointer than it is for the archiver. The more I think about this, the more I wonder whether we really need to include archive_cleanup_command and recovery_end_command in recovery modules. Another weird thing with the checkpointer is that the restore_library will stay loaded long after recovery is finished, and it'll be loaded regardless of whether recovery is required in the first place. Of course, that typically won't cause any problems, and we could wait until we need to do archive cleanup to load the library (and call its shutdown callback when recovery is finished), but this strikes me as potentially more complexity than the feature is worth. Perhaps we should just focus on covering the restore_command functionality for now and add the rest later. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: recovery modules
On Wed, Jan 25, 2023 at 04:34:21PM +0900, Michael Paquier wrote: > The loop part is annoying.. I've never been a fan of adding this > cross-value checks for the archiver modules in the first place, and it > would make things much simpler in the checkpointer if we need to think > about that as we want these values to be reloadable. Perhaps this > could just be an exception where we just give priority on one over the > other archive_cleanup_command? The startup process has a well-defined > sequence after a failure, while the checkpointer is designed to remain > robust. Yeah, there are some problems here. If we ERROR, we'll just bounce back to the sigsetjmp() block once a second, and we'll never pick up configuration reloads, shutdown signals, etc. If we FATAL, we'll just rapidly restart over and over. Given the dicussion about misconfigured archiving parameters [0], I doubt folks will be okay with giving priority to one or the other. I'm currently thinking that the checkpointer should set a flag and clear the recovery callbacks when a misconfiguration is detected. Anytime the checkpointer tries to use the archive-cleanup callback, a WARNING would be emitted. This is similar to an approach I proposed for archiving misconfigurations (that we didn't proceed with) [1]. Given the aformentioned problems, this approach might be more suitable for the checkpointer than it is for the archiver. Thoughts? [0] https://postgr.es/m/9ee5d180-2c32-a1ca-d3d7-63a723f68d9a%40enterprisedb.com [1] https://postgr.es/m/20220914222736.GA3042279%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: recovery modules
the >> checkpointer reports an error but it does not enforce a process >> restart, hence it keeps around the new, incorrect, configuration while >> waiting for a new checkpoint to happen once restore_library and >> archive_cleanup_command are set. This could lead to surprises, IMO. >> Upgrading to a FATAL in this code path triggers an infinite loop, like >> the startup path. > > If we move the parameter check in CheckpointerMain() as described above, > the checkpointer should be unable to proceed with an incorrect > configuration. For the normal shutdown part, do you think the > ShutdownRequestPending block should be moved to before the > ConfigReloadPending block in HandleCheckpointerInterrupts()? I would not touch this order. This could influence the setup a shutdown checkpoint relies on, for one. >> If the archive_cleanup_command ballback of a restore library triggers >> a FATAL, it seems to me that it would continuously trigger a server >> restart, actually. Perhaps that's something to document, in >> comparison to the safe fallbacks of the shell command where we don't >> force an ERROR to give priority to the stability of the checkpointer. > > I'm not sure it's worth documenting that ereport(FATAL, ...) in the > checkpointer process will cause a server restart. In most cases, an > extension author would use ERROR, which, if we make the aforementioned > changes, would at most cause the checkpointer to effectively restart. This > is similar to archive modules where an ERROR causes only the archiver > process to restart. Also, we document that recovery libraries are loaded > in the startup and checkpointer processes, so IMO it should be relatively > apparent that doing something like FATAL or proc_exit() is bad. Okay. Fine by me. This could always be amended later, as required. + if (recoveryRestoreCommand[0] == '\0') + RecoveryContext.restore_cb = NULL; + if (archiveCleanupCommand[0] == '\0') + RecoveryContext.archive_cleanup_cb = NULL; + if (recoveryEndCommand[0] == '\0') + RecoveryContext.recovery_end_cb = NULL; Could it be cleaner to put this knowledge directly in shell_restore.c with a fast-exit path after entering each callback? It does not strike me as a good thing to sprinkle more than necessary the knowledge about the commands. Another question that popped in my mind: could it be better to have two different shutdown callbacks for the checkpointer and the startup process? Having some tests for both, like shell_archive.c, would be nice, actually. -- Michael From 7d4425744e2bc96418d6ef85c9408bed9e91aa9b Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 25 Jan 2023 15:39:23 +0900 Subject: [PATCH v11 1/3] doc: Refactor the chapter related to archive modules to be WAL modules Archive modules are now a subsection of this more generic section, and recovery modules would be a second subsection of it. --- doc/src/sgml/archive-modules.sgml | 136 -- doc/src/sgml/backup.sgml | 2 +- doc/src/sgml/basic-wal-module.sgml | 2 +- doc/src/sgml/config.sgml | 2 +- doc/src/sgml/filelist.sgml | 2 +- doc/src/sgml/postgres.sgml | 2 +- doc/src/sgml/system-views.sgml | 2 +- doc/src/sgml/wal-modules.sgml | 149 + 8 files changed, 155 insertions(+), 142 deletions(-) delete mode 100644 doc/src/sgml/archive-modules.sgml create mode 100644 doc/src/sgml/wal-modules.sgml diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml deleted file mode 100644 index 1a32006e2c..00 --- a/doc/src/sgml/archive-modules.sgml +++ /dev/null @@ -1,136 +0,0 @@ - - - - Archive Modules - - Archive Modules - - - - PostgreSQL provides infrastructure to create custom modules for continuous - archiving (see ). While archiving via - a shell command (i.e., ) is much - simpler, a custom archive module will often be considerably more robust and - performant. - - - - When a custom is configured, PostgreSQL - will submit completed WAL files to the module, and the server will avoid - recycling or removing these WAL files until the module indicates that the files - were successfully archived. It is ultimately up to the module to decide what - to do with each WAL file, but many recommendations are listed at - . - - - - Archiving modules must at least consist of an initialization function (see - ) and the required callbacks (see - ). However, archive modules are - also permitted to do much more (e.g., declare GUCs and register background - workers). - - - - The contrib/basic_wal_module module contains a working - example, which demonstrates some useful techniques. - - - - Initialization Functions - - _PG_archive_module_init - - - An archive library is loaded by dynamically loadin
Re: recovery modules
On Mon, Jan 23, 2023 at 11:44:57AM +0900, Michael Paquier wrote:
> Thanks for the rebase.
Thanks for the detailed review.
> The final state of the documentation is as follows:
> 51. Archive and Recovery Modules
> 51.1. Archive Module Initialization Functions
> 51.2. Archive Module Callbacks
> 51.3. Recovery Module Initialization Functions
> 51.4. Recovery Module Callbacks
>
> I am not completely sure whether this grouping is the best thing to
> do. Wouldn't it be better to move that into two different
> sub-sections instead? One layout suggestion:
> 51. WAL Modules
> 51.1. Archive Modules
> 51.1.1. Archive Module Initialization Functions
> 51.1.2. Archive Module Callbacks
> 51.2. Recovery Modules
> 51.2.1. Recovery Module Initialization Functions
> 51.2.2. Recovery Module Callbacks
>
> Putting both of them in the same section sounds like a good idea per
> the symmetry that one would likely have between the code paths of
> archiving and recovery, so as they share some common knowledge.
>
> This kinds of comes back to the previous suggestion to rename
> basic_archive to something like wal_modules, that covers both
> archiving and recovery. I does not seem like this would overlap with
> RMGRs, which is much more internal anyway.
I updated the documentation as you suggested, and I renamed basic_archive
to basic_wal_module.
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> - errmsg("must specify restore_command when standby mode is not
> enabled")));
> + errmsg("must specify restore_command or a restore_library that defines "
> +"a restore callback when standby mode is not enabled")));
> This is long. Shouldn't this be split into an errdetail() to list all
> the options at hand?
Should the errmsg() be something like "recovery parameters misconfigured"?
> - if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
> - ereport(ERROR,
> - (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> -errmsg("both archive_command and archive_library set"),
> -errdetail("Only one of archive_command, archive_library may
> be set.")));
> + CheckMutuallyExclusiveGUCs(XLogArchiveLibrary, "archive_library",
> + XLogArchiveCommand, "archive_command");
>
> The introduction of this routine could be a patch on its own, as it
> impacts the archiving path.
I moved this to a separate patch.
> - Startup reloading, as of StartupRereadConfig(). This code could
> involve a WAL receiver restart depending on a change in the slot
> change or in primary_conninfo, and force at the same time an ERROR
> because of conflicting recovery library and command configuration.
> This one should be safe because pendingWalRcvRestart would just be
> considered later on by the startup process itself while waiting for
> WAL to become available. Still this could deserve a comment? Even if
> there is a misconfiguration, a reload on a standby would enforce a
> FATAL in the startup process, taking down the whole server.
Do you think the parameter checks should go before the WAL receiver restart
logic?
> - Checkpointer initialization, as of CheckpointerMain(). A
> configuration failure in this code path, aka server startup, causes
> the server to loop infinitely on FATAL with the misconfiguration
> showing up all the time.. This is a problem.
Perhaps this is a reason to move the parameter check in CheckpointerMain()
to after the sigsetjmp() block. This should avoid full server restarts.
Only the checkpointer process would loop with the ERROR.
> - Last comes the checkpointer GUC reloading, as of
> HandleCheckpointerInterrupts(), with a second problem. This
> introduces a failure path where ConfigReloadPending is processed at
> the same time as ShutdownRequestPending based on the way it is coded,
> interacting with what would be a normal shutdown in some cases? And
> actually, if you enforce a misconfiguration on reload, the
> checkpointer reports an error but it does not enforce a process
> restart, hence it keeps around the new, incorrect, configuration while
> waiting for a new checkpoint to happen once restore_library and
> archive_cleanup_command are set. This could lead to surprises, IMO.
> Upgrading to a FATAL in this code path triggers an infinite loop, like
> the startup path.
If we move the parameter check in CheckpointerMain() as described above,
the checkpointer should be unable to proceed with an incorrect
configuration. For the normal shutdown part, do you think the
ShutdownRequestPending block should be moved to before the
ConfigReloadPending block in HandleCheckpointerInterrupt
Re: recovery modules
On Tue, Jan 17, 2023 at 08:44:27PM -0800, Nathan Bossart wrote:
> Thanks. Here's a rebased version of the last patch.
Thanks for the rebase.
The final state of the documentation is as follows:
51. Archive and Recovery Modules
51.1. Archive Module Initialization Functions
51.2. Archive Module Callbacks
51.3. Recovery Module Initialization Functions
51.4. Recovery Module Callbacks
I am not completely sure whether this grouping is the best thing to
do. Wouldn't it be better to move that into two different
sub-sections instead? One layout suggestion:
51. WAL Modules
51.1. Archive Modules
51.1.1. Archive Module Initialization Functions
51.1.2. Archive Module Callbacks
51.2. Recovery Modules
51.2.1. Recovery Module Initialization Functions
51.2.2. Recovery Module Callbacks
Putting both of them in the same section sounds like a good idea per
the symmetry that one would likely have between the code paths of
archiving and recovery, so as they share some common knowledge.
This kinds of comes back to the previous suggestion to rename
basic_archive to something like wal_modules, that covers both
archiving and recovery. I does not seem like this would overlap with
RMGRs, which is much more internal anyway.
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("must specify restore_command when standby mode is not enabled")));
+ errmsg("must specify restore_command or a restore_library that defines "
+"a restore callback when standby mode is not enabled")));
This is long. Shouldn't this be split into an errdetail() to list all
the options at hand?
- if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-errmsg("both archive_command and archive_library set"),
-errdetail("Only one of archive_command, archive_library may be
set.")));
+ CheckMutuallyExclusiveGUCs(XLogArchiveLibrary, "archive_library",
+ XLogArchiveCommand, "archive_command");
The introduction of this routine could be a patch on its own, as it
impacts the archiving path.
+ CheckMutuallyExclusiveGUCs(restoreLibrary, "restore_library",
+ archiveCleanupCommand,
"archive_cleanup_command");
+ if (strcmp(prevRestoreLibrary, restoreLibrary) != 0 ||
+ strcmp(prevArchiveCleanupCommand, archiveCleanupCommand) != 0)
+ {
+ call_recovery_module_shutdown_cb(0, (Datum) 0);
+ LoadRecoveryCallbacks();
+ }
+
+ pfree(prevRestoreLibrary);
+ pfree(prevArchiveCleanupCommand);
Hm.. The callers of CheckMutuallyExclusiveGUCs() with the new ERROR
paths they introduce need a close lookup. As far as I can see this
concerns four areas depending on the three restore commands
(restore_command and recovery_end command for startup,
archive_cleanup_command for the checkpointer):
- Startup process initialization, as of validateRecoveryParameters()
where the postmaster GUCs for the recovery target are evaluated. This
one is an early stage which is fine.
- Startup reloading, as of StartupRereadConfig(). This code could
involve a WAL receiver restart depending on a change in the slot
change or in primary_conninfo, and force at the same time an ERROR
because of conflicting recovery library and command configuration.
This one should be safe because pendingWalRcvRestart would just be
considered later on by the startup process itself while waiting for
WAL to become available. Still this could deserve a comment? Even if
there is a misconfiguration, a reload on a standby would enforce a
FATAL in the startup process, taking down the whole server.
- Checkpointer initialization, as of CheckpointerMain(). A
configuration failure in this code path, aka server startup, causes
the server to loop infinitely on FATAL with the misconfiguration
showing up all the time.. This is a problem.
- Last comes the checkpointer GUC reloading, as of
HandleCheckpointerInterrupts(), with a second problem. This
introduces a failure path where ConfigReloadPending is processed at
the same time as ShutdownRequestPending based on the way it is coded,
interacting with what would be a normal shutdown in some cases? And
actually, if you enforce a misconfiguration on reload, the
checkpointer reports an error but it does not enforce a process
restart, hence it keeps around the new, incorrect, configuration while
waiting for a new checkpoint to happen once restore_library and
archive_cleanup_command are set. This could lead to surprises, IMO.
Upgrading to a FATAL in this code path triggers an infinite loop, like
the startup path.
If the archive_cleanup_command ballback of a restore library triggers
a FATAL, it seems to me that it would continuously trigger a server
restart, actually. Perhaps
Re: recovery modules
rue;
@@ -368,3 +389,45 @@ compare_files(const char *file1, const char *file2)
return ret;
}
+
+/*
+ * basic_restore_file
+ *
+ * Retrieves one file from the WAL archives.
+ */
+static bool
+basic_restore_file(const char *file, const char *path,
+ const char *lastRestartPointFileName)
+{
+ char source[MAXPGPATH];
+ struct stat st;
+
+ ereport(DEBUG1,
+ (errmsg("restoring \"%s\" via basic_archive", file)));
+
+ if (archive_directory == NULL || archive_directory[0] == '\0')
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"basic_archive.archive_directory\" is not set")));
+
+ /*
+ * Check whether the file exists. If not, we return false to indicate that
+ * there are no more files to restore.
+ */
+ snprintf(source, MAXPGPATH, "%s/%s", archive_directory, file);
+ if (stat(source, ) != 0)
+ {
+ int elevel = (errno == ENOENT) ? DEBUG1 : ERROR;
+
+ ereport(elevel,
+(errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", source)));
+ return false;
+ }
+
+ copy_file(source, path);
+
+ ereport(DEBUG1,
+ (errmsg("restored \"%s\" via basic_archive", file)));
+ return true;
+}
diff --git a/contrib/basic_archive/meson.build b/contrib/basic_archive/meson.build
index bc1380e6f6..af4580dea9 100644
--- a/contrib/basic_archive/meson.build
+++ b/contrib/basic_archive/meson.build
@@ -7,7 +7,7 @@ basic_archive_sources = files(
if host_system == 'windows'
basic_archive_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
'--NAME', 'basic_archive',
-'--FILEDESC', 'basic_archive - basic archive module',])
+'--FILEDESC', 'basic_archive - basic archive and recovery module',])
endif
basic_archive = shared_module('basic_archive',
@@ -31,4 +31,9 @@ tests += {
# which typical runningcheck users do not have (e.g. buildfarm clients).
'runningcheck': false,
},
+ 'tap': {
+'tests': [
+ 't/001_restore.pl',
+],
+ },
}
diff --git a/contrib/basic_archive/t/001_restore.pl b/contrib/basic_archive/t/001_restore.pl
new file mode 100644
index 00..ec8767d740
--- /dev/null
+++ b/contrib/basic_archive/t/001_restore.pl
@@ -0,0 +1,44 @@
+
+# Copyright (c) 2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# start a node
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(has_archiving => 1, allows_streaming => 1);
+my $archive_dir = $node->archive_dir;
+$archive_dir =~ s!\\!/!g if $PostgreSQL::Test::Utils::windows_os;
+$node->append_conf('postgresql.conf', "archive_command = ''");
+$node->append_conf('postgresql.conf', "archive_library = 'basic_archive'");
+$node->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$node->start;
+
+# backup the node
+my $backup = 'backup';
+$node->backup($backup);
+
+# generate some new WAL files
+$node->safe_psql('postgres', "CREATE TABLE test (a INT);");
+$node->safe_psql('postgres', "SELECT pg_switch_wal();");
+$node->safe_psql('postgres', "INSERT INTO test VALUES (1);");
+
+# shut down the node (this should archive all WAL files)
+$node->stop;
+
+# restore from the backup
+my $restore = PostgreSQL::Test::Cluster->new('restore');
+$restore->init_from_backup($node, $backup, has_restoring => 1, standby => 0);
+$restore->append_conf('postgresql.conf', "restore_command = ''");
+$restore->append_conf('postgresql.conf', "restore_library = 'basic_archive'");
+$restore->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$restore->start;
+
+# ensure post-backup WAL was replayed
+my $result = $restore->safe_psql("postgres", "SELECT count(*) FROM test;");
+is($result, "1", "check restore content");
+
+done_testing();
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..53e657040b 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -1,34 +1,40 @@
- Archive Modules
+ Archive and Recovery Modules
- Archive Modules
+ Archive and Recovery Modules
PostgreSQL provides infrastructure to create custom modules for continuous
- archiving (see ). While archiving via
- a shell command (i.e., ) is much
- simpler, a custom archive module will often be considerably more robust and
- performant.
+ archiving and recovery (see ). While
+ a shell command (e.g., ,
+ ) is much simpler, a custom module will
+ often be considerably more robust and performant.
When a custom is configured, PostgreSQL
will submit completed WAL files to the module, and the server will avoid
recycling or removing these WAL files until the module
Re: recovery modules
On Tue, Jan 17, 2023 at 10:23:56AM -0800, Nathan Bossart wrote:
> Yeah, this seems cleaner. I removed BuildRestoreCommand() in v8.
if (*sp == *lp)
{
- if (val)
- {
- appendStringInfoString(, val);
- found = true;
- }
- /* If val is NULL, we will report an error. */
+ appendStringInfoString(, val);
+ found = true;
In 0002, this code block has been removed as an effect of the removal
of BuildRestoreCommand(), because RestoreArchivedFile() needs to
handle two flags with two values. The current design has the
advantage to warn extension developers with an unexpected
manipulation, as well, so I have kept the logic in percentrepl.c
as-is.
I was wondering also if ExecuteRecoveryCommand() should use a bits32
for its two boolean flags, but did not bother as it is static in
shell_restore.c so ABI does not matter, even if there are three
callers of it with 75% of the combinations possible (only false/true
is not used).
And 0002 is applied.
--
Michael
signature.asc
Description: PGP signature
Re: recovery modules
eck whether the file exists. If not, we return false to indicate that
+ * there are no more files to restore.
+ */
+ snprintf(source, MAXPGPATH, "%s/%s", archive_directory, file);
+ if (stat(source, ) != 0)
+ {
+ int elevel = (errno == ENOENT) ? DEBUG1 : ERROR;
+
+ ereport(elevel,
+(errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", source)));
+ return false;
+ }
+
+ copy_file(source, unconstify(char *, path));
+
+ ereport(DEBUG1,
+ (errmsg("restored \"%s\" via basic_archive", file)));
+ return true;
+}
diff --git a/contrib/basic_archive/meson.build b/contrib/basic_archive/meson.build
index bc1380e6f6..af4580dea9 100644
--- a/contrib/basic_archive/meson.build
+++ b/contrib/basic_archive/meson.build
@@ -7,7 +7,7 @@ basic_archive_sources = files(
if host_system == 'windows'
basic_archive_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
'--NAME', 'basic_archive',
-'--FILEDESC', 'basic_archive - basic archive module',])
+'--FILEDESC', 'basic_archive - basic archive and recovery module',])
endif
basic_archive = shared_module('basic_archive',
@@ -31,4 +31,9 @@ tests += {
# which typical runningcheck users do not have (e.g. buildfarm clients).
'runningcheck': false,
},
+ 'tap': {
+'tests': [
+ 't/001_restore.pl',
+],
+ },
}
diff --git a/contrib/basic_archive/t/001_restore.pl b/contrib/basic_archive/t/001_restore.pl
new file mode 100644
index 00..ec8767d740
--- /dev/null
+++ b/contrib/basic_archive/t/001_restore.pl
@@ -0,0 +1,44 @@
+
+# Copyright (c) 2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# start a node
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(has_archiving => 1, allows_streaming => 1);
+my $archive_dir = $node->archive_dir;
+$archive_dir =~ s!\\!/!g if $PostgreSQL::Test::Utils::windows_os;
+$node->append_conf('postgresql.conf', "archive_command = ''");
+$node->append_conf('postgresql.conf', "archive_library = 'basic_archive'");
+$node->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$node->start;
+
+# backup the node
+my $backup = 'backup';
+$node->backup($backup);
+
+# generate some new WAL files
+$node->safe_psql('postgres', "CREATE TABLE test (a INT);");
+$node->safe_psql('postgres', "SELECT pg_switch_wal();");
+$node->safe_psql('postgres', "INSERT INTO test VALUES (1);");
+
+# shut down the node (this should archive all WAL files)
+$node->stop;
+
+# restore from the backup
+my $restore = PostgreSQL::Test::Cluster->new('restore');
+$restore->init_from_backup($node, $backup, has_restoring => 1, standby => 0);
+$restore->append_conf('postgresql.conf', "restore_command = ''");
+$restore->append_conf('postgresql.conf', "restore_library = 'basic_archive'");
+$restore->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$restore->start;
+
+# ensure post-backup WAL was replayed
+my $result = $restore->safe_psql("postgres", "SELECT count(*) FROM test;");
+is($result, "1", "check restore content");
+
+done_testing();
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..53e657040b 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -1,34 +1,40 @@
- Archive Modules
+ Archive and Recovery Modules
- Archive Modules
+ Archive and Recovery Modules
PostgreSQL provides infrastructure to create custom modules for continuous
- archiving (see ). While archiving via
- a shell command (i.e., ) is much
- simpler, a custom archive module will often be considerably more robust and
- performant.
+ archiving and recovery (see ). While
+ a shell command (e.g., ,
+ ) is much simpler, a custom module will
+ often be considerably more robust and performant.
When a custom is configured, PostgreSQL
will submit completed WAL files to the module, and the server will avoid
recycling or removing these WAL files until the module indicates that the files
- were successfully archived. It is ultimately up to the module to decide what
- to do with each WAL file, but many recommendations are listed at
- .
+ were successfully archived. When a custom
+ is configured, PostgreSQL will use the
+ module for recovery actions. It is ultimately up to the module to decide how
+ to accomplish each task, but some recommendations are listed at
+ and
+ .
- Archiving modules must at least consist of an initialization function (see
- ) and the required callbacks (see
- ). However, archive modules are
- also permitted to do much more (e.g., declare GUCs and
Re: recovery modules
On Mon, Jan 16, 2023 at 02:40:40PM -0800, Nathan Bossart wrote: > On Mon, Jan 16, 2023 at 04:36:01PM +0900, Michael Paquier wrote: > > Once this issue was fixed, nothing else stood out, so applied this > > part. > > Thanks! I've attached a rebased version of the rest of the patch set. When it comes to 0002, the only difference between the three code paths of shell_recovery_end(), shell_archive_cleanup() and shell_restore() is the presence of BuildRestoreCommand(). However this is now just a thin wrapper of replace_percent_placeholders() that does just one extra make_native_path() for the xlogpath. Could it be cleaner in the long term to remove entirely BuildRestoreCommand() and move the conversion of the xlogpath with make_native_path() one level higher in the stack? -- Michael signature.asc Description: PGP signature
Re: recovery modules
st::Cluster->new('restore');
+$restore->init_from_backup($node, $backup, has_restoring => 1, standby => 0);
+$restore->append_conf('postgresql.conf', "restore_command = ''");
+$restore->append_conf('postgresql.conf', "restore_library = 'basic_archive'");
+$restore->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$restore->start;
+
+# ensure post-backup WAL was replayed
+my $result = $restore->safe_psql("postgres", "SELECT count(*) FROM test;");
+is($result, "1", "check restore content");
+
+done_testing();
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..53e657040b 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -1,34 +1,40 @@
- Archive Modules
+ Archive and Recovery Modules
- Archive Modules
+ Archive and Recovery Modules
PostgreSQL provides infrastructure to create custom modules for continuous
- archiving (see ). While archiving via
- a shell command (i.e., ) is much
- simpler, a custom archive module will often be considerably more robust and
- performant.
+ archiving and recovery (see ). While
+ a shell command (e.g., ,
+ ) is much simpler, a custom module will
+ often be considerably more robust and performant.
When a custom is configured, PostgreSQL
will submit completed WAL files to the module, and the server will avoid
recycling or removing these WAL files until the module indicates that the files
- were successfully archived. It is ultimately up to the module to decide what
- to do with each WAL file, but many recommendations are listed at
- .
+ were successfully archived. When a custom
+ is configured, PostgreSQL will use the
+ module for recovery actions. It is ultimately up to the module to decide how
+ to accomplish each task, but some recommendations are listed at
+ and
+ .
- Archiving modules must at least consist of an initialization function (see
- ) and the required callbacks (see
- ). However, archive modules are
- also permitted to do much more (e.g., declare GUCs and register background
- workers).
+ Archive and recovery modules must at least consist of an initialization
+ function (see and
+ ) and the required callbacks (see
+ and
+ ). However, archive and recovery
+ modules are also permitted to do much more (e.g., declare GUCs and register
+ background workers). A module may be used for both
+ archive_library and restore_library.
@@ -37,7 +43,7 @@
- Initialization Functions
+ Archive Module Initialization Functions
_PG_archive_module_init
@@ -64,6 +70,12 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
Only the archive_file_cb callback is required. The
others are optional.
+
+
+
+archive_library is only loaded in the archiver process.
+
+
@@ -129,6 +141,132 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
typedef void (*ArchiveShutdownCB) (void);
+
+
+
+
+
+
+ Recovery Module Initialization Functions
+
+ _PG_recovery_module_init
+
+
+ A recovery library is loaded by dynamically loading a shared library with the
+as the library base name. The normal
+ library search path is used to locate the library. To provide the required
+ recovery module callbacks and to indicate that the library is actually a
+ recovery module, it needs to provide a function named
+ _PG_recovery_module_init. This function is passed a
+ struct that needs to be filled with the callback function pointers for
+ individual actions.
+
+
+typedef struct RecoveryModuleCallbacks
+{
+RecoveryRestoreCB restore_cb;
+RecoveryArchiveCleanupCB archive_cleanup_cb;
+RecoveryEndCB recovery_end_cb;
+RecoveryShutdownCB shutdown_cb;
+} RecoveryModuleCallbacks;
+typedef void (*RecoveryModuleInit) (struct RecoveryModuleCallbacks *cb);
+
+
+ The restore_cb callback is required for archive
+ recovery, but it is optional for streaming replication. The others are
+ always optional.
+
+
+
+
+restore_library is only loaded in the startup and
+checkpointer processes and in single-user mode.
+
+
+
+
+
+ Recovery Module Callbacks
+
+ The recovery callbacks define the actual behavior of the module. The server
+ will call them as required to execute recovery actions.
+
+
+
+ Restore Callback
+
+The restore_cb callback is called to retrieve a single
+archived segment of the WAL file series for archive recovery or streaming
+replication.
+
+
+typedef bool (*RecoveryRestoreCB) (const char *file, const char *path, const char *lastRestartPointFileName);
+
+
+This callback must return true only if the file was
+successfully retrieved. If the file is not available in the archives, the
+callback
Re: recovery modules
On Thu, Jan 12, 2023 at 10:17:21AM -0800, Nathan Bossart wrote:
> On Thu, Jan 12, 2023 at 03:30:40PM +0900, Michael Paquier wrote:
> IMHO I don't think there's an urgent need to rename it, but if there's a
> better name that people like, I'm happy to do so.
Okay.
>> Saying that, 0001 seems fine on its own (minus the redo LSN/TLI with
>> the duplication for the segment name build), so I would be tempted to
>> get this one done. My gut tells me that we'd better remove the
>> duplication and just pass down the two fields to
>> shell_archive_cleanup() and shell_recovery_end(), with the segment
>> name given to ExecuteRecoveryCommand()..
>
> I moved the duplicated logic to its own function in v6.
While looking at 0001, I have noticed one issue as of the following
block in shell_restore():
+ if (wait_result_is_signal(rc, SIGTERM))
+ proc_exit(1);
+
+ ereport(wait_result_is_any_signal(rc, true) ? FATAL : DEBUG2,
+ (errmsg("could not restore file \"%s\" from archive: %s",
+ file, wait_result_to_str(rc;
This block of code would have been executed even if rc == 0, which is
incorrect because the command would have succeeded. HEAD would not
have done that, actually, as RestoreArchivedFile() would return
before. I guess that you have not noticed it because this basically
just generated incorrect DEBUG2 messages when rc == 0?
One part that this slightly influences is the order of the reports
when the command succeeds the follow-up stat() fails to check the
size, where we reverse these two logs:
DEBUG2, "could not restore"
LOG/FATAL, "could not stat file"
However, that does not really change the value of the information
reported: if the stat() failed, the failure mode is the same except
that we don't get the extra DEBUG2/"could not restore" message, which
does not really matter except if your elevel is high enough for
that and there is always the LOG for that..
Once this issue was fixed, nothing else stood out, so applied this
part.
--
Michael
signature.asc
Description: PGP signature
Re: recovery modules
);
+ if (stat(source, ) != 0)
+ {
+ int elevel = (errno == ENOENT) ? DEBUG1 : ERROR;
+
+ ereport(elevel,
+(errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", source)));
+ return false;
+ }
+
+ copy_file(source, unconstify(char *, path));
+
+ ereport(DEBUG1,
+ (errmsg("restored \"%s\" via basic_archive", file)));
+ return true;
+}
diff --git a/contrib/basic_archive/meson.build b/contrib/basic_archive/meson.build
index bc1380e6f6..af4580dea9 100644
--- a/contrib/basic_archive/meson.build
+++ b/contrib/basic_archive/meson.build
@@ -7,7 +7,7 @@ basic_archive_sources = files(
if host_system == 'windows'
basic_archive_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
'--NAME', 'basic_archive',
-'--FILEDESC', 'basic_archive - basic archive module',])
+'--FILEDESC', 'basic_archive - basic archive and recovery module',])
endif
basic_archive = shared_module('basic_archive',
@@ -31,4 +31,9 @@ tests += {
# which typical runningcheck users do not have (e.g. buildfarm clients).
'runningcheck': false,
},
+ 'tap': {
+'tests': [
+ 't/001_restore.pl',
+],
+ },
}
diff --git a/contrib/basic_archive/t/001_restore.pl b/contrib/basic_archive/t/001_restore.pl
new file mode 100644
index 00..ec8767d740
--- /dev/null
+++ b/contrib/basic_archive/t/001_restore.pl
@@ -0,0 +1,44 @@
+
+# Copyright (c) 2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# start a node
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(has_archiving => 1, allows_streaming => 1);
+my $archive_dir = $node->archive_dir;
+$archive_dir =~ s!\\!/!g if $PostgreSQL::Test::Utils::windows_os;
+$node->append_conf('postgresql.conf', "archive_command = ''");
+$node->append_conf('postgresql.conf', "archive_library = 'basic_archive'");
+$node->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$node->start;
+
+# backup the node
+my $backup = 'backup';
+$node->backup($backup);
+
+# generate some new WAL files
+$node->safe_psql('postgres', "CREATE TABLE test (a INT);");
+$node->safe_psql('postgres', "SELECT pg_switch_wal();");
+$node->safe_psql('postgres', "INSERT INTO test VALUES (1);");
+
+# shut down the node (this should archive all WAL files)
+$node->stop;
+
+# restore from the backup
+my $restore = PostgreSQL::Test::Cluster->new('restore');
+$restore->init_from_backup($node, $backup, has_restoring => 1, standby => 0);
+$restore->append_conf('postgresql.conf', "restore_command = ''");
+$restore->append_conf('postgresql.conf', "restore_library = 'basic_archive'");
+$restore->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$restore->start;
+
+# ensure post-backup WAL was replayed
+my $result = $restore->safe_psql("postgres", "SELECT count(*) FROM test;");
+is($result, "1", "check restore content");
+
+done_testing();
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..53e657040b 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -1,34 +1,40 @@
- Archive Modules
+ Archive and Recovery Modules
- Archive Modules
+ Archive and Recovery Modules
PostgreSQL provides infrastructure to create custom modules for continuous
- archiving (see ). While archiving via
- a shell command (i.e., ) is much
- simpler, a custom archive module will often be considerably more robust and
- performant.
+ archiving and recovery (see ). While
+ a shell command (e.g., ,
+ ) is much simpler, a custom module will
+ often be considerably more robust and performant.
When a custom is configured, PostgreSQL
will submit completed WAL files to the module, and the server will avoid
recycling or removing these WAL files until the module indicates that the files
- were successfully archived. It is ultimately up to the module to decide what
- to do with each WAL file, but many recommendations are listed at
- .
+ were successfully archived. When a custom
+ is configured, PostgreSQL will use the
+ module for recovery actions. It is ultimately up to the module to decide how
+ to accomplish each task, but some recommendations are listed at
+ and
+ .
- Archiving modules must at least consist of an initialization function (see
- ) and the required callbacks (see
- ). However, archive modules are
- also permitted to do much more (e.g., declare GUCs and register background
- workers).
+ Archive and recovery modules must at least consist of an initialization
+ function (see and
+ ) and the required callbacks (see
+ and
+ ). However, archiv
Re: recovery modules
On Wed, Jan 11, 2023 at 11:29:01AM -0800, Nathan Bossart wrote: > I'm having trouble thinking of any practical advantage of providing the > redo LSN and TLI. If the main use-case is removing older archives as the > documentation indicates, it seems better to provide the file name so that > you can plug it straight into strcmp() to determine whether the file can be > removed (i.e., what pg_archivecleanup does). If we provided the LSN and > TLI instead, you'd either need to convert that into a WAL file name for > strcmp(), or you'd need to convert the candidate file name into an LSN and > TLI and compare against those. Logging was one thing that came immediately in mind, to let the module know the redo LSN and TLI the segment name was built from without having to recalculate it back. If you don't feel strongly about that, I am fine to discard this remark. It is not like this hook should be set in stone across major releases, in any case. > I initially created a separate basic_restore module, but decided to fold it > into basic_archive to simplify the patch and tests. I hesitated to rename > it because it already exists in v15, and since it deals with creating and > restoring archive files, the name still seemed somewhat accurate. That > being said, I don't mind renaming it if that's what folks want. I've done that in the past for pg_verify_checksums -> pg_checksums, so I would not mind renaming it so as it reflects better its role. (Being outvoted is fine for me if this suggestion sounds bad). Saying that, 0001 seems fine on its own (minus the redo LSN/TLI with the duplication for the segment name build), so I would be tempted to get this one done. My gut tells me that we'd better remove the duplication and just pass down the two fields to shell_archive_cleanup() and shell_recovery_end(), with the segment name given to ExecuteRecoveryCommand().. -- Michael signature.asc Description: PGP signature
Re: recovery modules
e)));
+
+ if (archive_directory == NULL || archive_directory[0] == '\0')
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"basic_archive.archive_directory\" is not set")));
+
+ /*
+ * Check whether the file exists. If not, we return false to indicate that
+ * there are no more files to restore.
+ */
+ snprintf(source, MAXPGPATH, "%s/%s", archive_directory, file);
+ if (stat(source, ) != 0)
+ {
+ int elevel = (errno == ENOENT) ? DEBUG1 : ERROR;
+
+ ereport(elevel,
+(errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", source)));
+ return false;
+ }
+
+ copy_file(source, unconstify(char *, path));
+
+ ereport(DEBUG1,
+ (errmsg("restored \"%s\" via basic_archive", file)));
+ return true;
+}
diff --git a/contrib/basic_archive/meson.build b/contrib/basic_archive/meson.build
index bc1380e6f6..af4580dea9 100644
--- a/contrib/basic_archive/meson.build
+++ b/contrib/basic_archive/meson.build
@@ -7,7 +7,7 @@ basic_archive_sources = files(
if host_system == 'windows'
basic_archive_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
'--NAME', 'basic_archive',
-'--FILEDESC', 'basic_archive - basic archive module',])
+'--FILEDESC', 'basic_archive - basic archive and recovery module',])
endif
basic_archive = shared_module('basic_archive',
@@ -31,4 +31,9 @@ tests += {
# which typical runningcheck users do not have (e.g. buildfarm clients).
'runningcheck': false,
},
+ 'tap': {
+'tests': [
+ 't/001_restore.pl',
+],
+ },
}
diff --git a/contrib/basic_archive/t/001_restore.pl b/contrib/basic_archive/t/001_restore.pl
new file mode 100644
index 00..ec8767d740
--- /dev/null
+++ b/contrib/basic_archive/t/001_restore.pl
@@ -0,0 +1,44 @@
+
+# Copyright (c) 2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# start a node
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(has_archiving => 1, allows_streaming => 1);
+my $archive_dir = $node->archive_dir;
+$archive_dir =~ s!\\!/!g if $PostgreSQL::Test::Utils::windows_os;
+$node->append_conf('postgresql.conf', "archive_command = ''");
+$node->append_conf('postgresql.conf', "archive_library = 'basic_archive'");
+$node->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$node->start;
+
+# backup the node
+my $backup = 'backup';
+$node->backup($backup);
+
+# generate some new WAL files
+$node->safe_psql('postgres', "CREATE TABLE test (a INT);");
+$node->safe_psql('postgres', "SELECT pg_switch_wal();");
+$node->safe_psql('postgres', "INSERT INTO test VALUES (1);");
+
+# shut down the node (this should archive all WAL files)
+$node->stop;
+
+# restore from the backup
+my $restore = PostgreSQL::Test::Cluster->new('restore');
+$restore->init_from_backup($node, $backup, has_restoring => 1, standby => 0);
+$restore->append_conf('postgresql.conf', "restore_command = ''");
+$restore->append_conf('postgresql.conf', "restore_library = 'basic_archive'");
+$restore->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$restore->start;
+
+# ensure post-backup WAL was replayed
+my $result = $restore->safe_psql("postgres", "SELECT count(*) FROM test;");
+is($result, "1", "check restore content");
+
+done_testing();
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..53e657040b 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -1,34 +1,40 @@
- Archive Modules
+ Archive and Recovery Modules
- Archive Modules
+ Archive and Recovery Modules
PostgreSQL provides infrastructure to create custom modules for continuous
- archiving (see ). While archiving via
- a shell command (i.e., ) is much
- simpler, a custom archive module will often be considerably more robust and
- performant.
+ archiving and recovery (see ). While
+ a shell command (e.g., ,
+ ) is much simpler, a custom module will
+ often be considerably more robust and performant.
When a custom is configured, PostgreSQL
will submit completed WAL files to the module, and the server will avoid
recycling or removing these WAL files until the module indicates that the files
- were successfully archived. It is ultimately up to the module to decide what
- to do with each WAL file, but many recommendations are listed at
- .
+ were successfully archived. When a custom
+ is configured, PostgreSQL will use the
+ module for recovery actions. It is ultimately up to the module to decide how
+ to accomplish each task, but some recommendations are
Re: recovery modules
On Tue, Jan 03, 2023 at 11:05:38AM -0800, Nathan Bossart wrote: > I noticed that cfbot's Windows tests are failing because the backslashes in > the archive directory path are causing escaping problems. Here is an > attempt to fix that by converting all backslashes to forward slashes, which > is what other tests (e.g., 025_stuck_on_old_timeline.pl) do. + GetOldestRestartPoint(, ); + XLByteToSeg(restartRedoPtr, restartSegNo, wal_segment_size); + XLogFileName(lastRestartPointFname, restartTli, restartSegNo, +wal_segment_size); + + shell_archive_cleanup(lastRestartPointFname); Hmm. Is passing down the file name used as a cutoff point the best interface for the modules? Perhaps passing down the redo LSN and its TLI would be a cleaner approach in terms of flexibility? I agree with letting the startup enforce these numbers as that can be easy to mess up for plugin authors, leading to critical problems. The same code pattern is repeated twice for the end-of-recovery callback and the cleanup commands when it comes to building the file name. Not critical, still not really nice. MODULES = basic_archive -PGFILEDESC = "basic_archive - basic archive module" +PGFILEDESC = "basic_archive - basic archive and recovery module" "basic_archive" does not reflect what this module does. Using one library simplifies the whole configuration picture and the tests, so perhaps something like basic_wal_module, or something like that, would be better long-term? -- Michael signature.asc Description: PGP signature
Re: recovery modules
es.
+ */
+static bool
+basic_restore_file(const char *file, const char *path,
+ const char *lastRestartPointFileName)
+{
+ char source[MAXPGPATH];
+ struct stat st;
+
+ ereport(DEBUG1,
+ (errmsg("restoring \"%s\" via basic_archive", file)));
+
+ if (archive_directory == NULL || archive_directory[0] == '\0')
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"basic_archive.archive_directory\" is not set")));
+
+ /*
+ * Check whether the file exists. If not, we return false to indicate that
+ * there are no more files to restore.
+ */
+ snprintf(source, MAXPGPATH, "%s/%s", archive_directory, file);
+ if (stat(source, ) != 0)
+ {
+ int elevel = (errno == ENOENT) ? DEBUG1 : ERROR;
+
+ ereport(elevel,
+(errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", source)));
+ return false;
+ }
+
+ copy_file(source, unconstify(char *, path));
+
+ ereport(DEBUG1,
+ (errmsg("restored \"%s\" via basic_archive", file)));
+ return true;
+}
diff --git a/contrib/basic_archive/meson.build b/contrib/basic_archive/meson.build
index bc1380e6f6..af4580dea9 100644
--- a/contrib/basic_archive/meson.build
+++ b/contrib/basic_archive/meson.build
@@ -7,7 +7,7 @@ basic_archive_sources = files(
if host_system == 'windows'
basic_archive_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
'--NAME', 'basic_archive',
-'--FILEDESC', 'basic_archive - basic archive module',])
+'--FILEDESC', 'basic_archive - basic archive and recovery module',])
endif
basic_archive = shared_module('basic_archive',
@@ -31,4 +31,9 @@ tests += {
# which typical runningcheck users do not have (e.g. buildfarm clients).
'runningcheck': false,
},
+ 'tap': {
+'tests': [
+ 't/001_restore.pl',
+],
+ },
}
diff --git a/contrib/basic_archive/t/001_restore.pl b/contrib/basic_archive/t/001_restore.pl
new file mode 100644
index 00..ec8767d740
--- /dev/null
+++ b/contrib/basic_archive/t/001_restore.pl
@@ -0,0 +1,44 @@
+
+# Copyright (c) 2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# start a node
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(has_archiving => 1, allows_streaming => 1);
+my $archive_dir = $node->archive_dir;
+$archive_dir =~ s!\\!/!g if $PostgreSQL::Test::Utils::windows_os;
+$node->append_conf('postgresql.conf', "archive_command = ''");
+$node->append_conf('postgresql.conf', "archive_library = 'basic_archive'");
+$node->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$node->start;
+
+# backup the node
+my $backup = 'backup';
+$node->backup($backup);
+
+# generate some new WAL files
+$node->safe_psql('postgres', "CREATE TABLE test (a INT);");
+$node->safe_psql('postgres', "SELECT pg_switch_wal();");
+$node->safe_psql('postgres', "INSERT INTO test VALUES (1);");
+
+# shut down the node (this should archive all WAL files)
+$node->stop;
+
+# restore from the backup
+my $restore = PostgreSQL::Test::Cluster->new('restore');
+$restore->init_from_backup($node, $backup, has_restoring => 1, standby => 0);
+$restore->append_conf('postgresql.conf', "restore_command = ''");
+$restore->append_conf('postgresql.conf', "restore_library = 'basic_archive'");
+$restore->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$restore->start;
+
+# ensure post-backup WAL was replayed
+my $result = $restore->safe_psql("postgres", "SELECT count(*) FROM test;");
+is($result, "1", "check restore content");
+
+done_testing();
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..53e657040b 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -1,34 +1,40 @@
- Archive Modules
+ Archive and Recovery Modules
- Archive Modules
+ Archive and Recovery Modules
PostgreSQL provides infrastructure to create custom modules for continuous
- archiving (see ). While archiving via
- a shell command (i.e., ) is much
- simpler, a custom archive module will often be considerably more robust and
- performant.
+ archiving and recovery (see ). While
+ a shell command (e.g., ,
+ ) is much simpler, a custom module will
+ often be considerably more robust and performant.
When a custom is configured, PostgreSQL
will submit completed WAL files to the module, and the server will avoid
recycling or removing these WAL files until the module indicates that the files
- were successfully archived. It is ultimately up to the module to decide what
- to do with each WAL file, but many recommendations
Re: recovery modules
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"basic_archive.archive_directory\" is not set")));
+
+ /*
+ * Check whether the file exists. If not, we return false to indicate that
+ * there are no more files to restore.
+ */
+ snprintf(source, MAXPGPATH, "%s/%s", archive_directory, file);
+ if (stat(source, ) != 0)
+ {
+ int elevel = (errno == ENOENT) ? DEBUG1 : ERROR;
+
+ ereport(elevel,
+(errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", source)));
+ return false;
+ }
+
+ copy_file(source, unconstify(char *, path));
+
+ ereport(DEBUG1,
+ (errmsg("restored \"%s\" via basic_archive", file)));
+ return true;
+}
diff --git a/contrib/basic_archive/meson.build b/contrib/basic_archive/meson.build
index bc1380e6f6..af4580dea9 100644
--- a/contrib/basic_archive/meson.build
+++ b/contrib/basic_archive/meson.build
@@ -7,7 +7,7 @@ basic_archive_sources = files(
if host_system == 'windows'
basic_archive_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
'--NAME', 'basic_archive',
-'--FILEDESC', 'basic_archive - basic archive module',])
+'--FILEDESC', 'basic_archive - basic archive and recovery module',])
endif
basic_archive = shared_module('basic_archive',
@@ -31,4 +31,9 @@ tests += {
# which typical runningcheck users do not have (e.g. buildfarm clients).
'runningcheck': false,
},
+ 'tap': {
+'tests': [
+ 't/001_restore.pl',
+],
+ },
}
diff --git a/contrib/basic_archive/t/001_restore.pl b/contrib/basic_archive/t/001_restore.pl
new file mode 100644
index 00..86ff0dc7f5
--- /dev/null
+++ b/contrib/basic_archive/t/001_restore.pl
@@ -0,0 +1,42 @@
+
+# Copyright (c) 2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+# start a node
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(has_archiving => 1, allows_streaming => 1);
+my $archive_dir = $node->archive_dir;
+$node->append_conf('postgresql.conf', "archive_command = ''");
+$node->append_conf('postgresql.conf', "archive_library = 'basic_archive'");
+$node->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$node->start;
+
+# backup the node
+my $backup = 'backup';
+$node->backup($backup);
+
+# generate some new WAL files
+$node->safe_psql('postgres', "CREATE TABLE test (a INT);");
+$node->safe_psql('postgres', "SELECT pg_switch_wal();");
+$node->safe_psql('postgres', "INSERT INTO test VALUES (1);");
+
+# shut down the node (this should archive all WAL files)
+$node->stop;
+
+# restore from the backup
+my $restore = PostgreSQL::Test::Cluster->new('restore');
+$restore->init_from_backup($node, $backup, has_restoring => 1, standby => 0);
+$restore->append_conf('postgresql.conf', "restore_command = ''");
+$restore->append_conf('postgresql.conf', "restore_library = 'basic_archive'");
+$restore->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$restore->start;
+
+# ensure post-backup WAL was replayed
+my $result = $restore->safe_psql("postgres", "SELECT count(*) FROM test;");
+is($result, "1", "check restore content");
+
+done_testing();
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..53e657040b 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -1,34 +1,40 @@
- Archive Modules
+ Archive and Recovery Modules
- Archive Modules
+ Archive and Recovery Modules
PostgreSQL provides infrastructure to create custom modules for continuous
- archiving (see ). While archiving via
- a shell command (i.e., ) is much
- simpler, a custom archive module will often be considerably more robust and
- performant.
+ archiving and recovery (see ). While
+ a shell command (e.g., ,
+ ) is much simpler, a custom module will
+ often be considerably more robust and performant.
When a custom is configured, PostgreSQL
will submit completed WAL files to the module, and the server will avoid
recycling or removing these WAL files until the module indicates that the files
- were successfully archived. It is ultimately up to the module to decide what
- to do with each WAL file, but many recommendations are listed at
- .
+ were successfully archived. When a custom
+ is configured, PostgreSQL will use the
+ module for recovery actions. It is ultimately up to the module to decide how
+ to accomplish each task, but some recommendations are listed at
+ and
+ .
- Archiving modules must at least consist of an initialization function (see
- ) and the required callbacks (see
- ). However, archive modules are
- also perm
Re: recovery modules
true;
@@ -368,3 +389,45 @@ compare_files(const char *file1, const char *file2)
return ret;
}
+
+/*
+ * basic_restore_file
+ *
+ * Retrieves one file from the WAL archives.
+ */
+static bool
+basic_restore_file(const char *file, const char *path,
+ const char *lastRestartPointFileName)
+{
+ char source[MAXPGPATH];
+ struct stat st;
+
+ ereport(DEBUG1,
+ (errmsg("restoring \"%s\" via basic_archive", file)));
+
+ if (archive_directory == NULL || archive_directory[0] == '\0')
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"basic_archive.archive_directory\" is not set")));
+
+ /*
+ * Check whether the file exists. If not, we return false to indicate that
+ * there are no more files to restore.
+ */
+ snprintf(source, MAXPGPATH, "%s/%s", archive_directory, file);
+ if (stat(source, ) != 0)
+ {
+ int elevel = (errno == ENOENT) ? DEBUG1 : ERROR;
+
+ ereport(elevel,
+(errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", source)));
+ return false;
+ }
+
+ copy_file(source, unconstify(char *, path));
+
+ ereport(DEBUG1,
+ (errmsg("restored \"%s\" via basic_archive", file)));
+ return true;
+}
diff --git a/contrib/basic_archive/meson.build b/contrib/basic_archive/meson.build
index b25dce99a3..5b5a6d79e7 100644
--- a/contrib/basic_archive/meson.build
+++ b/contrib/basic_archive/meson.build
@@ -7,7 +7,7 @@ basic_archive_sources = files(
if host_system == 'windows'
basic_archive_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
'--NAME', 'basic_archive',
-'--FILEDESC', 'basic_archive - basic archive module',])
+'--FILEDESC', 'basic_archive - basic archive and recovery module',])
endif
basic_archive = shared_module('basic_archive',
@@ -31,4 +31,9 @@ tests += {
# which typical runningcheck users do not have (e.g. buildfarm clients).
'runningcheck': false,
},
+ 'tap': {
+'tests': [
+ 't/001_restore.pl',
+],
+ },
}
diff --git a/contrib/basic_archive/t/001_restore.pl b/contrib/basic_archive/t/001_restore.pl
new file mode 100644
index 00..86ff0dc7f5
--- /dev/null
+++ b/contrib/basic_archive/t/001_restore.pl
@@ -0,0 +1,42 @@
+
+# Copyright (c) 2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+# start a node
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(has_archiving => 1, allows_streaming => 1);
+my $archive_dir = $node->archive_dir;
+$node->append_conf('postgresql.conf', "archive_command = ''");
+$node->append_conf('postgresql.conf', "archive_library = 'basic_archive'");
+$node->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$node->start;
+
+# backup the node
+my $backup = 'backup';
+$node->backup($backup);
+
+# generate some new WAL files
+$node->safe_psql('postgres', "CREATE TABLE test (a INT);");
+$node->safe_psql('postgres', "SELECT pg_switch_wal();");
+$node->safe_psql('postgres', "INSERT INTO test VALUES (1);");
+
+# shut down the node (this should archive all WAL files)
+$node->stop;
+
+# restore from the backup
+my $restore = PostgreSQL::Test::Cluster->new('restore');
+$restore->init_from_backup($node, $backup, has_restoring => 1, standby => 0);
+$restore->append_conf('postgresql.conf', "restore_command = ''");
+$restore->append_conf('postgresql.conf', "restore_library = 'basic_archive'");
+$restore->append_conf('postgresql.conf', "basic_archive.archive_directory = '$archive_dir'");
+$restore->start;
+
+# ensure post-backup WAL was replayed
+my $result = $restore->safe_psql("postgres", "SELECT count(*) FROM test;");
+is($result, "1", "check restore content");
+
+done_testing();
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..53e657040b 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -1,34 +1,40 @@
- Archive Modules
+ Archive and Recovery Modules
- Archive Modules
+ Archive and Recovery Modules
PostgreSQL provides infrastructure to create custom modules for continuous
- archiving (see ). While archiving via
- a shell command (i.e., ) is much
- simpler, a custom archive module will often be considerably more robust and
- performant.
+ archiving and recovery (see ). While
+ a shell command (e.g., ,
+ ) is much simpler, a custom module will
+ often be considerably more robust and performant.
When a custom is configured, PostgreSQL
will submit completed WAL files to the module, and the server will avoid
recycling or removing these WAL files until the module indicates that the files
- were successfully archived. It is ultimately u
Re: recovery modules
Hi, On 2022-12-27 15:04:28 -0800, Nathan Bossart wrote: > I'm sorry, I'm still lost here. Wouldn't restoration via library tend to > improve latency? Is your point that clusters may end up depending on this > improvement so much that a shell command would no longer be able to keep > up? Yes. > I might be creating a straw man, but this seems like less of a concern > for pg_rewind since it isn't meant for continuous, ongoing restoration. pg_rewind is in the critical path of a bunch of HA scenarios, so I wouldn't say that restore performance isn't important... Greetings, Andres Freund
Re: recovery modules
On Tue, Dec 27, 2022 at 02:11:11PM -0800, Andres Freund wrote: > On 2022-12-27 11:24:49 -0800, Nathan Bossart wrote: >> * Unlike archive modules, recovery libraries cannot be changed at runtime. >> There isn't a safe way to unload a library, and archive libraries work >> around this restriction by restarting the archiver process. Since recovery >> libraries are loaded via the startup and checkpointer processes (which >> cannot be trivially restarted like the archiver), the same workaround is >> not feasible. > > I don't think that's a convincing reason to not support configuration > changes. Sure, libraries cannot be unloaded, but an unnecessarily loaded > library is cheap. All that's needed is to redirect the relevant function > calls. Agreed. That seems worth the cost to switching this stuff to be SIGHUP-able. >> * pg_rewind uses restore_command, but there isn't a straightforward path to >> support restore_library. I haven't addressed this in the attached patches, >> but perhaps this is a reason to allow specifying both restore_command and >> restore_library at the same time. pg_rewind would use restore_command, and >> the server would use restore_library. > > That seems problematic, leading to situations where one might not be able to > use restore_command anymore, because it's not feasible to do > segment-by-segment restoration. Do you mean that supporting restore_library in pg_rewind is a hard requirement? I fail to see why this should be the case. Note that having the possibility to do dlopen() calls in the frontend (aka libpq) for loading some callbacks is something I've been looking for in the past. Having consistency in terms of restrictions between library and command like for archives would make sense I guess (FWIW, I mentioned on the thread of d627ce3 that we'd better not put any restrictions for the archives). -- Michael signature.asc Description: PGP signature
Re: recovery modules
On Tue, Dec 27, 2022 at 02:45:30PM -0800, Andres Freund wrote: > On 2022-12-27 14:37:11 -0800, Nathan Bossart wrote: >> On Tue, Dec 27, 2022 at 02:11:11PM -0800, Andres Freund wrote: >> > On 2022-12-27 11:24:49 -0800, Nathan Bossart wrote: >> >> * pg_rewind uses restore_command, but there isn't a straightforward path >> >> to >> >> support restore_library. I haven't addressed this in the attached >> >> patches, >> >> but perhaps this is a reason to allow specifying both restore_command and >> >> restore_library at the same time. pg_rewind would use restore_command, >> >> and >> >> the server would use restore_library. >> > >> > That seems problematic, leading to situations where one might not be able >> > to >> > use restore_command anymore, because it's not feasible to do >> > segment-by-segment restoration. >> >> I'm not following why this would make segment-by-segment restoration >> infeasible. Would you mind elaborating? > > Latency effects for example can make it infeasible to do segment-by-segment > restoration infeasible performance wise. On the most extreme end, imagine WAL > archived to tape or such. I'm sorry, I'm still lost here. Wouldn't restoration via library tend to improve latency? Is your point that clusters may end up depending on this improvement so much that a shell command would no longer be able to keep up? I might be creating a straw man, but this seems like less of a concern for pg_rewind since it isn't meant for continuous, ongoing restoration. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: recovery modules
Hi, On 2022-12-27 14:37:11 -0800, Nathan Bossart wrote: > On Tue, Dec 27, 2022 at 02:11:11PM -0800, Andres Freund wrote: > > On 2022-12-27 11:24:49 -0800, Nathan Bossart wrote: > >> I've attached a patch set that adds the restore_library, > >> archive_cleanup_library, and recovery_end_library parameters to allow > >> archive recovery via loadable modules. This is a follow-up to the > >> archive_library parameter added in v15 [0] [1]. > > > > Why do we need N parameters for this? To me it seems more sensible to have > > one > > parameter that then allows a library to implement all these (potentially > > optionally). > > The main reason is flexibility. Separate parameters allow using a library > for one thing and a command for another, or different libraries for > different things. If that isn't a use-case we wish to support, I don't > mind combining all three into a single recovery_library parameter. I think the configuration complexity is a sufficient concern to not go that direction... > >> * Unlike archive modules, recovery libraries cannot be changed at runtime. > >> There isn't a safe way to unload a library, and archive libraries work > >> around this restriction by restarting the archiver process. Since recovery > >> libraries are loaded via the startup and checkpointer processes (which > >> cannot be trivially restarted like the archiver), the same workaround is > >> not feasible. > > > > I don't think that's a convincing reason to not support configuration > > changes. Sure, libraries cannot be unloaded, but an unnecessarily loaded > > library is cheap. All that's needed is to redirect the relevant function > > calls. > > This might leave some stuff around (e.g., GUCs, background workers), but if > that isn't a concern, I can adjust it to work as you describe. You can still have a shutdown hook re background workers. I don't think the GUCs matter, given that it's the startup/checkpointer processes. > >> * pg_rewind uses restore_command, but there isn't a straightforward path to > >> support restore_library. I haven't addressed this in the attached patches, > >> but perhaps this is a reason to allow specifying both restore_command and > >> restore_library at the same time. pg_rewind would use restore_command, and > >> the server would use restore_library. > > > > That seems problematic, leading to situations where one might not be able to > > use restore_command anymore, because it's not feasible to do > > segment-by-segment restoration. > > I'm not following why this would make segment-by-segment restoration > infeasible. Would you mind elaborating? Latency effects for example can make it infeasible to do segment-by-segment restoration infeasible performance wise. On the most extreme end, imagine WAL archived to tape or such. Greetings, Andres Freund
Re: recovery modules
On Tue, Dec 27, 2022 at 02:11:11PM -0800, Andres Freund wrote: > On 2022-12-27 11:24:49 -0800, Nathan Bossart wrote: >> I've attached a patch set that adds the restore_library, >> archive_cleanup_library, and recovery_end_library parameters to allow >> archive recovery via loadable modules. This is a follow-up to the >> archive_library parameter added in v15 [0] [1]. > > Why do we need N parameters for this? To me it seems more sensible to have one > parameter that then allows a library to implement all these (potentially > optionally). The main reason is flexibility. Separate parameters allow using a library for one thing and a command for another, or different libraries for different things. If that isn't a use-case we wish to support, I don't mind combining all three into a single recovery_library parameter. >> * Unlike archive modules, recovery libraries cannot be changed at runtime. >> There isn't a safe way to unload a library, and archive libraries work >> around this restriction by restarting the archiver process. Since recovery >> libraries are loaded via the startup and checkpointer processes (which >> cannot be trivially restarted like the archiver), the same workaround is >> not feasible. > > I don't think that's a convincing reason to not support configuration > changes. Sure, libraries cannot be unloaded, but an unnecessarily loaded > library is cheap. All that's needed is to redirect the relevant function > calls. This might leave some stuff around (e.g., GUCs, background workers), but if that isn't a concern, I can adjust it to work as you describe. >> * pg_rewind uses restore_command, but there isn't a straightforward path to >> support restore_library. I haven't addressed this in the attached patches, >> but perhaps this is a reason to allow specifying both restore_command and >> restore_library at the same time. pg_rewind would use restore_command, and >> the server would use restore_library. > > That seems problematic, leading to situations where one might not be able to > use restore_command anymore, because it's not feasible to do > segment-by-segment restoration. I'm not following why this would make segment-by-segment restoration infeasible. Would you mind elaborating? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: recovery modules
Hi, On 2022-12-27 11:24:49 -0800, Nathan Bossart wrote: > I've attached a patch set that adds the restore_library, > archive_cleanup_library, and recovery_end_library parameters to allow > archive recovery via loadable modules. This is a follow-up to the > archive_library parameter added in v15 [0] [1]. Why do we need N parameters for this? To me it seems more sensible to have one parameter that then allows a library to implement all these (potentially optionally). > * Unlike archive modules, recovery libraries cannot be changed at runtime. > There isn't a safe way to unload a library, and archive libraries work > around this restriction by restarting the archiver process. Since recovery > libraries are loaded via the startup and checkpointer processes (which > cannot be trivially restarted like the archiver), the same workaround is > not feasible. I don't think that's a convincing reason to not support configuration changes. Sure, libraries cannot be unloaded, but an unnecessarily loaded library is cheap. All that's needed is to redirect the relevant function calls. > * pg_rewind uses restore_command, but there isn't a straightforward path to > support restore_library. I haven't addressed this in the attached patches, > but perhaps this is a reason to allow specifying both restore_command and > restore_library at the same time. pg_rewind would use restore_command, and > the server would use restore_library. That seems problematic, leading to situations where one might not be able to use restore_command anymore, because it's not feasible to do segment-by-segment restoration. Greetings, Andres Freund
recovery modules
I've attached a patch set that adds the restore_library, archive_cleanup_library, and recovery_end_library parameters to allow archive recovery via loadable modules. This is a follow-up to the archive_library parameter added in v15 [0] [1]. The motivation behind this change is similar to that of archive_library (e.g., robustness, performance). The recovery functions are provided via a similar interface to archive modules (i.e., an initialization function that returns the function pointers). Also, I've extended basic_archive to work as a restore_library, which makes it easy to demonstrate both archiving and recovery via a loadable module in a TAP test. A few miscellaneous design notes: * Unlike archive modules, recovery libraries cannot be changed at runtime. There isn't a safe way to unload a library, and archive libraries work around this restriction by restarting the archiver process. Since recovery libraries are loaded via the startup and checkpointer processes (which cannot be trivially restarted like the archiver), the same workaround is not feasible. * pg_rewind uses restore_command, but there isn't a straightforward path to support restore_library. I haven't addressed this in the attached patches, but perhaps this is a reason to allow specifying both restore_command and restore_library at the same time. pg_rewind would use restore_command, and the server would use restore_library. * I've combined the documentation to create one "Archive and Recovery Modules" chapter. They are similar enough that it felt silly to write a separate chapter for recovery modules. However, I've still split them up within the chapter, and they have separate initialization functions. This retains backward compatibility with v15 archive modules, keeps them logically separate, and hopefully hints at the functional differences. Even so, if you want to create one library for both archive and recovery, there is nothing stopping you. * Unlike archive modules, I didn't add any sort of "check" or "shutdown" callbacks. The recovery_end_library parameter makes a "shutdown" callback largely redundant, and I couldn't think of any use-case for a "check" callback. However, new callbacks could be added in the future if needed. * Unlike archive modules, restore_library and recovery_end_library may be loaded in single-user mode. I believe this works out-of-the-box, but it's an extra thing to be cognizant of. * If both the library and command parameter for a recovery action is specified, the server should fail to startup, but if a misconfiguration is detected after SIGHUP, we emit a WARNING and continue using the library. I originally thought about emitting an ERROR like the archiver does in this case, but failing recovery and stopping the server felt a bit too harsh. I'm curious what folks think about this. * Іt could be nice to rewrite pg_archivecleanup for use as an archive_cleanup_library, but I don't think that needs to be a part of this patch set. [0] https://postgr.es/m/668D2428-F73B-475E-87AE-F89D67942270%40amazon.com [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=5ef1eef -- Nathan Bossart Amazon Web Services: https://aws.amazon.com >From b2e826baef398998ba93b27c5d68e89d439b4962 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Fri, 23 Dec 2022 16:35:25 -0800 Subject: [PATCH v1 1/3] Move the code to restore files via the shell to a separate file. This is preparatory work for allowing more extensibility in this area. --- src/backend/access/transam/Makefile| 1 + src/backend/access/transam/meson.build | 1 + src/backend/access/transam/shell_restore.c | 194 + src/backend/access/transam/xlog.c | 44 - src/backend/access/transam/xlogarchive.c | 158 + src/include/access/xlogarchive.h | 7 +- 6 files changed, 240 insertions(+), 165 deletions(-) create mode 100644 src/backend/access/transam/shell_restore.c diff --git a/src/backend/access/transam/Makefile b/src/backend/access/transam/Makefile index 661c55a9db..099c315d03 100644 --- a/src/backend/access/transam/Makefile +++ b/src/backend/access/transam/Makefile @@ -19,6 +19,7 @@ OBJS = \ multixact.o \ parallel.o \ rmgr.o \ + shell_restore.o \ slru.o \ subtrans.o \ timeline.o \ diff --git a/src/backend/access/transam/meson.build b/src/backend/access/transam/meson.build index 65c77531be..a0870217b8 100644 --- a/src/backend/access/transam/meson.build +++ b/src/backend/access/transam/meson.build @@ -7,6 +7,7 @@ backend_sources += files( 'multixact.c', 'parallel.c', 'rmgr.c', + 'shell_restore.c', 'slru.c', 'subtrans.c', 'timeline.c', diff --git a/src/backend/access/transam/shell_restore.c b/src/backend/access/transam/shell_restore.c new file mode 100644 index 00..3ddcabd969 --- /dev/null +++ b/src/
