Re: define PG_REPLSLOT_DIR

On Wed, 14 Aug 2024 11:32:14 +0000
Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)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(at)sraoss(dot)co(dot)jp>