Re: Skipping logical replication transactions on subscriber side - Mailing list pgsql-hackers

On Fri, Aug 13, 2021 at 1:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Aug 12, 2021 at 5:41 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
> >
> > On Thu, Aug 12, 2021 at 9:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > > A minor comment on the 0001 patch: In the message I think that using
> > > > "ID" would look better than lowercase "id" and AFAICS it's more
> > > > consistent with existing messages.
> > > >
> > > > + appendStringInfo(&buf, _(" in transaction id %u with commit timestamp %s"),
> > > >
> > >
> > > You have a point but I think in this case it might look a bit odd as
> > > we have another field 'commit timestamp' after that which is
> > > lowercase.
> > >
> >
> > I did a quick search and I couldn't find any other messages in the
> > Postgres code that use "transaction id", but I could find some that
> > use "transaction ID" and "transaction identifier".
> >
>
> Okay, but that doesn't mean using it here is bad. I am personally fine
> with a message containing something like "... in transaction
> id 740 with commit timestamp 2021-08-10 14:44:38.058174+05:30" but I
> won't mind if you and or others find some other way convenient. Any
> opinion from others?

I don't have a strong opinion on this but in terms of consistency we
often use like "transaction %u" in messages when showing XID value,
rather than "transaction [id|ID|identifier]":

$ git grep -i "errmsg.*transaction %u" src/backend/
src/backend/access/transam/commit_ts.c:              errmsg("cannot
retrieve commit timestamp for transaction %u", xid)));
src/backend/access/transam/slru.c:                   errmsg("could not
access status of transaction %u", xid),
src/backend/access/transam/slru.c:                   errmsg("could not
access status of transaction %u", xid),
src/backend/access/transam/slru.c:                       errmsg("could
not access status of transaction %u", xid),
src/backend/access/transam/slru.c:                      (errmsg("could
not access status of transaction %u", xid),
src/backend/access/transam/slru.c:                       errmsg("could
not access status of transaction %u", xid),
src/backend/access/transam/slru.c:                      (errmsg("could
not access status of transaction %u", xid),
src/backend/access/transam/slru.c:                   errmsg("could not
access status of transaction %u", xid),
src/backend/access/transam/slru.c:                   errmsg("could not
access status of transaction %u", xid),
src/backend/access/transam/twophase.c:
(errmsg("recovering prepared transaction %u from shared memory",
xid)));
src/backend/access/transam/twophase.c:
(errmsg("removing stale two-phase state file for transaction %u",
src/backend/access/transam/twophase.c:
(errmsg("removing stale two-phase state from memory for transaction
%u",
src/backend/access/transam/twophase.c:
(errmsg("removing future two-phase state file for transaction %u",
src/backend/access/transam/twophase.c:
(errmsg("removing future two-phase state from memory for transaction
%u",
src/backend/access/transam/twophase.c:
errmsg("corrupted two-phase state file for transaction %u",
src/backend/access/transam/twophase.c:
errmsg("corrupted two-phase state in memory for transaction %u",
src/backend/access/transam/xlog.c:                  (errmsg("recovery
stopping before commit of transaction %u, time %s",
src/backend/access/transam/xlog.c:                  (errmsg("recovery
stopping before abort of transaction %u, time %s",
src/backend/access/transam/xlog.c:
(errmsg("recovery stopping after commit of transaction %u, time %s",
src/backend/access/transam/xlog.c:
(errmsg("recovery stopping after abort of transaction %u, time %s",
src/backend/replication/logical/worker.c:
errmsg_internal("transaction %u not found in stream XID hash table",
src/backend/replication/logical/worker.c:
errmsg_internal("transaction %u not found in stream XID hash table",
src/backend/replication/logical/worker.c:
errmsg_internal("transaction %u not found in stream XID hash table",
src/backend/replication/logical/worker.c:
errmsg_internal("transaction %u not found in stream XID hash table",

$ git grep -i "errmsg.*transaction identifier" src/backend/
src/backend/access/transam/twophase.c:
errmsg("transaction identifier \"%s\" is too long",
src/backend/access/transam/twophase.c:
errmsg("transaction identifier \"%s\" is already in use",

$ git grep -i "errmsg.*transaction id" src/backend/
src/backend/access/transam/twophase.c:
errmsg("transaction identifier \"%s\" is too long",
src/backend/access/transam/twophase.c:
errmsg("transaction identifier \"%s\" is already in use",
src/backend/access/transam/varsup.c:
(errmsg_internal("transaction ID wrap limit is %u, limited by database
with OID %u",
src/backend/access/transam/xlog.c:          (errmsg_internal("next
transaction ID: " UINT64_FORMAT "; next OID: %u",
src/backend/access/transam/xlog.c:          (errmsg_internal("oldest
unfrozen transaction ID: %u, in database %u",
src/backend/access/transam/xlog.c:              (errmsg("invalid next
transaction ID")));
src/backend/replication/logical/snapbuild.c:
(errmsg_plural("exported logical decoding snapshot: \"%s\" with %u
transaction ID",
src/backend/replication/logical/worker.c:
errmsg_internal("invalid transaction ID in streamed replication
transaction")));
src/backend/replication/logical/worker.c:
errmsg_internal("invalid transaction ID in streamed replication
transaction")));
src/backend/replication/logical/worker.c:
errmsg_internal("invalid two-phase transaction ID")));
src/backend/utils/adt/xid8funcs.c:               errmsg("transaction
ID %s is in the future",

Therefore, perhaps a message like "... in transaction 740 with commit
timestamp 2021-08-10 14:44:38.058174+05:30" is better in terms of
consistency with other messages?

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/