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. -- With Regards, Amit Kapila.
v4-0001-Avoid-unnecessary-streaming-of-transactions-durin.patch
Description: Binary data
