RE: Avoid streaming the transaction which are skipped (in corner cases)

On Saturday, December 3, 2022 7:37 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Tue, Nov 29, 2022 at 12:23 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Tuesday, November 29, 2022 12:08 PM Dilip Kumar
> <dilipbalaut@gmail.com> wrote:
> >
> > I have few comments about the patch.
> >
> > 1.
> >
> > 1.1.
> > -       /* For streamed transactions notify the remote node about the abort.
> */
> > -       if (rbtxn_is_streamed(txn))
> > -               rb->stream_abort(rb, txn, lsn);
> > +       /* the transaction which is being skipped shouldn't have been
> streamed */
> > +       Assert(!rbtxn_is_streamed(txn));
> >
> > 1.2
> > -               rbtxn_is_serialized(txn))
> > +               rbtxn_is_serialized(txn) &&
> > +               rbtxn_has_streamable_change(txn))
> >                 ReorderBufferStreamTXN(rb, toptxn);
> >
> > In the above two places, I think we should do the check for the
> > top-level transaction(e.g. toptxn) because the patch only set flag for
> > the top-level transaction.
> >
> 
> Among these, the first one seems okay because it will check both the transaction
> and its subtransactions from that path and none of those should be marked as
> streamed. I have fixed the second one in the attached patch.
> 
> > 2.
> >
> > +       /*
> > +        * If there are any streamable changes getting queued then get the
> top
> > +        * transaction and mark it has streamable change.  This is required
> for
> > +        * streaming in-progress transactions, the in-progress transaction will
> > +        * not be selected for streaming unless it has at least one streamable
> > +        * change.
> > +        */
> > +       if (change->action == REORDER_BUFFER_CHANGE_INSERT ||
> > +               change->action == REORDER_BUFFER_CHANGE_UPDATE ||
> > +               change->action == REORDER_BUFFER_CHANGE_DELETE ||
> > +               change->action ==
> REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT ||
> > +               change->action ==
> REORDER_BUFFER_CHANGE_TRUNCATE)
> >
> > I think that a transaction that contains REORDER_BUFFER_CHANGE_MESSAGE
> > can also be considered as streamable. Is there a reason that we don't check it
> here ?
> >
> 
> No, I don't see any reason not to do this check for
> REORDER_BUFFER_CHANGE_MESSAGE.
> 
> Apart from the above, I have slightly adjusted the comments in the attached. Do
> let me know what you think of the attached.

Thanks for updating the patch. It looks good to me.

Best regards,
Hou zj