Re: Synchronizing slots from primary to standby

On Tue, Feb 20, 2024 at 6:19 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> Thank you for the explanation. It makes sense to me to move the check.
>
> As for ValidateSlotSyncParams() called by SlotSyncWorkerAllowed(), I
> have two comments:
>
> 1. The error messages are not very descriptive and seem not to match
> other messages the postmaster says. When starting the standby server
> with misconfiguration about the slotsync, I got the following messages
> from the postmaster:
>
> 2024-02-20 17:01:16.356 JST [456741] LOG: database system is ready to
> accept read-only connections
> 2024-02-20 17:01:16.358 JST [456741] LOG: bad configuration for slot
> synchronization
> 2024-02-20 17:01:16.358 JST [456741] HINT: "hot_standby_feedback"
> must be enabled.
>
> It says "bad configuration" but is still working, and does not say
> further information such as whether it skipped to start the slotsync
> worker etc. I think these messages could work for the slotsync worker
> but we might want to have more descriptive messages for the
> postmaster. For example, "skipped starting slot sync worker because
> hot_standby_feedback is disabled".
>

We are planning to change it to something like:"slot synchronization
requires hot_standby_feedback to be enabled". See [1]

> 2. If the wal_level is not logical, the server will need to restart
> anyway to change the wal_level and have the slotsync worker work. Does
> it make sense to have the postmaster exit if the wal_level is not
> logical and sync_replication_slots is enabled? For instance, we have
> similar checks in PostmsaterMain():
>
> if (summarize_wal && wal_level == WAL_LEVEL_MINIMAL)
> ereport(ERROR,
> (errmsg("WAL cannot be summarized when wal_level is
> \"minimal\"")));
>

+1. I think giving an error in this case makes sense.

Miscellaneous comments:
========================
1.
+void
+ShutDownSlotSync(void)
+{
+ SpinLockAcquire(&SlotSyncCtx->mutex);
+
+ SlotSyncCtx->stopSignaled = true;

This flag is never reset back. I think we should reset this once the
promotion is complete. Though offhand, I don't see any problem with
this but it doesn't look clean and can be a source of bugs in the
future.

2.
+char *
+CheckDbnameInConninfo(void)
{
char *dbname;

Let's name this function as CheckAndGetDbnameFromConninfo().

Apart from the above, I have made cosmetic changes in the attached.

[1] - https://www.postgresql.org/message-id/CAJpy0uBWomyAjP0zyFdzhGxn%2BXsAb2OdJA%2BKfNyZRv2nV6PD9g%40mail.gmail.com

--
With Regards,
Amit Kapila.