On Mon, Dec 6, 2021 at 5:09 PM Michael Paquier <mich...@paquier.xyz> wrote: > > On Mon, Dec 06, 2021 at 04:35:07PM +0900, Masahiko Sawada wrote: > > I've attached a patch to add replication origin information to > > xact_desc_prepare(). > > Yeah. > > + if (origin_id != InvalidRepOriginId) > + appendStringInfo(buf, "; origin: node %u, lsn %X/%X, at %s", > + origin_id, > + LSN_FORMAT_ARGS(parsed.origin_lsn), > + timestamptz_to_str(parsed.origin_timestamp)); > > Shouldn't you check for parsed.origin_lsn instead? The replication > origin is stored there as far as I read EndPrepare().
Yeah, I was thinking check origin_lsn instead. That way, we don't show invalid origin_timestamp and origin_lsn even if origin_id is set. But as far as I read, the same is true for xact_desc_commit() (and xact_desc_rollback()). That is, since apply workers always its origin id and could do commit transactions that are not replicated from the publisher, it's possible that xact_desc_commit() reports like: rmgr: Transaction len (rec/tot): 117/ 117, tx: 725, lsn: 0/014BE938, prev 0/014BE908, desc: COMMIT 2021-12-06 22:04:44.462200 JST; inval msgs: catcache 55 catcache 54 catcache 64; origin: node 1, lsn 0/0, at 2000-01-01 09:00:00.000000 JST Also, looking at PrepareRedoAdd(), we check the replication origin id. So I think that it'd be better to check origin_id for consistency. > > Commit records check after XACT_XINFO_HAS_ORIGIN, but > xact_desc_abort() may include this information for ROLLBACK PREPARED > transactions so we could use the same logic as xact_desc_commit() for > the abort case, no? Good catch! I'll submit an updated patch. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/