On Thu, Mar 17, 2022 at 12:39 PM osumi.takami...@fujitsu.com <osumi.takami...@fujitsu.com> wrote: > > On Thursday, March 17, 2022 3:04 PM Amit Kapila <amit.kapil...@gmail.com> > wrote: > > On Wed, Mar 16, 2022 at 1:53 PM Masahiko Sawada > > <sawada.m...@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.
