On Mon, Sep 12, 2022 at 4:27 PM kuroda.hayato@fujitsu.com
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Hou-san,
>
> Thank you for updating the patch! Followings are comments for v28-0001.
> I will dig your patch more, but I send partially to keep the activity of the thread.
>
> ===
> For applyparallelworker.c
>
> 01. filename
> The word-ordering of filename seems not good
> because you defined the new worker as "parallel apply worker".
>
I think in the future we may have more files for apply work (like
applyddl.c for DDL apply work), so it seems okay to name all apply
related files in a similar way.
>
> ===
> For worker.c
>
> 07. general
>
> In many lines if-else statement is used for apply_action, but I think they should rewrite as switch-case statement.
>
Sounds reasonable to me.
> 08. global variable
>
> ```
> -static bool in_streamed_transaction = false;
> +bool in_streamed_transaction = false;
> ```
>
> a.
>
> It seems that in_streamed_transaction is used only in the worker.c, so we can change to stati variable.
>
Yeah, I don't know why it has been changed in the first place.
> b.
>
> That flag is set only when an apply worker spill the transaction to the disk.
> How about "in_streamed_transaction" -> "in_spilled_transaction"?
>
Isn't this an existing variable? If so, it doesn't seem like a good
idea to change the name unless we are changing its meaning.
--
With Regards,
Amit Kapila.
