On Saturday, December 3, 2022 7:37 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Nov 29, 2022 at 12:23 PM houzj.f...@fujitsu.com > <houzj.f...@fujitsu.com> wrote: > > > > On Tuesday, November 29, 2022 12:08 PM Dilip Kumar > <dilipbal...@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
