define PG_REPLSLOT_DIR

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 isolation level 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 Thu, Aug 15, 2024 at 08:40:42PM -0400, Alvaro Herrera wrote:

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 did not look in details, but yeah that could probably be explained that way.

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 isolation level higher than READ COMMITTED, another procedure, or a function."{+, PG_WAL_DIR+})));

Yeah, would not cross my mind ;-). I might have been tempted to do the change
in pg_combinebackup.c though.

Having said that, I agree to focus only on:

pg_replslot
pg_tblspc
pg_logical/*

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.

Regards,

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

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

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.

to "contain" errors.

I am not sure what you mean by that.
--
Michael

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.
--
Michael

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 10:15:44AM -0400, Alvaro Herrera wrote:

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.

Yeah, makes sense, thanks.

+#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.

Please find attached v3 that:

- takes care of your comments (and also removed the use of PG_TBLSPC_DIR in
RELATIVE_PG_TBLSPC_DIR).
- removes the new macros from the comments (see Michael's and Yugo-San's
comments in [0]/messages/by-id/ZsRYPcOtoqbWzjGG@paquier.xyz resp. [1]/messages/by-id/20240820213048.207aade6a75e0dc1fe4d1067@sraoss.co.jp).
- adds a missing sizeof() (see [1]/messages/by-id/20240820213048.207aade6a75e0dc1fe4d1067@sraoss.co.jp).
- implements Ashutosh's idea of adding a new SLOT_DIRNAME_ARGS (see [2]/messages/by-id/CAExHW5vkjxuvyQ1fPPnuDW4nAT5jqox09ie36kciOV2+rhjbHA@mail.gmail.com). It's
done in 0002 (I used REPLSLOT_DIR_ARGS though).
- fixed a macro usage in ReorderBufferCleanupSerializedTXNs() (was not at the
right location, discovered while implementing 0002).

[0]: /messages/by-id/ZsRYPcOtoqbWzjGG@paquier.xyz
[1]: /messages/by-id/20240820213048.207aade6a75e0dc1fe4d1067@sraoss.co.jp
[2]: /messages/by-id/CAExHW5vkjxuvyQ1fPPnuDW4nAT5jqox09ie36kciOV2+rhjbHA@mail.gmail.com

Regards,

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

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