public inbox for [email protected]
help / color / mirror / Atom feedRe: basic_archive lost archive_directory
8+ messages / 3 participants
[nested] [flat]
* Re: basic_archive lost archive_directory
@ 2026-02-10 16:06 Nathan Bossart <[email protected]>
0 siblings, 2 replies; 8+ messages in thread
From: Nathan Bossart @ 2026-02-10 16:06 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Sergei Kornilov <[email protected]>; Олег Самойлов <[email protected]>; [email protected]; Álvaro Herrera <[email protected]>
On Tue, Feb 10, 2026 at 10:23:02AM +0900, Fujii Masao wrote:
> 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.
Oh, I see. Even so, I don't think this problem is best fixed by moving
code from the GUC check hook to the archive module check callback. For
starters, since the archive module check callback is called for each file
to archive, this adds unnecessary overhead to repeatedly verify things that
really shouldn't change as long as the GUC's value stays the same. In
practice, the check callback is mostly meant to ensure the user hasn't
temporarily halted archiving, as per the following note in basic_archive's
documentation:
The default is an empty string, which effectively halts WAL archiving,
but if archive_mode is enabled, the server will accumulate WAL segment
files in the expectation that a value will soon be provided.
Furthermore, fixing this problem in basic_archive doesn't fix it for other
archive modules that use GUC check hooks. I think it should be fixed more
generally, perhaps by bumping up the log level in the archiver when the
GUC's prefix matches the archive_library.
I also want to push back a bit on the idea that basic_archive not working
when the directory is missing at startup is a bug. The documentation for
basic_archive clearly states that the directory must already exist. That
being said, I have no objection to making basic_archive more lenient or
even to back-patching such a change. As I mentioned upthread, IMHO we
should simply remove the existence check from the GUC check hook.
basic_archive must already be written to handle the archive directory
disappearing at any moment, so we should be able to rely on it without the
extra stat().
--
nathan
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: basic_archive lost archive_directory
@ 2026-02-11 21:14 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
1 sibling, 1 reply; 8+ messages in thread
From: Nathan Bossart @ 2026-02-11 21:14 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Sergei Kornilov <[email protected]>; Олег Самойлов <[email protected]>; [email protected]; Álvaro Herrera <[email protected]>
On Tue, Feb 10, 2026 at 10:06:25AM -0600, Nathan Bossart wrote:
> As I mentioned upthread, IMHO we should simply remove the existence check
> from the GUC check hook. basic_archive must already be written to handle
> the archive directory disappearing at any moment, so we should be able to
> rely on it without the extra stat().
Concretely, like the attached.
--
nathan
From e89acb5ad9c37d79374f9b1084b594c9c8e47a0a Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 11 Feb 2026 15:10:27 -0600
Subject: [PATCH v1 1/1] basic_archive: Allow archive directory to be missing
at startup.
---
contrib/basic_archive/basic_archive.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 6c7f985d48b..8c7e9be624f 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -90,13 +90,11 @@ _PG_archive_module_init(void)
/*
* check_archive_directory
*
- * Checks that the provided archive directory exists.
+ * Checks that the provided archive directory path isn't too long.
*/
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;
}
--
2.50.1 (Apple Git-155)
Attachments:
[text/plain] v1-0001-basic_archive-Allow-archive-directory-to-be-missi.patch (1.6K, 2-v1-0001-basic_archive-Allow-archive-directory-to-be-missi.patch)
download | inline diff:
From e89acb5ad9c37d79374f9b1084b594c9c8e47a0a Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 11 Feb 2026 15:10:27 -0600
Subject: [PATCH v1 1/1] basic_archive: Allow archive directory to be missing
at startup.
---
contrib/basic_archive/basic_archive.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 6c7f985d48b..8c7e9be624f 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -90,13 +90,11 @@ _PG_archive_module_init(void)
/*
* check_archive_directory
*
- * Checks that the provided archive directory exists.
+ * Checks that the provided archive directory path isn't too long.
*/
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;
}
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: basic_archive lost archive_directory
@ 2026-02-23 21:38 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Nathan Bossart @ 2026-02-23 21:38 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Sergei Kornilov <[email protected]>; Олег Самойлов <[email protected]>; [email protected]; Álvaro Herrera <[email protected]>
On Wed, Feb 11, 2026 at 03:14:15PM -0600, Nathan Bossart wrote:
> On Tue, Feb 10, 2026 at 10:06:25AM -0600, Nathan Bossart wrote:
>> As I mentioned upthread, IMHO we should simply remove the existence check
>> from the GUC check hook. basic_archive must already be written to handle
>> the archive directory disappearing at any moment, so we should be able to
>> rely on it without the extra stat().
>
> Concretely, like the attached.
Any thoughts? I'll wait another week or so, but then will proceed with
committing/back-patching this.
--
nathan
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: basic_archive lost archive_directory
@ 2026-02-24 04:28 Fujii Masao <[email protected]>
parent: Nathan Bossart <[email protected]>
1 sibling, 2 replies; 8+ messages in thread
From: Fujii Masao @ 2026-02-24 04:28 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Sergei Kornilov <[email protected]>; Олег Самойлов <[email protected]>; [email protected]; Álvaro Herrera <[email protected]>
On Wed, Feb 11, 2026 at 1:06 AM Nathan Bossart <[email protected]> wrote:
>
> On Tue, Feb 10, 2026 at 10:23:02AM +0900, Fujii Masao wrote:
> > 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.
>
> Oh, I see. Even so, I don't think this problem is best fixed by moving
> code from the GUC check hook to the archive module check callback. For
> starters, since the archive module check callback is called for each file
> to archive, this adds unnecessary overhead to repeatedly verify things that
> really shouldn't change as long as the GUC's value stays the same.
Yes, so if we had an archive module callback invoked just after
ProcessConfigFile(), we could use it to check the archive directory only
when needed. However, introducing a new callback isn't suitable for
back branches.
> I also want to push back a bit on the idea that basic_archive not working
> when the directory is missing at startup is a bug. The documentation for
> basic_archive clearly states that the directory must already exist. That
> being said, I have no objection to making basic_archive more lenient or
> even to back-patching such a change. As I mentioned upthread, IMHO we
> should simply remove the existence check from the GUC check hook.
I agree with removing the check from the GUC check hook. My only concern is
that simply removing it (rather than relocating it) changes the error message
users see when archive_directory is misconfigured, which may make
troubleshooting WAL archiving failures slightly harder.
[Current]
WARNING: invalid value for parameter
"basic_archive.archive_directory": "not_exists"
DETAIL: Specified archive directory does not exist.
[With the patch]
ERROR: could not create file
"not_exists/archtemp.00000001000000000000000E.80107.1771905339058": No
such file or directory
One option would be to perform the check in check_configured_cb(),
but as you noted, that would add an extra stat() call for each WAL archiving.
If that overhead is unacceptable, another approach would be to wrap
copy_file() in basic_archive_file() with PG_TRY() / PG_CATCH(). On error,
we could stat() the archive directory and report a clearer reason if it does
not exist.
That said, if users are generally fine with the "could not create file" error,
I'm ok with the proposed patch (i.e., just removing the check).
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: basic_archive lost archive_directory
@ 2026-02-24 07:33 Sergei Kornilov <[email protected]>
parent: Fujii Masao <[email protected]>
1 sibling, 0 replies; 8+ messages in thread
From: Sergei Kornilov @ 2026-02-24 07:33 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Олег Самойлов <[email protected]>; [email protected]; Álvaro Herrera <[email protected]>; Nathan Bossart <[email protected]>
Hello
Honestly, I don't think that only 8 stat calls per each 1Gbps of WAL writes is too much. I doubt basic_archive will be used in systems where there's a lot of writing. Although there is little benefit from this stat call, I agree.
> That said, if users are generally fine with the "could not create file" error,
> I'm ok with the proposed patch (i.e., just removing the check).
I'm ok with this error message.
regards, Sergei
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: basic_archive lost archive_directory
@ 2026-02-24 17:34 Nathan Bossart <[email protected]>
parent: Fujii Masao <[email protected]>
1 sibling, 1 reply; 8+ messages in thread
From: Nathan Bossart @ 2026-02-24 17:34 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Sergei Kornilov <[email protected]>; Олег Самойлов <[email protected]>; [email protected]; Álvaro Herrera <[email protected]>
On Tue, Feb 24, 2026 at 01:28:50PM +0900, Fujii Masao wrote:
> I agree with removing the check from the GUC check hook. My only concern is
> that simply removing it (rather than relocating it) changes the error message
> users see when archive_directory is misconfigured, which may make
> troubleshooting WAL archiving failures slightly harder.
>
> [Current]
> WARNING: invalid value for parameter
> "basic_archive.archive_directory": "not_exists"
> DETAIL: Specified archive directory does not exist.
>
> [With the patch]
> ERROR: could not create file
> "not_exists/archtemp.00000001000000000000000E.80107.1771905339058": No
> such file or directory
>
> One option would be to perform the check in check_configured_cb(),
> but as you noted, that would add an extra stat() call for each WAL archiving.
> If that overhead is unacceptable, another approach would be to wrap
> copy_file() in basic_archive_file() with PG_TRY() / PG_CATCH(). On error,
> we could stat() the archive directory and report a clearer reason if it does
> not exist.
>
> That said, if users are generally fine with the "could not create file" error,
> I'm ok with the proposed patch (i.e., just removing the check).
I think it's fine as-is. If something is wrong with the error message,
IMHO we should fix the error message. Adding extra stat() calls to try to
give a nicer message might work most of the time, but there are still race
conditions where users will see the original one. But in any case, I
believe the current message style is used in many places, so I don't see a
strong reason to do anything different here.
--
nathan
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: basic_archive lost archive_directory
@ 2026-02-26 23:18 Fujii Masao <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Fujii Masao @ 2026-02-26 23:18 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Sergei Kornilov <[email protected]>; Олег Самойлов <[email protected]>; [email protected]; Álvaro Herrera <[email protected]>
On Wed, Feb 25, 2026 at 2:34 AM Nathan Bossart <[email protected]> wrote:
>
> On Tue, Feb 24, 2026 at 01:28:50PM +0900, Fujii Masao wrote:
> > I agree with removing the check from the GUC check hook. My only concern is
> > that simply removing it (rather than relocating it) changes the error message
> > users see when archive_directory is misconfigured, which may make
> > troubleshooting WAL archiving failures slightly harder.
> >
> > [Current]
> > WARNING: invalid value for parameter
> > "basic_archive.archive_directory": "not_exists"
> > DETAIL: Specified archive directory does not exist.
> >
> > [With the patch]
> > ERROR: could not create file
> > "not_exists/archtemp.00000001000000000000000E.80107.1771905339058": No
> > such file or directory
> >
> > One option would be to perform the check in check_configured_cb(),
> > but as you noted, that would add an extra stat() call for each WAL archiving.
> > If that overhead is unacceptable, another approach would be to wrap
> > copy_file() in basic_archive_file() with PG_TRY() / PG_CATCH(). On error,
> > we could stat() the archive directory and report a clearer reason if it does
> > not exist.
> >
> > That said, if users are generally fine with the "could not create file" error,
> > I'm ok with the proposed patch (i.e., just removing the check).
>
> I think it's fine as-is. If something is wrong with the error message,
> IMHO we should fix the error message. Adding extra stat() calls to try to
> give a nicer message might work most of the time, but there are still race
> conditions where users will see the original one. But in any case, I
> believe the current message style is used in many places, so I don't see a
> strong reason to do anything different here.
Ok, so since I seem to be the only one with some reservations about
the message, I'm fine with proceeding with the proposed change.
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: basic_archive lost archive_directory
@ 2026-03-02 19:15 Nathan Bossart <[email protected]>
parent: Fujii Masao <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Nathan Bossart @ 2026-03-02 19:15 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Sergei Kornilov <[email protected]>; Олег Самойлов <[email protected]>; [email protected]; Álvaro Herrera <[email protected]>
Committed.
--
nathan
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-03-02 19:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-02-10 16:06 Re: basic_archive lost archive_directory Nathan Bossart <[email protected]>
2026-02-11 21:14 ` Nathan Bossart <[email protected]>
2026-02-23 21:38 ` Nathan Bossart <[email protected]>
2026-02-24 04:28 ` Fujii Masao <[email protected]>
2026-02-24 07:33 ` Sergei Kornilov <[email protected]>
2026-02-24 17:34 ` Nathan Bossart <[email protected]>
2026-02-26 23:18 ` Fujii Masao <[email protected]>
2026-03-02 19:15 ` Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox