RE: Skipping logical replication transactions on subscriber side
От | osumi.takamichi@fujitsu.com |
---|---|
Тема | RE: Skipping logical replication transactions on subscriber side |
Дата | |
Msg-id | TYCPR01MB837319BD0128F4B2018C4B6BED129@TYCPR01MB8373.jpnprd01.prod.outlook.com обсуждение исходный текст |
Ответ на | Re: Skipping logical replication transactions on subscriber side (Amit Kapila <amit.kapila16@gmail.com>) |
Ответы |
Re: Skipping logical replication transactions on subscriber side
(Amit Kapila <amit.kapila16@gmail.com>)
|
Список | 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
В списке pgsql-hackers по дате отправления:
Следующее
От: Pavan DeolaseeДата:
Сообщение: Shmem queue is not flushed if receiver is not yet attached