Horton

Re: Skipping logical replication transactions on subscriber side

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>
Cc: Masahiko Sawada <sawada.mshk@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-17T08:52:20Z
Lists: pgsql-hackers 
On Thu, Mar 17, 2022 at 12:39 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> 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.
>

Hmm, the current comment seems more appropriate. What you are
suggesting is almost writing the code in sentence form.

>
> (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
>
...
>
> 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.
>

I think the intention is to avoid duplicate code as we have a common
function that gets called from both of those. OTOH, if Sawada-San or
others also prefer your approach to rearrange the code then I am fine
with it.

-- 
With Regards,
Amit Kapila.

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