RE: Perform streaming logical transactions by background workers and parallel apply

On Friday, December 23, 2022 1:52 PM houzj(dot)fnst(at)fujitsu(dot)com <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Thursday, December 22, 2022 8:05 PM Amit Kapila
> <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Dec 21, 2022 at 11:02 AM houzj(dot)fnst(at)fujitsu(dot)com
> > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > > Attach the new patch set which also includes some cosmetic comment
> > > changes.
> > >
> >
> > Few minor comments:
> > =================
> > 1.
> > + <literal>t</literal> = spill the changes of in-progress
> > transactions to+ disk and apply at once after the transaction is
> > committed on the+ publisher,
> >
> > Can we change this description to: "spill the changes of in-progress
> > transactions to disk and apply at once after the transaction is
> > committed on the publisher and received by the subscriber,"
>
> Changed.
>
> > 2.
> > table is in progress, there will be additional workers for the tables
> > - being synchronized.
> > + being synchronized. Moreover, if the streaming transaction is applied in
> > + parallel, there will be additional workers.
> >
> > Do we need this change in the first patch? We skip parallel apply
> > workers from view for the first patch. Am, I missing something?
>
> No, I moved this to 0007 which include parallel apply workers in the view.
>
> > 3.
> > I think we would need a catversion bump for parallel apply feature
> > because of below change:
> > @@ -7913,11 +7913,16 @@ SCRAM-SHA-256$<replaceable>&lt;iteration
> > count&gt;</replaceable>:<replaceable>&l
> >
> > <row>
> > <entry role="catalog_table_entry"><para role="column_definition">
> > - <structfield>substream</structfield> <type>bool</type>
> > + <structfield>substream</structfield> <type>char</type>
> > </para>
> >
> > Am, I missing something? If not, then I think we can note that in the
> > commit message to avoid forgetting it before commit.
>
> Added.
>
> >
> > 4. Kindly change the below comments:
> > diff --git a/src/backend/replication/logical/applyparallelworker.c
> > b/src/backend/replication/logical/applyparallelworker.c
> > index 97f4a3037c..02bb608188 100644
> > --- a/src/backend/replication/logical/applyparallelworker.c
> > +++ b/src/backend/replication/logical/applyparallelworker.c
> > @@ -9,11 +9,10 @@
> > *
> > * This file contains the code to launch, set up, and teardown a parallel apply
> > * worker which receives the changes from the leader worker and
> > invokes routines
> > - * to apply those on the subscriber database.
> > - *
> > - * This file contains routines that are intended to support setting
> > up, using
> > - * and tearing down a ParallelApplyWorkerInfo which is required so
> > the leader
> > - * worker and parallel apply workers can communicate with each other.
> > + * to apply those on the subscriber database. Additionally, this file
> > + contains
> > + * routines that are intended to support setting up, using, and
> > + tearing down a
> > + * ParallelApplyWorkerInfo which is required so the leader worker and
> > + parallel
> > + * apply workers can communicate with each other.
> > *
> > * The parallel apply workers are assigned (if available) as soon as xact's
> > * first stream is received for subscriptions that have set their 'streaming'
>
> Merged.
>
> Besides, I also did the following changes:
> 1. Added maybe_reread_subscription_info in leader before assigning the
> transaction to parallel apply worker (Sawada-san's comments[1]) 2. Removed
> the "out of parallel apply workers" LOG ( Sawada-san's comments[1]) 3.
> Improved a elog message (Sawada-san's comments[1]).
> 4. Moved the testcases from 032_xx into existing 015_stream.pl which can save
> the initialization time. Since we introduced quite a few testcases in this patch set,
> so I did this to try to reduce the testing time that increased after applying these
> patches.

I noticed a CFbot failure in one of the new testcases in 015_stream.pl which
comes from old 032_xx.pl. It's because I slightly adjusted the change size in a
transaction in last version which cause the transaction's size not to exceed the
decoding work mem, so the transaction is not being applied as expected as
streaming transactions(it is applied as a non-stremaing transaction) which
cause the failure. Attach the new version patch which fixed this miss.

Best regards,
Hou zj