Horton
Menu

Re: Perform streaming logical transactions by background workers and parallel apply

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Cc: "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>, Masahiko Sawada <sawada.mshk@gmail.com>, "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2022-06-21T04:24:14Z
Lists: pgsql-hackers 
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.

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix invalid memory access during the shutdown of the parallel apply worker.

  2. Fix assertion failure in apply worker.

  3. Use elog to report unexpected action in handle_streamed_transaction().

  4. Use appropriate wait event when sending data in the apply worker.

  5. Allow the logical_replication_mode to be used on the subscriber.

  6. Rename GUC logical_decoding_mode to logical_replication_mode.

  7. Display the leader apply worker's PID for parallel apply workers.

  8. Improve the code to decide and process the apply action.

  9. Document the newly added wait events added by commit 216a784829.

  10. Perform apply of large transactions by parallel workers.

  11. Wake up a subscription's replication worker processes after DDL.

  12. Add copyright notices to meson files

  13. Better document logical replication parameters

  14. Add a common function to generate the origin name.

  15. Harmonize parameter names in storage and AM code.

  16. Avoid using list_length() to test for empty list.

  17. Improve two comments related to a boolean DefElem's value

  18. Fix partition table's REPLICA IDENTITY checking on the subscriber.

  19. Fix data inconsistency between publisher and subscriber.

  20. Fix cache look-up failures while applying changes in logical replication.