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

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.