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.
--
With Regards,
Amit Kapila.
