On Wed, Jul 19, 2023 at 8:38 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Some review comments for v19-0001
>
...
> ======
> src/backend/replication/logical/worker.c
>
> 3. set_stream_options
>
> +/*
> + * Sets streaming options including replication slot name and origin start
> + * position. Workers need these options for logical replication.
> + */
> +void
> +set_stream_options(WalRcvStreamOptions *options,
>
> I'm not sure if the last sentence of the comment is adding anything useful.
>
Personally, I find it useful as at a high-level it tells the purpose
of setting these options.
> ~~~
>
> 4. start_apply
> /*
> * Run the apply loop with error handling. Disable the subscription,
> * if necessary.
> *
> * Note that we don't handle FATAL errors which are probably because
> * of system resource error and are not repeatable.
> */
> void
> start_apply(XLogRecPtr origin_startpos)
>
> ~
>
> 4a.
> Somehow I found the function names to be confusing. Intuitively (IMO)
> 'start_apply' is for apply worker and 'start_tablesync' is for
> tablesync worker. But actually, the start_apply() function is the
> *common* function for both kinds of worker. Might be easier to
> understand if start_apply function name can be changed to indicate it
> is really common -- e.g. common_apply_loop(), or similar.
>
> ~
>
> 4b.
> If adverse to changing the function name, it might be helpful anyway
> if the function comment can emphasize this function is shared by
> different worker types. e.g. "Common function to run the apply
> loop..."
>
I would prefer to change the comments as suggested by you in 4b
because both the workers (apply and tablesync) need to perform apply,
so it seems logical for both of them to invoke start_apply.
--
With Regards,
Amit Kapila.