RE: Avoid streaming the transaction which are skipped (in corner cases) - Mailing list pgsql-hackers

On Tuesday, November 29, 2022 12:08 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Hi,

> 
> On Mon, Nov 28, 2022 at 3:19 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Mon, Nov 28, 2022 at 1:46 PM shiy.fnst@fujitsu.com
> > <shiy.fnst@fujitsu.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