MIME-Version: 1.0
References: <202601301728.sfkizrto3t5i@alvherre.pgsql> <9b9341b0-942e-4d34-b94f-92bd918fad04@ya.ru>
 <1317421770387925@cea5cfd9-50d3-4d85-a924-a7cc75f8f215> <CAHGQGwF3tFP=oQgYHoS3h9TMeFbZwXc6+xdPTW8CkMV2TkjTmg@mail.gmail.com>
 <aYpYaRipZ_-lYxNY@nathan> <CAHGQGwFzC+HYEwJGhpmu1NBiQp_Ln8t0z9CDt8QCQDnUtFAFYA@mail.gmail.com>
 <aYtXgaKfiYpaqQyW@nathan>
In-Reply-To: <aYtXgaKfiYpaqQyW@nathan>
From: Fujii Masao <masao.fujii@gmail.com>
Date: Tue, 24 Feb 2026 13:28:50 +0900
Message-ID: <CAHGQGwFq-7p1nCfemJCe0_VXVWxpGdq2D4ss+ns6p7=W1FTDqg@mail.gmail.com>
Subject: Re: basic_archive lost archive_directory
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Sergei Kornilov <sk@zsrv.org>, =?UTF-8?B?0J7Qu9C10LMg0KHQsNC80L7QudC70L7Qsg==?= <splarv@ya.ru>, 
	pgsql-bugs@lists.postgresql.org, =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAHGQGwFq-7p1nCfemJCe0_VXVWxpGdq2D4ss%2Bns6p7%3DW1FTDqg%40mail.gmail.com>
Precedence: bulk

On Wed, Feb 11, 2026 at 1:06=E2=80=AFAM Nathan Bossart <nathandbossart@gmai=
l.com> 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 configu=
ration
> > 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 th=
at
> 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 messa=
ge
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 archivin=
g.
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 doe=
s
not exist.

That said, if users are generally fine with the "could not create file" err=
or,
I'm ok with the proposed patch (i.e., just removing the check).

Regards,

--=20
Fujii Masao