Horton

Thread

Commits

  1. Define PG_TBLSPC_DIR for path pg_tblspc/ in data folder

  2. Define PG_LOGICAL_DIR for path pg_logical/ in data folder

  3. Define PG_REPLSLOT_DIR for path pg_replslot/ in data folder

  1. define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-14T11:32:14Z

    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

  2. Re: define PG_REPLSLOT_DIR

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-08-16T00:40:42Z

    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)

  3. Re: define PG_REPLSLOT_DIR

    Yugo Nagata <nagata@sraoss.co.jp> — 2024-08-16T04:31:11Z

    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>

  4. Re: define PG_REPLSLOT_DIR

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-08-19T10:41:31Z

    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

  5. Re: define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-19T14:11:55Z

    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

  6. Re: define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-19T14:13:37Z

    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

  7. Re: define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-19T14:17:50Z

    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

  8. Re: define PG_REPLSLOT_DIR

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-08-20T03:56:59Z

    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

  9. Re: define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-20T05:19:56Z

    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

  10. Re: define PG_REPLSLOT_DIR

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-08-20T05:40:46Z

    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

  11. Re: define PG_REPLSLOT_DIR

    Michael Paquier <michael@paquier.xyz> — 2024-08-20T08:41:48Z

    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

  12. Re: define PG_REPLSLOT_DIR

    Michael Paquier <michael@paquier.xyz> — 2024-08-20T08:47:57Z

    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

  13. Re: define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-20T12:06:52Z

    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

  14. Re: define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-20T12:16:10Z

    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

  15. Re: define PG_REPLSLOT_DIR

    Yugo Nagata <nagata@sraoss.co.jp> — 2024-08-20T12:30:48Z

    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>

  16. Re: define PG_REPLSLOT_DIR

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-08-20T14:15:44Z

    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)

  17. Re: define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-20T16:23:06Z

    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] resp. [1]).
- adds a missing sizeof() (see [1]).
- implements Ashutosh's idea of adding a new SLOT_DIRNAME_ARGS (see [2]). 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]: https://www.postgresql.org/message-id/ZsRYPcOtoqbWzjGG%40paquier.xyz
[1]: https://www.postgresql.org/message-id/20240820213048.207aade6a75e0dc1fe4d1067%40sraoss.co.jp
[2]: https://www.postgresql.org/message-id/CAExHW5vkjxuvyQ1fPPnuDW4nAT5jqox09ie36kciOV2%2BrhjbHA%40mail.gmail.com

Regards,

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

  18. Re: define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-20T16:26:23Z

    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

  19. Re: define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-20T16:28:34Z

    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

  20. Re: define PG_REPLSLOT_DIR

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-08-21T04:44:20Z

    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

  21. Re: define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-21T05:03:57Z

    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

  22. Re: define PG_REPLSLOT_DIR

    Michael Paquier <michael@paquier.xyz> — 2024-08-30T06:34:56Z

    On Tue, Aug 20, 2024 at 04:23:06PM +0000, Bertrand Drouvot wrote:
> 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] resp. [1]).
> - adds a missing sizeof() (see [1]).
> - implements Ashutosh's idea of adding a new SLOT_DIRNAME_ARGS (see [2]). 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).

I was looking at that, and applied 0001 for pg_replslot and merged
0003~0005 together for pg_logical.  For the first one, at the end I
have updated the comments in genfile.c.  For the second one, it looked
a bit strange to ignore "pg_logical/", which is the base for all the
others, so I have added a variable for it.  Locating them in
reorderbuffer.h with the GUCs was OK, but perhaps there was an
argument for logical.h.  The paths in origin.c refer to files, not
directories.

Not sure that 0002 is an improvement overall, so I have left this part
out.

In 0006, I am not sure that RELATIVE_PG_TBLSPC_DIR is really something
we should have.  Sure, that's a special case for base backups when
sending a directory, but we have also pg_wal/ and its XLOGDIR so
that's inconsistent.  Perhaps this part should be left as-is.
--
Michael

  23. Re: define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-30T12:21:29Z

    Hi,

On Fri, Aug 30, 2024 at 03:34:56PM +0900, Michael Paquier wrote:
> On Tue, Aug 20, 2024 at 04:23:06PM +0000, Bertrand Drouvot wrote:
> > 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] resp. [1]).
> > - adds a missing sizeof() (see [1]).
> > - implements Ashutosh's idea of adding a new SLOT_DIRNAME_ARGS (see [2]). 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).
> 
> I was looking at that, and applied 0001 for pg_replslot and merged
> 0003~0005 together for pg_logical.

Thanks!

> In 0006, I am not sure that RELATIVE_PG_TBLSPC_DIR is really something
> we should have.  Sure, that's a special case for base backups when
> sending a directory, but we have also pg_wal/ and its XLOGDIR so
> that's inconsistent.

That's right but OTOH there is no (for good reason) PG_WAL_DIR or such defined.

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.

Regards,

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

  24. Re: define PG_REPLSLOT_DIR

    Michael Paquier <michael@paquier.xyz> — 2024-09-03T00:15:50Z

    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.

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.  At
the end, I got no objections about the remaining pieces, so applied.

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

  25. Re: define PG_REPLSLOT_DIR

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-09-03T04:35:28Z

    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

  26. Re: define PG_REPLSLOT_DIR

    Michael Paquier <michael@paquier.xyz> — 2024-09-03T06:27:37Z

    On Tue, Sep 03, 2024 at 04:35:28AM +0000, Bertrand Drouvot wrote:
> 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?

Yes.  Folks are not going to do that, but changing PG_TBLSPC_DIR would
change the decision-making when streaming from versions older than
v17 with clients tools from v18~.
--
Michael