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
