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: 'Amit Kapila' <amit.kapila16@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>
Cc: 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-17T07:09:29Z
Lists: pgsql-hackers 
On Thursday, March 17, 2022 3:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Mar 16, 2022 at 1:53 PM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> >
> > I've attached an updated version patch.
> >
> 
> The patch LGTM. I have made minor changes in comments and docs in the
> attached patch. Kindly let me know what you think of the attached?
Hi, thank you for the patch. Few minor comments.


(1) comment of maybe_start_skipping_changes


+       /*
+        * Quick return if it's not requested to skip this transaction. This
+        * function is called for every remote transaction and we assume that
+        * skipping the transaction is not used often.
+        */

I feel this comment should explain more about our intention and
what it confirms. In a case when user requests skip,
but it doesn't match the condition, we don't start
skipping changes, strictly speaking.

From:
Quick return if it's not requested to skip this transaction.

To:
Quick return if we can't ensure possible skiplsn is set
and it equals to the finish LSN of this transaction.


(2) 029_on_error.pl

+       my $contents = slurp_file($node_subscriber->logfile, $offset);
+       $contents =~
+         qr/processing remote data for replication origin \"pg_\d+\" during "INSERT" for replication target relation "public.tbl" in transaction \d+ finishe$
+         or die "could not get error-LSN";

I think we shouldn't use a lot of new words.

How about a change below  ?

From:
could not get error-LSN
To:
failed to find expected error message that contains finish LSN for SKIP option


(3) apply_handle_commit_internal


Lastly, may I have the reasons to call both
stop_skipping_changes and clear_subscription_skip_lsn
in this function, instead of having them at the end
of apply_handle_commit and apply_handle_stream_commit ?

IMHO, this structure looks to create the
extra condition branches in apply_handle_commit_internal.

Also, because of this code, when we call stop_skipping_changes
in the apply_handle_commit_internal, after checking
is_skipping_changes() returns true, we check another
is_skipping_changes() at the top of stop_skipping_changes.

OTOH, for other cases like apply_handle_prepare, apply_handle_stream_prepare,
we call those two functions (or either one) depending on the needs,
after existing commits and during the closing processing.
(In the case of rollback_prepare, it's also called after existing commit)

I feel if we move those two functions at the end
of the apply_handle_commit and apply_handle_stream_commit,
then we will have more aligned codes and improve readability.



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