On Monday, September 5, 2022 8:41 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
>
> On Friday, September 2, 2022 2:10 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Thu, Sep 1, 2022 at 4:53 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> >
> > Review of v27-0001*:
>
> Thanks for the comments.
>
> > ================
> > 1. I feel the usage of in_remote_transaction and in_use flags is slightly complex.
> > IIUC, the patch uses in_use flag to ensure commit ordering by waiting
> > for it to become false before proceeding in transaction finish
> > commands in leader apply worker. If so, I think it is better to name
> > it in_parallel_apply_xact and set it to true only when we start
> > applying xact in parallel apply worker and set it to false when we
> > finish the xact in parallel apply worker. It can be initialized to
> > false while setting up DSM. Also, accordingly change the function
> > parallel_apply_wait_for_free() to parallel_apply_wait_for_xact_finish
> > and parallel_apply_set_idle to parallel_apply_set_xact_finish. We can
> > change the name of the in_remote_transaction flag to in_use.
>
> Agreed. One thing I found when addressing this is that there could be a race
> condition if we want to set the flag in parallel apply worker:
>
> where the leader has already started waiting for the parallel apply worker to
> finish processing the transaction(set the in_parallel_apply_xact to false) while the
> child process has not yet processed the first STREAM_START and has not set the
> in_parallel_apply_xact to true.
Sorry, I didn’t complete this sentence. I meant it's safer to set this flag in apply leader,
So I changed the code like that and added some comments to explain the same.
...
>
> Attach the new version patch set which addressed above comments and also
> fixed another problem while subscriber to a low version publisher.
Attach the correct patch set this time.
Best regards,
Hou zj
