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