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

On Tuesday, November 29, 2022 12:08 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

Hi,

>
> On Mon, Nov 28, 2022 at 3:19 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Mon, Nov 28, 2022 at 1:46 PM shiy(dot)fnst(at)fujitsu(dot)com
> > <shiy(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > > Thanks for your patch.
> > >
> > > I saw that the patch added a check when selecting largest
> > > transaction, but in addition to ReorderBufferCheckMemoryLimit(), the
> > > transaction can also be streamed in
> > > ReorderBufferProcessPartialChange(). Should we add the check in this
> function, too?
> > >
> > > diff --git a/src/backend/replication/logical/reorderbuffer.c
> > > b/src/backend/replication/logical/reorderbuffer.c
> > > index 9a58c4bfb9..108737b02f 100644
> > > --- a/src/backend/replication/logical/reorderbuffer.c
> > > +++ b/src/backend/replication/logical/reorderbuffer.c
> > > @@ -768,7 +768,8 @@
> ReorderBufferProcessPartialChange(ReorderBuffer *rb, ReorderBufferTXN
> *txn,
> > > */
> > > if (ReorderBufferCanStartStreaming(rb) &&
> > > !(rbtxn_has_partial_change(toptxn)) &&
> > > - rbtxn_is_serialized(txn))
> > > + rbtxn_is_serialized(txn) &&
> > > + rbtxn_has_streamable_change(txn))
> > > ReorderBufferStreamTXN(rb, toptxn); }
> >
> > You are right we need this in ReorderBufferProcessPartialChange() as
> > well. I will fix this in the next version.
>
> Fixed this in the attached patch.

Thanks for updating the patch.

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.

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 ?

Best regards,
Hou zj