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
