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 ====================== 1. IIRC, earlier, we thought of allowing to use of this option (SKIP) only for superusers (as this can lead to inconsistent data if not used carefully) but I don't see that check in the latest patch. What is the reason for the same? 2. + /* + * Update the subskiplsn of the tuple to InvalidXLogRecPtr. I think we can change the above part of the comment to "Clear subskiplsn." 3. + * Since we already have Isn't it better to say here: Since we have already ...? -- With Regards, Amit Kapila.
