Re: define PG_REPLSLOT_DIR

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