Re: Skipping logical replication transactions on subscriber side

On Thu, Mar 17, 2022 at 12:39 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Thursday, March 17, 2022 3:04 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Wed, Mar 16, 2022 at 1:53 PM Masahiko Sawada
> > <sawada(dot)mshk(at)gmail(dot)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.