Horton

RE: Skipping logical replication transactions on subscriber side

Takamichi Osumi (Fujitsu) <osumi.takamichi@fujitsu.com>

View thread View on postgr.es
From: "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>
To: 'Masahiko Sawada' <sawada.mshk@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, "David G. Johnston" <david.g.johnston@gmail.com>, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, vignesh C <vignesh21@gmail.com>, Greg Nancarrow <gregn4422@gmail.com>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, Alexey Lesovsky <lesovsky@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2022-03-15T14:00:52Z
Lists: pgsql-hackers 
On Tuesday, March 15, 2022 3:13 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I've attached an updated version patch.

A couple of minor comments on v14.

(1) apply_handle_commit_internal


+       if (is_skipping_changes())
+       {
+               stop_skipping_changes();
+
+               /*
+                * Start a new transaction to clear the subskipxid, if not started
+                * yet. The transaction is committed below.
+                */
+               if (!IsTransactionState())
+                       StartTransactionCommand();
+       }
+

I suppose we can move this condition check and stop_skipping_changes() call
to the inside of the block we enter when IsTransactionState() returns true.

As the comment of apply_handle_commit_internal() mentions,
it's the helper function for apply_handle_commit() and
apply_handle_stream_commit().

Then, I couldn't think that both callers don't open
a transaction before the call of apply_handle_commit_internal().
For applying spooled messages, we call begin_replication_step as well.

I can miss something, but timing when we receive COMMIT message
without opening a transaction, would be the case of empty transactions
where the subscription (and its subscription worker) is not interested.
If this is true, currently the patch's code includes
such cases within the range of is_skipping_changes() check.

(2) clear_subscription_skip_lsn's comments.

The comments for this function shouldn't touch
update of origin states, now that we don't update those.

+/*
+ * Clear subskiplsn of pg_subscription catalog with origin state updated.
+ *


This applies to other comments.

+       /*
+        * Update the subskiplsn of the tuple to InvalidXLogRecPtr.  If user has
+        * already changed subskiplsn before clearing it we don't update the
+        * catalog and don't advance the replication origin state.  
...
+        *            ....                We can reduce the possibility by
+        * logging a replication origin WAL record to advance the origin LSN
+        * instead but there is no way to advance the origin timestamp and it
+        * doesn't seem to be worth doing anything about it since it's a very rare
+        * case.
+        */



Best Regards,
	Takamichi Osumi

Commits

  1. Test ALIGNOF_DOUBLE==4 compatibility under ALIGNOF_DOUBLE==8.

  2. Reorder subskiplsn in pg_subscription to avoid alignment issues.

  3. Add ALTER SUBSCRIPTION ... SKIP.

  4. Optionally disable subscriptions on error.

  5. Update docs of logical replication for commit 8d74fc96db.

  6. Respect permissions within logical replication.

  7. Fix regression test failure caused by commit 8d74fc96db.

  8. Add a view to show the stats of subscription workers.

  9. Add logical change details to logical replication worker errcontext.

  10. Rename LOGICAL_REP_MSG_STREAM_END to LOGICAL_REP_MSG_STREAM_STOP.

  11. Fix typo in protocol.sgml.

  12. Remove unused argument in apply_handle_commit_internal().

  13. Fix replication of in-progress transactions in tablesync worker.

  14. Reorder pg_sequence columns to avoid alignment issue