RE: Synchronizing slots from primary to standby

On Monday, March 4, 2024 9:55 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Sun, Mar 3, 2024 at 6:51 PM Zhijie Hou (Fujitsu) <houzj(dot)fnst(at)fujitsu(dot)com>
> wrote:
> >
> > On Sunday, March 3, 2024 7:47 AM Peter Smith <smithpb2250(at)gmail(dot)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.
> >
> > As suggested by Amit, I moved this to code comments.
>
> Was the total removal of this note deliberate? I only suggested removing the
> *reason* for the rule, not the entire rule. Otherwise, the user won't know to
> avoid doing this until they try it and get an error.

OK, Added.

>
>
> > >
> > > 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.
> >
> > Same as Amit, I didn't change this.
>
> AFAICT my previous review comment was misinterpreted. Please see [1] for
> more details.
>
> ~~~~
>
> Here are some more review comments for v104-00001

Thanks!

>
> ======
> Commit message
>
> 1.
> Additionally, The SQL functions pg_logical_slot_get_changes,
> pg_logical_slot_peek_changes and pg_replication_slot_advance are modified
> to wait for the replication slots specified in 'standby_slot_names' to catch up
> before returning.
>
> ~
>
> Maybe that should be expressed using similar wording as the docs...
>
> SUGGESTION
> Additionally, The SQL functions ... are modified. Now, when used with
> failover-enabled logical slots, these functions will block until all physical slots
> specified in 'standby_slot_names' have confirmed WAL receipt.

Changed.

>
> ======
> doc/src/sgml/config.sgml
>
> 2.
> + <function>pg_logical_slot_peek_changes</function></link>,
> + when used with failover enabled logical slots, will block until all
> + physical slots specified in
> <varname>standby_slot_names</varname> have
> + confirmed WAL receipt.
>
> /failover enabled logical slots/failover-enabled logical slots/

Changed. Note that for this comment and remaining comments,
I used the later version we agreed(logical failover slot).

>
> ======
> doc/src/sgml/func.sgml
>
> 3.
> + The function may be blocked if the specified slot is a failover enabled
> + slot and <link
> linkend="guc-standby-slot-names"><varname>standby_slot_names</varna
> me></link>
> + is configured.
> </para></entry>
>
> /a failover enabled slot//a failover-enabled slot/

Changed.

>
> ~~~
>
> 4.
> + slot may return to an earlier position. The function may be blocked if
> + the specified slot is a failover enabled slot and
> + <link
> linkend="guc-standby-slot-names"><varname>standby_slot_names</varna
> me></link>
> + is configured.
>
> /a failover enabled slot//a failover-enabled slot/

Changed.

>
> ======
> src/backend/replication/slot.c
>
> 5.
> +/*
> + * Wait for physical standbys to confirm receiving the given lsn.
> + *
> + * Used by logical decoding SQL functions that acquired failover enabled slot.
> + * It waits for physical standbys corresponding to the physical slots
> +specified
> + * in the standby_slot_names GUC.
> + */
> +void
> +WaitForStandbyConfirmation(XLogRecPtr wait_for_lsn)
>
> /failover enabled slot/failover-enabled slot/

Changed.

>
> ~~~
>
> 6.
> + /*
> + * Don't need to wait for the standby to catch up if the current
> + acquired
> + * slot is not a failover enabled slot, or there is no value in
> + * standby_slot_names.
> + */
>
> /failover enabled slot/failover-enabled slot/

Changed.

>
> ======
> src/backend/replication/slotfuncs.c
>
> 7.
> +
> + /*
> + * Wake up logical walsenders holding failover enabled slots after
> + * updating the restart_lsn of the physical slot.
> + */
> + PhysicalWakeupLogicalWalSnd();
>
> /failover enabled slots/failover-enabled slots/

Changed.

>
> ======
> src/backend/replication/walsender.c
>
> 8.
> +/*
> + * Wake up the logical walsender processes with failover enabled slots
> +if the
> + * currently acquired physical slot is specified in standby_slot_names GUC.
> + */
> +void
> +PhysicalWakeupLogicalWalSnd(void)
>
> /failover enabled slots/failover-enabled slots/

Changed.

>
> 9.
> +/*
> + * Returns true if not all standbys have caught up to the flushed
> +position
> + * (flushed_lsn) when the current acquired slot is a failover enabled
> +logical
> + * slot and we are streaming; otherwise, returns false.
> + *
> + * If returning true, the function sets the appropriate wait event in
> + * wait_event; otherwise, wait_event is set to 0.
> + */
> +static bool
> +NeedToWaitForStandby(XLogRecPtr target_lsn, XLogRecPtr flushed_lsn,
> + uint32 *wait_event)
>
> 9a.
> /failover enabled logical slot/failover-enabled logical slot/

Changed.

>
> ~
>
> 9b.
> Probably that function name should be plural.
>
> /NeedToWaitForStandby/NeedToWaitForStandbys/
>
> ~~~
>
> 10.
> +/*
> + * Returns true if we need to wait for WALs to be flushed to disk, or
> +if not
> + * all standbys have caught up to the flushed position (flushed_lsn)
> +when the
> + * current acquired slot is a failover enabled logical slot and we are
> + * streaming; otherwise, returns false.
> + *
> + * If returning true, the function sets the appropriate wait event in
> + * wait_event; otherwise, wait_event is set to 0.
> + */
> +static bool
> +NeedToWaitForWal(XLogRecPtr target_lsn, XLogRecPtr flushed_lsn,
> + uint32 *wait_event)
>
> /failover enabled logical slot/failover-enabled logical slot/

Changed.

>
> ~~~
>
> 11. WalSndWaitForWal
>
> + /*
> + * Within the loop, we wait for the necessary WALs to be flushed to
> + * disk first, followed by waiting for standbys to catch up if there
> + * are enought WALs or upon receiving the shutdown signal. To avoid
> + * the scenario where standbys need to catch up to a newer WAL
> + * location in each iteration, we update our idea of the currently
> + * flushed position only if we are not waiting for standbys to catch
> + * up.
> + */
>
> typo
>
> /enought/enough/

Fixed.

Best Regards,
Hou zj