public inbox for [email protected]help / color / mirror / Atom feed
Re: basic_archive lost archive_directory 7+ messages / 4 participants [nested] [flat]
* Re: basic_archive lost archive_directory @ 2026-02-06 14:25 Sergei Kornilov <[email protected]> 0 siblings, 2 replies; 7+ messages in thread From: Sergei Kornilov @ 2026-02-06 14:25 UTC (permalink / raw) To: Олег Самойлов <[email protected]>; +Cc: [email protected]; Álvaro Herrera <[email protected]> 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 Attachments: [text/x-diff] basic_archive_move_is_dir.patch (1.7K, 2-basic_archive_move_is_dir.patch) download | inline diff: 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; } /* ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: basic_archive lost archive_directory @ 2026-02-07 07:57 Олег Самойлов <[email protected]> parent: Sergei Kornilov <[email protected]> 1 sibling, 0 replies; 7+ messages in thread From: Олег Самойлов @ 2026-02-07 07:57 UTC (permalink / raw) To: Sergei Kornilov <[email protected]>; +Cc: [email protected]; Álvaro Herrera <[email protected]> 06.02.2026 17:25, Sergei Kornilov пишет > 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 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/ ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: basic_archive lost archive_directory @ 2026-02-09 17:46 Fujii Masao <[email protected]> parent: Sergei Kornilov <[email protected]> 1 sibling, 3 replies; 7+ messages in thread From: Fujii Masao @ 2026-02-09 17:46 UTC (permalink / raw) To: Sergei Kornilov <[email protected]>; +Cc: Олег Самойлов <[email protected]>; [email protected]; Álvaro Herrera <[email protected]> On Fri, Feb 6, 2026 at 11:25 PM Sergei Kornilov <[email protected]> wrote: > > 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 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: basic_archive lost archive_directory @ 2026-02-09 20:09 Sergei Kornilov <[email protected]> parent: Fujii Masao <[email protected]> 2 siblings, 0 replies; 7+ messages in thread From: Sergei Kornilov @ 2026-02-09 20:09 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Олег Самойлов <[email protected]>; [email protected]; Álvaro Herrera <[email protected]> Hello! > 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. 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 Attachments: [text/x-diff] 001-bug_basic_archive_lost_archive_directory.patch (2.3K, 2-001-bug_basic_archive_lost_archive_directory.patch) download | inline diff: 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; } - /* - * 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)) + if (stat(archive_directory, &st) != 0 || !S_ISDIR(st.st_mode)) { - GUC_check_errdetail("Specified archive directory does not exist."); + arch_module_check_errdetail("Specified archive directory does not exist."); 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 * [text/x-diff] 002-basic_archive_remove_check_archive_directory.patch (1.2K, 3-002-basic_archive_remove_check_archive_directory.patch) download | inline diff: 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 * ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: basic_archive lost archive_directory @ 2026-02-09 21:48 Nathan Bossart <[email protected]> parent: Fujii Masao <[email protected]> 2 siblings, 0 replies; 7+ messages in thread From: Nathan Bossart @ 2026-02-09 21:48 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 02:46:39AM +0900, Fujii Masao wrote: > 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. 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: basic_archive lost archive_directory @ 2026-02-09 21:58 Nathan Bossart <[email protected]> parent: Fujii Masao <[email protected]> 2 siblings, 1 reply; 7+ messages in thread From: Nathan Bossart @ 2026-02-09 21:58 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 02:46:39AM +0900, Fujii Masao wrote: > 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. 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: basic_archive lost archive_directory @ 2026-02-10 01:23 Fujii Masao <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Fujii Masao @ 2026-02-10 01:23 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Sergei Kornilov <[email protected]>; Олег Самойлов <[email protected]>; [email protected]; Álvaro Herrera <[email protected]> On Tue, Feb 10, 2026 at 6:58 AM Nathan Bossart <[email protected]> wrote: > > On Tue, Feb 10, 2026 at 02:46:39AM +0900, Fujii Masao wrote: > > 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. > > 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. 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-02-10 01:23 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-02-06 14:25 Re: basic_archive lost archive_directory Sergei Kornilov <[email protected]> 2026-02-07 07:57 ` Олег Самойлов <[email protected]> 2026-02-09 17:46 ` Fujii Masao <[email protected]> 2026-02-09 20:09 ` Sergei Kornilov <[email protected]> 2026-02-09 21:48 ` Nathan Bossart <[email protected]> 2026-02-09 21:58 ` Nathan Bossart <[email protected]> 2026-02-10 01:23 ` Fujii Masao <[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