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
