On Tuesday, November 29, 2022 12:08 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
Hi, > > On Mon, Nov 28, 2022 at 3:19 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Mon, Nov 28, 2022 at 1:46 PM shiy.f...@fujitsu.com > > <shiy.f...@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