Thread: define PG_REPLSLOT_DIR
Hi hackers, while working on a replication slot tool (idea is to put it in contrib, not shared yet), I realized that "pg_replslot" is being used > 25 times in .c files. I think it would make sense to replace those occurrences with a $SUBJECT, attached a patch doing so. There is 2 places where it is not done: src/bin/initdb/initdb.c src/bin/pg_rewind/filemap.c for consistency with the existing PG_STAT_TMP_DIR define. Out of curiosity, checking the sizes of affected files (O2, no debug): with patch: text data bss dec hex filename 20315 224 17 20556 504c src/backend/backup/basebackup.o 30304 0 8 30312 7668 src/backend/replication/logical/reorderbuffer.o 23812 36 40 23888 5d50 src/backend/replication/slot.o 6367 0 0 6367 18df src/backend/utils/adt/genfile.o 40997 2574 2528 46099 b413 src/bin/initdb/initdb.o 6963 224 8 7195 1c1b src/bin/pg_rewind/filemap.o without patch: text data bss dec hex filename 20315 224 17 20556 504c src/backend/backup/basebackup.o 30286 0 8 30294 7656 src/backend/replication/logical/reorderbuffer.o 23766 36 40 23842 5d22 src/backend/replication/slot.o 6363 0 0 6363 18db src/backend/utils/adt/genfile.o 40997 2574 2528 46099 b413 src/bin/initdb/initdb.o 6963 224 8 7195 1c1b src/bin/pg_rewind/filemap.o Also, I think we could do the same for: pg_notify pg_serial pg_subtrans pg_wal pg_multixact pg_tblspc pg_logical And I volunteer to do so if you think that makes sense. Looking forward to your feedback, Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On 2024-Aug-14, Bertrand Drouvot wrote:
> Out of curiosity, checking the sizes of affected files (O2, no debug):
>
> with patch:
>
> text data bss dec hex filename
> 30304 0 8 30312 7668 src/backend/replication/logical/reorderbuffer.o
> without patch:
> 30286 0 8 30294 7656 src/backend/replication/logical/reorderbuffer.o
Hmm, I suppose this delta can be explained because because the literal
string is used inside larger snprintf() format strings or similar, so
things like
snprintf(path, sizeof(path),
- "pg_replslot/%s/%s", slotname,
+ "%s/%s/%s", PG_REPLSLOT_DIR, slotname,
spill_de->d_name);
and
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not remove file \"%s\" during removal of pg_replslot/%s/xid*: %m",
- path, slotname)));
+ errmsg("could not remove file \"%s\" during removal of %s/%s/xid*: %m",
+ PG_REPLSLOT_DIR, path, slotname)));
I don't disagree with making this change, but changing some of the other
directory names you suggest, such as
> pg_notify
> pg_serial
> pg_subtrans
> pg_multixact
> pg_wal
would probably make no difference, since there are no literal strings
that contain that as a substring(*). It might some sense to handle
pg_tblspc similarly. As for pg_logical, I think you'll want separate
defines for pg_logical/mappings, pg_logical/snapshots and so on.
(*) I hope you're not going to suggest this kind of change (word-diff):
if (GetOldestSnapshot())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("[-pg_wal-]{+%s+}_replay_wait() must be only called without an active or registered
snapshot"{+,PG_WAL_DIR+}),
errdetail("Make sure [-pg_wal-]{+%s+}_replay_wait() isn't called within a transaction with an
isolationlevel higher than READ COMMITTED, another procedure, or a function."{+, PG_WAL_DIR+})));
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Los trabajadores menos efectivos son sistematicamente llevados al lugar
donde pueden hacer el menor daño posible: gerencia." (El principio Dilbert)
On Wed, 14 Aug 2024 11:32:14 +0000
Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> Hi hackers,
>
> while working on a replication slot tool (idea is to put it in contrib, not
> shared yet), I realized that "pg_replslot" is being used > 25 times in
> .c files.
>
> I think it would make sense to replace those occurrences with a $SUBJECT, attached
> a patch doing so.
>
> There is 2 places where it is not done:
>
> src/bin/initdb/initdb.c
> src/bin/pg_rewind/filemap.c
>
> for consistency with the existing PG_STAT_TMP_DIR define.
>
> Out of curiosity, checking the sizes of affected files (O2, no debug):
>
> with patch:
>
> text data bss dec hex filename
> 20315 224 17 20556 504c src/backend/backup/basebackup.o
> 30304 0 8 30312 7668 src/backend/replication/logical/reorderbuffer.o
> 23812 36 40 23888 5d50 src/backend/replication/slot.o
> 6367 0 0 6367 18df src/backend/utils/adt/genfile.o
> 40997 2574 2528 46099 b413 src/bin/initdb/initdb.o
> 6963 224 8 7195 1c1b src/bin/pg_rewind/filemap.o
>
> without patch:
>
> text data bss dec hex filename
> 20315 224 17 20556 504c src/backend/backup/basebackup.o
> 30286 0 8 30294 7656 src/backend/replication/logical/reorderbuffer.o
> 23766 36 40 23842 5d22 src/backend/replication/slot.o
> 6363 0 0 6363 18db src/backend/utils/adt/genfile.o
> 40997 2574 2528 46099 b413 src/bin/initdb/initdb.o
> 6963 224 8 7195 1c1b src/bin/pg_rewind/filemap.o
>
> Also, I think we could do the same for:
>
> pg_notify
> pg_serial
> pg_subtrans
> pg_wal
> pg_multixact
> pg_tblspc
> pg_logical
>
> And I volunteer to do so if you think that makes sense.
>
> Looking forward to your feedback,
/* restore all slots by iterating over all on-disk entries */
- replication_dir = AllocateDir("pg_replslot");
- while ((replication_de = ReadDir(replication_dir, "pg_replslot")) != NULL)
+ replication_dir = AllocateDir(PG_REPLSLOT_DIR);
+ while ((replication_de = ReadDir(replication_dir, PG_REPLSLOT_DIR)) != NULL)
{
char path[MAXPGPATH + 12];
PGFileType de_type;
I think the size of path can be rewritten to "MAXPGPATH + sizeof(PG_REPLSLOT_DIR)"
and it seems easier to understand the reason why this size is used.
(That said, I wonder the path would never longer than MAXPGPATH...)
Regards,
Yugo Nagata
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
--
Yugo Nagata <nagata@sraoss.co.jp>
On Wed, Aug 14, 2024 at 5:02 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi hackers, > > while working on a replication slot tool (idea is to put it in contrib, not > shared yet), I realized that "pg_replslot" is being used > 25 times in > .c files. > > I think it would make sense to replace those occurrences with a $SUBJECT, attached > a patch doing so. Many of these places are slot specific directory/file names within pg_replslot. I think we can further improve the code by creating macro on the lines of LSN_FORMAT_ARGS #define SLOT_DIRNAME_ARGS(slotname) (PG_REPLSLOT_DIR, slotname) this way we "codify" method to construct the slot directory name everywhere. We may add another macro #define SLOT_DIRNAME_FORMAT "%s/%s" to further enforce the same. But I didn't find similar LSN_FORMAT macro defined as "%X/%X". I don't remember exactly why we didn't add it. May be because of trouble with translations. -- Best Wishes, Ashutosh Bapat
Hi,
On Fri, Aug 16, 2024 at 01:31:11PM +0900, Yugo Nagata wrote:
> On Wed, 14 Aug 2024 11:32:14 +0000
> Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> > Looking forward to your feedback,
>
> /* restore all slots by iterating over all on-disk entries */
> - replication_dir = AllocateDir("pg_replslot");
> - while ((replication_de = ReadDir(replication_dir, "pg_replslot")) != NULL)
> + replication_dir = AllocateDir(PG_REPLSLOT_DIR);
> + while ((replication_de = ReadDir(replication_dir, PG_REPLSLOT_DIR)) != NULL)
> {
> char path[MAXPGPATH + 12];
> PGFileType de_type;
>
> I think the size of path can be rewritten to "MAXPGPATH + sizeof(PG_REPLSLOT_DIR)"
> and it seems easier to understand the reason why this size is used.
Thanks for the feedback!
Yeah, fully agree, it has been done that way in v2 that I just shared up-thread.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Hi, On Mon, Aug 19, 2024 at 04:11:31PM +0530, Ashutosh Bapat wrote: > On Wed, Aug 14, 2024 at 5:02 PM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > > > Hi hackers, > > > > while working on a replication slot tool (idea is to put it in contrib, not > > shared yet), I realized that "pg_replslot" is being used > 25 times in > > .c files. > > > > I think it would make sense to replace those occurrences with a $SUBJECT, attached > > a patch doing so. > > Many of these places are slot specific directory/file names within > pg_replslot. I think we can further improve the code by creating macro > on the lines of LSN_FORMAT_ARGS > #define SLOT_DIRNAME_ARGS(slotname) (PG_REPLSLOT_DIR, slotname) > this way we "codify" method to construct the slot directory name > everywhere. Thanks for the feedback! I think that could make sense. As the already proposed mechanical changes are error prone (from my point of view), I would suggest to have a look at your proposal once the proposed changes go in. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Mon, Aug 19, 2024 at 7:47 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Mon, Aug 19, 2024 at 04:11:31PM +0530, Ashutosh Bapat wrote: > > On Wed, Aug 14, 2024 at 5:02 PM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > > > > Hi hackers, > > > > > > while working on a replication slot tool (idea is to put it in contrib, not > > > shared yet), I realized that "pg_replslot" is being used > 25 times in > > > .c files. > > > > > > I think it would make sense to replace those occurrences with a $SUBJECT, attached > > > a patch doing so. > > > > Many of these places are slot specific directory/file names within > > pg_replslot. I think we can further improve the code by creating macro > > on the lines of LSN_FORMAT_ARGS > > #define SLOT_DIRNAME_ARGS(slotname) (PG_REPLSLOT_DIR, slotname) > > this way we "codify" method to construct the slot directory name > > everywhere. > > Thanks for the feedback! > > I think that could make sense. As the already proposed mechanical changes are > error prone (from my point of view), I would suggest to have a look at your > proposal once the proposed changes go in. What do you mean by error prone? -- Best Wishes, Ashutosh Bapat
Hi, On Tue, Aug 20, 2024 at 09:26:59AM +0530, Ashutosh Bapat wrote: > On Mon, Aug 19, 2024 at 7:47 PM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > > > Hi, > > > > On Mon, Aug 19, 2024 at 04:11:31PM +0530, Ashutosh Bapat wrote: > > > On Wed, Aug 14, 2024 at 5:02 PM Bertrand Drouvot > > > <bertranddrouvot.pg@gmail.com> wrote: > > > > > > > > Hi hackers, > > > > > > > > while working on a replication slot tool (idea is to put it in contrib, not > > > > shared yet), I realized that "pg_replslot" is being used > 25 times in > > > > .c files. > > > > > > > > I think it would make sense to replace those occurrences with a $SUBJECT, attached > > > > a patch doing so. > > > > > > Many of these places are slot specific directory/file names within > > > pg_replslot. I think we can further improve the code by creating macro > > > on the lines of LSN_FORMAT_ARGS > > > #define SLOT_DIRNAME_ARGS(slotname) (PG_REPLSLOT_DIR, slotname) > > > this way we "codify" method to construct the slot directory name > > > everywhere. > > > > Thanks for the feedback! > > > > I think that could make sense. As the already proposed mechanical changes are > > error prone (from my point of view), I would suggest to have a look at your > > proposal once the proposed changes go in. > > What do you mean by error prone? I meant to say that it is "easy" to make mistakes when doing those manual mechanical changes. Also I think it's not that fun/easy to review, that's why I think it's better to do one change at a time. Does that make sense to you? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Tue, Aug 20, 2024 at 10:49 AM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Tue, Aug 20, 2024 at 09:26:59AM +0530, Ashutosh Bapat wrote: > > On Mon, Aug 19, 2024 at 7:47 PM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > > > > Hi, > > > > > > On Mon, Aug 19, 2024 at 04:11:31PM +0530, Ashutosh Bapat wrote: > > > > On Wed, Aug 14, 2024 at 5:02 PM Bertrand Drouvot > > > > <bertranddrouvot.pg@gmail.com> wrote: > > > > > > > > > > Hi hackers, > > > > > > > > > > while working on a replication slot tool (idea is to put it in contrib, not > > > > > shared yet), I realized that "pg_replslot" is being used > 25 times in > > > > > .c files. > > > > > > > > > > I think it would make sense to replace those occurrences with a $SUBJECT, attached > > > > > a patch doing so. > > > > > > > > Many of these places are slot specific directory/file names within > > > > pg_replslot. I think we can further improve the code by creating macro > > > > on the lines of LSN_FORMAT_ARGS > > > > #define SLOT_DIRNAME_ARGS(slotname) (PG_REPLSLOT_DIR, slotname) > > > > this way we "codify" method to construct the slot directory name > > > > everywhere. > > > > > > Thanks for the feedback! > > > > > > I think that could make sense. As the already proposed mechanical changes are > > > error prone (from my point of view), I would suggest to have a look at your > > > proposal once the proposed changes go in. > > > > What do you mean by error prone? > > I meant to say that it is "easy" to make mistakes when doing those manual > mechanical changes. Also I think it's not that fun/easy to review, that's why > I think it's better to do one change at a time. Does that make sense to you? Since these are all related changes, doing them at once might make it faster. You may use multiple commits (one for each change) to "contain" errors. -- Best Wishes, Ashutosh Bapat
Hi, On Tue, Aug 20, 2024 at 05:41:48PM +0900, Michael Paquier wrote: > On Tue, Aug 20, 2024 at 11:10:46AM +0530, Ashutosh Bapat wrote: > > Since these are all related changes, doing them at once might make it > > faster. You may use multiple commits (one for each change) > > Doing multiple commits with individual definitions for each path would > be the way to go for me. All that is mechanical, still that feels > slightly cleaner. Right, that's what v2 has done. If there is a need for v3 then I'll add one dedicated patch for Ashutosh's proposal in the v3 series. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On Tue, Aug 20, 2024 at 05:47:57PM +0900, Michael Paquier wrote: > On Mon, Aug 19, 2024 at 02:11:55PM +0000, Bertrand Drouvot wrote: > > I made the changes for pg_tblspc in pg_combinebackup.c as the number of occurences > > are greater that the "pg_wal" ones and we were to define PG_TBLSPC_DIR in any > > case. > > > > Please find attached the related patches. > > No real objection about the replslot and pg_logical bits. Thanks for looking at it! > > - * $PGDATA/pg_tblspc/spcoid/PG_MAJORVER_CATVER/dboid/relfilenumber > + * $PGDATA/PG_TBLSPC_DIR/spcoid/PG_MAJORVER_CATVER/dboid/relfilenumber > > For the tablespace parts, I am not sure that I would update the > comments to reflect the variables, TBH. Somebody reading the comments > would need to refer back to pg_tblspc/ in the header. I'm not sure as, for example, for PG_STAT_TMP_DIR we have those ones: src/backend/backup/basebackup.c: * Skip temporary statistics files. PG_STAT_TMP_DIR must be skipped src/bin/pg_rewind/filemap.c: * Skip temporary statistics files. PG_STAT_TMP_DIR must be skipped so I thought it would be better to be consistent. That said, I don't have a strong opinion about it, but then I guess you'd want to do the same for the ones related to replslot: src/backend/replication/slot.c: * Each replication slot gets its own directory inside the $PGDATA/PG_REPLSLOT_DIR src/backend/utils/adt/genfile.c: * Function to return the list of files in the PG_REPLSLOT_DIR/<replication_slot> and pg_logical: src/backend/utils/adt/genfile.c: * Function to return the list of files in the PG_LOGICAL_SNAPSHOTS_DIR directory. src/backend/utils/adt/genfile.c: * Function to return the list of files in the PG_LOGICAL_MAPPINGS_DIR directory. , right? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Tue, 20 Aug 2024 17:47:57 +0900
Michael Paquier <michael@paquier.xyz> wrote:
> On Mon, Aug 19, 2024 at 02:11:55PM +0000, Bertrand Drouvot wrote:
> > I made the changes for pg_tblspc in pg_combinebackup.c as the number of occurences
> > are greater that the "pg_wal" ones and we were to define PG_TBLSPC_DIR in any
> > case.
> >
> > Please find attached the related patches.
>
> No real objection about the replslot and pg_logical bits.
>
> - * $PGDATA/pg_tblspc/spcoid/PG_MAJORVER_CATVER/dboid/relfilenumber
> + * $PGDATA/PG_TBLSPC_DIR/spcoid/PG_MAJORVER_CATVER/dboid/relfilenumber
>
> For the tablespace parts, I am not sure that I would update the
> comments to reflect the variables, TBH. Somebody reading the comments
> would need to refer back to pg_tblspc/ in the header.
I also think that it is not necessary to change the comments even for pg_replslot.
- * Each replication slot gets its own directory inside the $PGDATA/pg_replslot
+ * Each replication slot gets its own directory inside the $PGDATA/PG_REPLSLOT_DIR
For example, I found that comments in xlog.c use "pg_wal" even though XLOGDIR is used
in the codes as below, and I don't feel any problem for this.
> static void
> ValidateXLOGDirectoryStructure(void)
> {
> char path[MAXPGPATH];
> struct stat stat_buf;
>
> /* Check for pg_wal; if it doesn't exist, error out */
> if (stat(XLOGDIR, &stat_buf) != 0 ||
> !S_ISDIR(stat_buf.st_mode))
Should be the follwing also rewritten using sizeof(PG_REPLSLOT_DIR)?
struct stat statbuf;
char path[MAXPGPATH * 2 + 12];
Regards,
Yugo Nagata
--
Yugo Nagata <nagata@sraoss.co.jp>
On 2024-Aug-19, Bertrand Drouvot wrote: > diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h > index 6f006d5a93..a6cb091635 100644 > --- a/src/include/common/relpath.h > +++ b/src/include/common/relpath.h > @@ -33,6 +33,10 @@ typedef Oid RelFileNumber; > #define TABLESPACE_VERSION_DIRECTORY "PG_" PG_MAJORVERSION "_" \ > CppAsString2(CATALOG_VERSION_NO) > > +#define PG_TBLSPC_DIR "pg_tblspc" This one is missing some commentary along the lines of "This must not be changed, unless you want to break every tool in the universe". As is, it's quite tempting. > +#define PG_TBLSPC_DIR_SLASH PG_TBLSPC_DIR "/" I would make this simply "pg_tblspc/", since it's not really possible to change pg_tblspc anyway. Also, have a comment explaining why we have it. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Los dioses no protegen a los insensatos. Éstos reciben protección de otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)
Hi, On Tue, Aug 20, 2024 at 09:30:48PM +0900, Yugo Nagata wrote: > Should be the follwing also rewritten using sizeof(PG_REPLSLOT_DIR)? > > struct stat statbuf; > char path[MAXPGPATH * 2 + 12]; > > Yeah, done in v3 that I just shared up-thread (also removed the macros from the comments). Thanks! Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On Tue, Aug 20, 2024 at 12:06:52PM +0000, Bertrand Drouvot wrote: > Hi, > > On Tue, Aug 20, 2024 at 05:41:48PM +0900, Michael Paquier wrote: > > On Tue, Aug 20, 2024 at 11:10:46AM +0530, Ashutosh Bapat wrote: > > > Since these are all related changes, doing them at once might make it > > > faster. You may use multiple commits (one for each change) > > > > Doing multiple commits with individual definitions for each path would > > be the way to go for me. All that is mechanical, still that feels > > slightly cleaner. > > Right, that's what v2 has done. If there is a need for v3 then I'll add one > dedicated patch for Ashutosh's proposal in the v3 series. Ashutosh's idea is implemented in v3 that I just shared up-thread (I used REPLSLOT_DIR_ARGS though). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
If this isn't in commitfest already, please add it there. On Tue, Aug 20, 2024 at 9:58 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Tue, Aug 20, 2024 at 12:06:52PM +0000, Bertrand Drouvot wrote: > > Hi, > > > > On Tue, Aug 20, 2024 at 05:41:48PM +0900, Michael Paquier wrote: > > > On Tue, Aug 20, 2024 at 11:10:46AM +0530, Ashutosh Bapat wrote: > > > > Since these are all related changes, doing them at once might make it > > > > faster. You may use multiple commits (one for each change) > > > > > > Doing multiple commits with individual definitions for each path would > > > be the way to go for me. All that is mechanical, still that feels > > > slightly cleaner. > > > > Right, that's what v2 has done. If there is a need for v3 then I'll add one > > dedicated patch for Ashutosh's proposal in the v3 series. > > Ashutosh's idea is implemented in v3 that I just shared up-thread (I used > REPLSLOT_DIR_ARGS though). > > Regards, > > -- > Bertrand Drouvot > PostgreSQL Contributors Team > RDS Open Source Databases > Amazon Web Services: https://aws.amazon.com -- Best Wishes, Ashutosh Bapat
Hi, On Wed, Aug 21, 2024 at 10:14:20AM +0530, Ashutosh Bapat wrote: > If this isn't in commitfest already, please add it there. > Done in [0]. [0]: https://commitfest.postgresql.org/49/5193/ Thanks! Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
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