On Sun, Mar 3, 2024 at 5:17 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> ~~~
>
> 3.
> + <note>
> + <para>
> + Value <literal>*</literal> is not accepted as it is inappropriate to
> + block logical replication for physical slots that either lack
> + associated standbys or have standbys associated that are not enabled
> + for replication slot synchronization. (see
> + <xref linkend="logicaldecoding-replication-slots-synchronization"/>).
> + </para>
> + </note>
>
> Why does the document need to provide an excuse/reason for the rule?
> You could just say something like:
>
> SUGGESTION
> The slots must be named explicitly. For example, specifying wildcard
> values like <literal>*</literal> is not permitted.
>
I would like to document the reason somewhere, if not in docs, then
let's write a comment for the same in code.
> ======
>
> ~~~
>
> 9. NeedToWaitForWal
>
> + /*
> + * Check if the standby slots have caught up to the flushed position. It
> + * is good to wait up to flushed position and then let it send the changes
> + * to logical subscribers one by one which are already covered in flushed
> + * position without needing to wait on every change for standby
> + * confirmation. Note that after receiving the shutdown signal, an ERROR
> + * is reported if any slots are dropped, invalidated, or inactive. This
> + * measure is taken to prevent the walsender from waiting indefinitely.
> + */
> + if (NeedToWaitForStandby(target_lsn, flushed_lsn, wait_event))
> + return true;
>
> I felt it was confusing things for this function to also call to the
> other one -- it seems an overlapping/muddling of the purpose of these.
> I think it will be easier to understand if the calling code just calls
> to one or both of these functions as required.
>
I felt otherwise because the caller has to call these functions at
more than one place which makes the caller's code difficult to follow.
It is better to encapsulate the computation of wait_event.
--
With Regards,
Amit Kapila.
