Re: define PG_REPLSLOT_DIR - Mailing list pgsql-hackers

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>