Re: define PG_REPLSLOT_DIR

Hi,

On Tue, Sep 03, 2024 at 09:15:50AM +0900, Michael Paquier wrote:
> On Fri, Aug 30, 2024 at 12:21:29PM +0000, Bertrand Drouvot wrote:
> > That said, I don't have a strong opinion on this one, I think that also makes
> > sense to leave it as it is. Please find attached v4 doing so.
>
> The changes in astreamer_file.c are actually wrong regarding the fact
> that should_allow_existing_directory() needs to be able to work with
> the branch where this code is located as well as back-branches,
> because pg_basebackup from version N supports ~(N-1) versions down to
> a certain version, so changing it is not right. This is why pg_xlog
> and pg_wal are both listed there.

I understand why pg_xlog and pg_wal both need to be listed here, but I don't
get why the proposed changes were "wrong". Or, are you saying that if for any
reason PG_TBLSPC_DIR needs to be changed that would not work anymore? If
that's the case, then I guess we'd have to add a new define and test like:

strcmp(filename, PG_TBLSPC_DIR) == 0) || strcmp(filename, NEW_PG_TBLSPC_DIR) == 0)

, no?

The question is more out of curiosity, not saying the changes should be applied
in astreamer_file.c though.

> Perhaps we should to more for the two entries in basebackup.c with the
> relative paths, but I'm not sure that's worth bothering, either.

I don't have a strong opinon on those ones.

> At
> the end, I got no objections about the remaining pieces, so applied.

Thanks!

> How do people feel about the suggestions to update the comments at the
> end? With the comment in relpath.h suggesting to not change that, the
> current state of HEAD is fine by me.

Yeah, I think that's fine and specially because there is still some places (
outside of the comments) that don't rely on the define(s).

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com