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