On Mon, Sept 19, 2022 11:26 AM Wang, Wei/王 威 <wangw.f...@fujitsu.com> wrote:
> 
> 
> Improved as suggested.
> 

Thanks for updating the patch. Here are some comments on 0001 patch.

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.

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.

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'.

4.
+ * ParallelApplyWorkersList. After successfully, launching a new worker it's
+ * information is added to the ParallelApplyWorkersList. Once the worker

Should `it's` be `its` ?

Regards
Shi yu

Reply via email to