On Tue, Dec 19, 2023 at 4:51 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
>
> ======
> doc/src/sgml/system-views.sgml
>
> 3.
> + <para>
> + The hot standby can have any of these sync_state values for the slots
> but
> + on a hot standby, the slots with state 'r' and 'i' can neither be used
> + for logical decoding nor dropped by the user.
> + The sync_state has no meaning on the primary server; the primary
> + sync_state value is default 'n' for all slots but may (if leftover
> + from a promoted standby) also be 'r'.
> + </para></entry>
>
> I still feel we are exposing too much useless information about the
> primary server values.
>
> Isn't it sufficient to just say "The sync_state values have no meaning
> on a primary server.", and not bother to mention what those
> meaningless values might be -- e.g. if they are meaningless then who
> cares what they are or how they got there?
>
I feel it would be good to mention somewhere that primary can have
slots in 'r' state, if not here, some other place.
>
> 7.
> +/*
> + * Exit the slot sync worker with given exit-code.
> + */
> +static void
> +slotsync_worker_exit(const char *msg, int code)
> +{
> + ereport(LOG, errmsg("%s", msg));
> + proc_exit(code);
> +}
>
> This could be written differently (don't pass the exit code, instead
> pass a bool) like:
>
> static void
> slotsync_worker_exit(const char *msg, bool restart_worker)
>
> By doing it this way, you can keep the special exit code values (0,1)
> within this function where you can comment all about them instead of
> having scattered comments about exit codes in the callers.
>
> SUGGESTION
> ereport(LOG, errmsg("%s", msg));
> /* <some big comment here about how the code causes the worker to
> restart or not> */
> proc_exit(restart_worker ? 1 : 0);
>
Hmm, I don't see the need for this function in the first place. We can
use proc_exit in the two callers directly.
--
With Regards,
Amit Kapila.
