On Tue, Jun 21, 2022 at 7:11 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Here are some review comments for the v11-0001 patch. > > (I will review the remaining patches 0002-0005 and post any comments later) > > ====== > > 1. General > > I still feel that 'apply' seems like a meaningless enum value for this > feature because from a user point-of-view every replicated change gets > "applied". IMO something like 'streaming = parallel' or 'streaming = > background' (etc) might have more meaning for a user. >
+1. I would prefer 'streaming = parallel' as that suits here because we allow streams (set of changes) of a transaction to be applied in parallel to other transactions or in parallel to a stream of changes from another streaming transaction. > ====== > > 10. src/backend/access/transam/xact.c > > @@ -1741,6 +1742,13 @@ RecordTransactionAbort(bool isSubXact) > elog(PANIC, "cannot abort transaction %u, it was already committed", > xid); > > + /* > + * Are we using the replication origins feature? Or, in other words, > + * are we replaying remote actions? > + */ > + replorigin = (replorigin_session_origin != InvalidRepOriginId && > + replorigin_session_origin != DoNotReplicateId); > + > /* Fetch the data we need for the abort record */ > nrels = smgrGetPendingDeletes(false, &rels); > nchildren = xactGetCommittedChildren(&children); > @@ -1765,6 +1773,11 @@ RecordTransactionAbort(bool isSubXact) > MyXactFlags, InvalidTransactionId, > NULL); > > + if (replorigin) > + /* Move LSNs forward for this replication origin */ > + replorigin_session_advance(replorigin_session_origin_lsn, > + XactLastRecEnd); > + > > I did not see any reason why the code assigning the 'replorigin' and > the code checking the 'replorigin' are separated like they are. I > thought these 2 new code fragments should be kept together. Perhaps it > was decided this assignment must be outside the critical section? But > if that’s the case maybe a comment explaining so would be good. > I also don't see any particular reason for this apart from being similar to RecordTransactionCommit(). I think it should be fine either way. > ~~~ > > 11. src/backend/access/transam/xact.c > > + if (replorigin) > + /* Move LSNs forward for this replication origin */ > + replorigin_session_advance(replorigin_session_origin_lsn, > + > > The positioning of that comment is unusual. Maybe better before the check? > This again seems to be due to a similar code in RecordTransactionCommit(). I would suggest let's keep the code consistent. -- With Regards, Amit Kapila.
