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

On Tues, Sep 20, 2022 at 11:41 AM Shi, Yu/侍 雨 <shiy(dot)fnst(at)cn(dot)fujitsu(dot)com> wrote:
> On Mon, Sept 19, 2022 11:26 AM Wang, Wei/王 威 <wangw(dot)fnst(at)fujitsu(dot)com>
> wrote:
> >
> >
> > Improved as suggested.
> >
>
> Thanks for updating the patch. Here are some comments on 0001 patch.

Thanks for your comments.

> 1.
> + case TRANS_LEADER_SERIALIZE:
>
> - oldctx = MemoryContextSwitchTo(ApplyContext);
> + /*
> + * Notify handle methods we're processing a remote in-
> progress
> + * transaction.
> + */
> + in_streamed_transaction = true;
>
> - MyLogicalRepWorker->stream_fileset = palloc(sizeof(FileSet));
> - FileSetInit(MyLogicalRepWorker->stream_fileset);
> + /*
> + * Since no parallel apply worker is used for the first
> stream
> + * start, serialize all the changes of the transaction.
> + *
> + * Start a transaction on stream start, this transaction will
> be
>
>
> It seems that the following comment can be removed after using switch case.
> + * Since no parallel apply worker is used for the first
> stream
> + * start, serialize all the changes of the transaction.

Removed.

> 2.
> + switch (apply_action)
> + {
> + case TRANS_LEADER_SERIALIZE:
> + if (!in_streamed_transaction)
> + ereport(ERROR,
> +
> (errcode(ERRCODE_PROTOCOL_VIOLATION),
> + errmsg_internal("STREAM STOP
> message without STREAM START")));
>
> In apply_handle_stream_stop(), I think we can move this check to the beginning
> of
> this function, to be consistent to other functions.

Improved as suggested.

> 3. I think the some of the changes in 0005 patch can be merged to 0001 patch,
> 0005 patch can only contain the changes about new column 'apply_leader_pid'.

Merged changes not related to 'apply_leader_pid' into patch 0001.

> 4.
> + * ParallelApplyWorkersList. After successfully, launching a new worker it's
> + * information is added to the ParallelApplyWorkersList. Once the worker
>
> Should `it's` be `its` ?

Fixed.

Attach the new patch set.

Regards,
Wang wei