On Mon, Dec 11, 2023 at 1:02 PM Peter Smith <smithpb2...@gmail.com> wrote:
>
> Here are some review comments for v44-0001
>
> ~~~
>
> 3. assign_standby_slot_names
>
> + if (!SplitIdentifierString(standby_slot_names_cpy, ',', &standby_slots))
> + {
> + /* This should not happen if GUC checked check_standby_slot_names. */
> + elog(ERROR, "list syntax is invalid");
> + }
>
> This error here and in validate_standby_slots() are different --
> "list" versus "List".
>
Note here elog(ERROR,.. is used and in the other place it is part of
the detail message. I have suggested in my previous review to make
them the same but I overlooked the difference, so I think we should
change the message to "invalid list syntax" as it was there
previously.
> ======
> src/backend/replication/walsender.c
>
>
> 4. WalSndFilterStandbySlots
>
>
> + foreach(lc, standby_slots_cpy)
> + {
> + char *name = lfirst(lc);
> + XLogRecPtr restart_lsn = InvalidXLogRecPtr;
> + bool invalidated = false;
> + char *warningfmt = NULL;
> + ReplicationSlot *slot;
> +
> + slot = SearchNamedReplicationSlot(name, true);
> +
> + if (slot && SlotIsPhysical(slot))
> + {
> + SpinLockAcquire(&slot->mutex);
> + restart_lsn = slot->data.restart_lsn;
> + invalidated = slot->data.invalidated != RS_INVAL_NONE;
> + SpinLockRelease(&slot->mutex);
> + }
> +
> + /* Continue if the current slot hasn't caught up. */
> + if (!invalidated && !XLogRecPtrIsInvalid(restart_lsn) &&
> + restart_lsn < wait_for_lsn)
> + {
> + /* Log warning if no active_pid for this physical slot */
> + if (slot->active_pid == 0)
> + ereport(WARNING,
> + errmsg("replication slot \"%s\" specified in parameter \"%s\" does
> not have active_pid",
> + name, "standby_slot_names"),
> + errdetail("Logical replication is waiting on the "
> + "standby associated with \"%s\"", name),
> + errhint("Consider starting standby associated with "
> + "\"%s\" or amend standby_slot_names", name));
> +
> + continue;
> + }
> + else if (!slot)
> + {
> + /*
> + * It may happen that the slot specified in standby_slot_names GUC
> + * value is dropped, so let's skip over it.
> + */
> + warningfmt = _("replication slot \"%s\" specified in parameter
> \"%s\" does not exist, ignoring");
> + }
> + else if (SlotIsLogical(slot))
> + {
> + /*
> + * If a logical slot name is provided in standby_slot_names, issue
> + * a WARNING and skip it. Although logical slots are disallowed in
> + * the GUC check_hook(validate_standby_slots), it is still
> + * possible for a user to drop an existing physical slot and
> + * recreate a logical slot with the same name. Since it is
> + * harmless, a WARNING should be enough, no need to error-out.
> + */
> + warningfmt = _("cannot have logical replication slot \"%s\" in
> parameter \"%s\", ignoring");
> + }
> + else if (XLogRecPtrIsInvalid(restart_lsn) || invalidated)
> + {
> + /*
> + * Specified physical slot may have been invalidated, so there is no point
> + * in waiting for it.
> + */
> + warningfmt = _("physical slot \"%s\" specified in parameter \"%s\"
> has been invalidated, ignoring");
> + }
> + else
> + {
> + Assert(restart_lsn >= wait_for_lsn);
> + }
>
> This if/else chain seems structured awkwardly. IMO it would be tidier
> to eliminate the NULL slot and IsLogicalSlot up-front, which would
> also simplify some of the subsequent conditions
>
> SUGGESTION
>
> slot = SearchNamedReplicationSlot(name, true);
>
> if (!slot)
> {
> ...
> }
> else if (SlotIsLogical(slot))
> {
> ...
> }
> else
> {
> Assert(SlotIsPhysical(slot))
>
> SpinLockAcquire(&slot->mutex);
> restart_lsn = slot->data.restart_lsn;
> invalidated = slot->data.invalidated != RS_INVAL_NONE;
> SpinLockRelease(&slot->mutex);
>
> if (XLogRecPtrIsInvalid(restart_lsn) || invalidated)
> {
> ...
> }
> else if (!invalidated && !XLogRecPtrIsInvalid(restart_lsn) &&
> restart_lsn < wait_for_lsn)
> {
> ...
> }
> else
> {
> Assert(restart_lsn >= wait_for_lsn);
> }
> }
>
+1.
--
With Regards,
Amit Kapila.
