On Wed, Mar 16, 2022 at 1:20 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Mar 16, 2022 at 7:58 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Wed, Mar 16, 2022 at 6:03 AM Masahiko Sawada <sawada.m...@gmail.com> > > wrote: > > > > > > On Tue, Mar 15, 2022 at 7:18 PM Amit Kapila <amit.kapil...@gmail.com> > > > wrote: > > > > > > > > On Tue, Mar 15, 2022 at 11:43 AM Masahiko Sawada > > > > <sawada.m...@gmail.com> wrote: > > > > > > > > > > > > > 6. > > > > @@ -1583,7 +1649,8 @@ apply_handle_insert(StringInfo s) > > > > TupleTableSlot *remoteslot; > > > > MemoryContext oldctx; > > > > > > > > - if (handle_streamed_transaction(LOGICAL_REP_MSG_INSERT, s)) > > > > + if (is_skipping_changes() || > > > > > > > > Is there a reason to keep the skip_changes check here and in other DML > > > > operations instead of at one central place in apply_dispatch? > > > > > > Since we already have the check of applying the change on the spot at > > > the beginning of the handlers I feel it's better to add > > > is_skipping_changes() to the check than add a new if statement to > > > apply_dispatch, but do you prefer to check it in one central place in > > > apply_dispatch? > > > > > > > I think either way is fine. I just wanted to know the reason, your > > current change looks okay to me. > > > > Some questions/comments > > ====================== > > > > Some cosmetic suggestions: > ====================== > 1. > +# Create subscriptions. Both subscription sets disable_on_error to on > +# so that they get disabled when a conflict occurs. > +$node_subscriber->safe_psql( > + 'postgres', > + qq[ > +CREATE SUBSCRIPTION $subname CONNECTION '$publisher_connstr' > PUBLICATION tap_pub WITH (streaming = on, two_phase = on, > disable_on_error = on); > +]); > > I don't understand what you mean by 'Both subscription ...' in the > above comments.
Fixed. > > 2. > + # Check the log indicating that successfully skipped the transaction, > > How about slightly rephrasing this to: "Check the log to ensure that > the transaction is skipped...."? Fixed. I've attached an updated version patch. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
v15-0001-Add-ALTER-SUBSCRIPTION-.-SKIP-to-skip-the-transa.patch
Description: Binary data
