Hi Shlok,
Thanks for your review.
> S1: BEGIN;
> S1: INSERT INTO t1 VALUES(11);
> S2: BEGIN;
> S2: ALTER TABLE t2 RENAME TO t2_new;
> S2: SAVEPOINT s1;
> S2: ALTER ROLE role1 RENAME TO role1_new;
> S2 ROLLBACK TO SAVEPOINT s1;
> S2: COMMIT;
>
> Now I debugged DecodeCommit for transaction in S2,
> It displayed parsed->nmsg = 10.
> Now I continued debugging and in 'SnapBuildDistributeSnapshotAndInval', we have:
> ninvalidations = ReorderBufferGetInvalidations(builder->reorder, xid, &msgs);
> Here, invalidations = 13.
> The first 10 invalidation messages in msgs were the same as
> parsed->msgs, but there were 3 additional messages. These additional
> messages had dbId = 0, indicating shared-catalog invalidations (I
> assume it is due to ALTER ROLE command).
>
> With the patch, InvalidationsTouchSharedCatalog(parsed->nmsgs,
> parsed->msgs) would therefore see only the first 10 messages, return
> false, and distribute would be set to false. The 3 shared-catalog
> invalidations returned by ReorderBufferGetInvalidations() would
> consequently not be distributed.
> Is this expected behavior? Thoughts?
Yes, it's expected.
parsed->msgs comes from the COMMIT WAL record, which carries only the
invalidation messages that survive to commit. When a subtransaction
aborts, AtEOSubXact_Inval(false) drops its messages instead of
propagating them to the parent. That is why parsed->nmsgs is 10.
ReorderBufferGetInvalidations(), on the other hand, returns the
accumulated set of the top-level transaction. While decoding an
XLOG_XACT_INVALIDATIONS record, its messages are merged into the
top-level transaction's invalidation array (rbtxn_get_toptxn() in
ReorderBufferAddInvalidations()), and that merge is irreversible: when
the subtransaction's abort record is later decoded,
ReorderBufferAbort() only cleans up the subtransaction's own reorder
buffer entry and cannot retract the messages already merged into the
top-level one. That is why it returns 13.
So, not distributing the invalidations of an aborted subxact
causes no correctness issue in this patch, because those catalog
changes were never committed.
Thanks,
Boyu Yang