Subject: Re: basic_archive lost archive_directory

From: Sergei Kornilov <[email-masked]>
Date: Fri, 06 Feb 2026 17:25:25 +0300

Hello

How to reproduce:

1) configure

archive_mode = on
archive_library = 'basic_archive'
basic_archive.archive_directory = '/some/path/'

2) start postgres and verify archive works
3) make this directory temporary inaccessible. NFS will give you many ways to achieve this, here just mv /some/ /some_moved/  is enough.
4) basic_archive will complain ERROR:  could not create file ... No such file or directory for new WAL archive attempts
5) restart archiver process with any reason: kill it or restart postgres
6) make archive_directory accessible again: archiver process will not check the directory's existence again and continue to complain about unconfigured archive_directory

Maybe it makes sense to move the directory existence check from check_archive_directory (guc check callback) to basic_archive_configured? (attached)

regards, Sergei

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 6c7f985d48b..d46aab39806 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -95,8 +95,6 @@ _PG_archive_module_init(void)
 static bool
 check_archive_directory(char **newval, void **extra, GucSource source)
 {
-	struct stat st;
-
 	/*
 	 * The default value is an empty string, so we have to accept that value.
 	 * Our check_configured callback also checks for this and prevents
@@ -115,17 +113,6 @@ check_archive_directory(char **newval, void **extra, GucSource source)
 		return false;
 	}
 
-	/*
-	 * Do a basic sanity check that the specified archive directory exists. It
-	 * could be removed at some point in the future, so we still need to be
-	 * prepared for it not to exist in the actual archiving logic.
-	 */
-	if (stat(*newval, &st) != 0 || !S_ISDIR(st.st_mode))
-	{
-		GUC_check_errdetail("Specified archive directory does not exist.");
-		return false;
-	}
-
 	return true;
 }
 
@@ -137,12 +124,23 @@ check_archive_directory(char **newval, void **extra, GucSource source)
 static bool
 basic_archive_configured(ArchiveModuleState *state)
 {
-	if (archive_directory != NULL && archive_directory[0] != '\0')
-		return true;
+	struct stat st;
 
-	arch_module_check_errdetail("%s is not set.",
+	if (archive_directory == NULL || archive_directory[0] == '\0')
+	{
+		arch_module_check_errdetail("%s is not set.",
 								"basic_archive.archive_directory");
-	return false;
+
+		return false;
+	}
+
+	if (stat(archive_directory, &st) != 0 || !S_ISDIR(st.st_mode))
+	{
+		arch_module_check_errdetail("Specified archive directory does not exist.");
+		return false;
+	}
+
+	return true;
 }
 
 /*

---

From: Олег Самойлов <[email-masked]>
Date: Sat, 7 Feb 2026 10:57:05 +0300

06.02.2026 17:25, Sergei Kornilov пишет
> Maybe it makes sense to move the directory existence check from check_archive_directory (guc check callback) to basic_archive_configured? (attached)

You described, may be, different bug or depended. I'll try again to explain.

First error was:

"invalid value for parameter
""basic_archive.archive_directory"": ""/mnt/ocean/postgres/stars/
WAL""","Specified archive directory does not exist."

And this is, may be, correct. But second, just after the first:
"""archive_mode"" enabled, yet archiving is not
configured","basic_archive.archive_directory is not
set."

The value of basic_archive.archive_directory
was erased after the first error. But it was erased only inside archive_library 'basic_archive', postgresql itself it was still
basic_archive.archive_directory=/mnt/ocean/postgres/stars/

---

From: Fujii Masao <[email-masked]>
Date: Tue, 10 Feb 2026 02:46:39 +0900

> 6) make archive_directory accessible again: archiver process will not check the directory's existence again and continue to complain about unconfigured archive_directory

Yes, this issue can last until, for example, the configuration is reloaded and
archive_directory is set again. I agree this needs to be addressed.

> Maybe it makes sense to move the directory existence check from check_archive_directory (guc check callback) to basic_archive_configured? (attached)

Basically I like the idea of moving the checks for archive_directory from
check_archive_directory() to basic_archive_configured(). This would not only
address this issue, but also other problems caused by performing these checks
in the GUC check hook.

basic_archive is usually loaded only by the archiver via archive_library.
In that case, errors reported by check_archive_directory() are not logged
by default, since GUC check hook errors are normally emitted only by
the postmaster. As a result, misconfigurations (e.g., a non-existent
archive_directory) may go unnoticed, which is problematic for users.

Moving the checks currently done in check_archive_directory() to
basic_archive_configured() would resolve this, which is one reason
I like your proposal.

In your patch, only the existence check is moved to basic_archive_configured().
Would it also make sense to move the filename length check there? If so,
we could potentially remove the check_archive_directory GUC check hook entirely.

Regards,

-- 
Fujii Masao

---

From: Sergei Kornilov <[email-masked]>
Date: Mon, 09 Feb 2026 23:09:41 +0300

Hello!

> we could potentially remove the check_archive_directory GUC check hook entirely.

Indeed.

I'm not sure, do I need to keep the check_archive_directory declaration for backport branches? I will split the patch into two parts.

regards, Sergei

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 6c7f985d48b..273fd54dc6d 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -94,57 +94,47 @@ _PG_archive_module_init(void)
  */
 static bool
 check_archive_directory(char **newval, void **extra, GucSource source)
+{
+	return true;
+}
+
+/*
+ * basic_archive_configured
+ *
+ * Checks that archive_directory is not blank and exists.
+ */
+static bool
+basic_archive_configured(ArchiveModuleState *state)
 {
 	struct stat st;
 
-	/*
-	 * The default value is an empty string, so we have to accept that value.
-	 * Our check_configured callback also checks for this and prevents
-	 * archiving from proceeding if it is still empty.
-	 */
-	if (*newval == NULL || *newval[0] == '\0')
-		return true;
+	if (archive_directory == NULL || archive_directory[0] == '\0')
+	{
+		arch_module_check_errdetail("%s is not set.",
+								"basic_archive.archive_directory");
+
+		return false;
+	}
 
 	/*
 	 * Make sure the file paths won't be too long.  The docs indicate that the
 	 * file names to be archived can be up to 64 characters long.
 	 */
-	if (strlen(*newval) + 64 + 2 >= MAXPGPATH)
+	if (strlen(archive_directory) + 64 + 2 >= MAXPGPATH)
 	{
-		GUC_check_errdetail("Archive directory too long.");
+		arch_module_check_errdetail("Archive directory too long.");
 		return false;
 	}
 
-	/*
 
 	return true;
 }
 
-/*
- * basic_archive_configured
- *
- * Checks that archive_directory is not blank.
- */
-static bool
-basic_archive_configured(ArchiveModuleState *state)
-{
-	if (archive_directory != NULL && archive_directory[0] != '\0')
-		return true;
-
-	arch_module_check_errdetail("%s is not set.",
-								"basic_archive.archive_directory");
-	return false;
-}
-
 /*
  * basic_archive_file
  *
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 273fd54dc6d..b57fd4c89dc 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -46,7 +46,6 @@ static char *archive_directory = NULL;
 
 static bool basic_archive_configured(ArchiveModuleState *state);
 static bool basic_archive_file(ArchiveModuleState *state, 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 = {
@@ -71,7 +70,7 @@ _PG_init(void)
 							   "",
 							   PGC_SIGHUP,
 							   0,
-							   check_archive_directory, NULL, NULL);
+							   NULL, NULL, NULL);
 
 	MarkGUCPrefixReserved("basic_archive");
 }
@@ -87,17 +86,6 @@ _PG_archive_module_init(void)
 	return &basic_archive_callbacks;
 }
 
-/*
- * check_archive_directory
- *
- * Checks that the provided archive directory exists.
- */
-static bool
-check_archive_directory(char **newval, void **extra, GucSource source)
-{
-	return true;
-}
-
 /*
  * basic_archive_configured
  *

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 9 Feb 2026 15:48:05 -0600

> address this issue, but also other problems caused by performing these checks

Note that the check_configured_cb is called for every segment to archive.
That means we'd be calling stat() much more, which seems like unnecessary
overhead to me.  And we still need to be prepared for the archive directory
to disappear at any time.  I'm wondering if it would be better to simply
remove this archive directory existence check from the check_configured_cb.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 9 Feb 2026 15:58:01 -0600

> archive_directory) may go unnoticed, which is problematic for users.

I don't think this is true.  With default parameters, I see the following
in my logs with a misconfigured archive directory setting:

2026-02-09 15:53:10.372 CST [12803] WARNING:  invalid value for parameter "basic_archive.archive_directory": "/does/not/exist"
2026-02-09 15:53:10.372 CST [12803] DETAIL:  Specified archive directory does not exist.

-- 
nathan

---

From: Fujii Masao <[email-masked]>
Date: Tue, 10 Feb 2026 10:23:02 +0900

> 2026-02-09 15:53:10.372 CST [12803] DETAIL:  Specified archive directory does not exist.

You're right if an invalid value for basic_archive.archive_directory is detected
at server startup. However, when the setting is changed and the configuration
is reloaded, the default behavior does not emit an error log.
Please see the steps below.

-----------------------------------
initdb -D data
mkdir arch
cat <<EOF >> data/postgresql.conf
archive_mode = on
archive_library = 'basic_archive'
basic_archive.archive_directory = '../arch'
EOF
pg_ctl -D data start
echo "basic_archive.archive_directory = 'not_exists'" >> data/postgresql.conf
pg_ctl -D data reload
-----------------------------------

With these steps, the only log messages I see are:

-----------------------------------
LOG:  received SIGHUP, reloading configuration files
LOG:  parameter "basic_archive.archive_directory" changed to "not_exists"
-----------------------------------

BTW, if basic_archive is specified in shared_preload_libraries, the same steps
produce:

-----------------------------------
LOG:  invalid value for parameter "basic_archive.archive_directory":
"not_exists"
DETAIL:  Specified archive directory does not exist.
LOG:  configuration file "/hoge/data/postgresql.conf" contains errors;
unaffected changes were applied
-----------------------------------

Similarly, lowering the archiver log level to DEBUG3 (for example,
via log_min_messages = 'warning,archiver:debug3') also results in:

-----------------------------------
DEBUG:  invalid value for parameter "basic_archive.archive_directory":
"not_exists"
DETAIL:  Specified archive directory does not exist.
DEBUG:  configuration file
"/System/Volumes/Data/dav/head-pgsql/data/postgresql.conf" contains
errors; unaffected changes were applied
-----------------------------------

This illustrates that, with default settings, the error can go unnoticed
on reload.

Regards,

-- 
Fujii Masao