On Tues, Sep 20, 2022 at 11:41 AM Shi, Yu/侍 雨 <shiy.fnst@cn.fujitsu.com> wrote:
> On Mon, Sept 19, 2022 11:26 AM Wang, Wei/王 威 <wangw.fnst@fujitsu.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
