On Tue, Jun 21, 2022 at 7:11 AM Peter Smith <smithpb2250@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.