On Fri, Feb 9, 2024 at 9:57 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for patch v81-0001.
>
> ======
>
> 1. GENERAL - ReplicationSlotInvalidationCause enum.
>
> I was thinking that the ReplicationSlotInvalidationCause should
> explicitly set RS_INVAL_NONE = 0 (it's zero anyway, but making it
> explicit with a comment /* Must be zero. */. will stop it from being
> changed in the future).
>
> ------
> /*
> * Slots can be invalidated, e.g. due to max_slot_wal_keep_size. If so, the
> * 'invalidated' field is set to a value other than _NONE.
> */
> typedef enum ReplicationSlotInvalidationCause
> {
> RS_INVAL_NONE = 0, /* Must be zero. */
> ...
> } ReplicationSlotInvalidationCause;
> ------
>
> The reason to do this is because many places in the patch check for
> RS_INVAL_NONE, but if RS_INVAL_NONE == 0 is assured, all those code
> fragments can be simplified and IMO also become more readable.
>
> e.g. update_local_synced_slot()
>
> BEFORE
> Assert(slot->data.invalidated == RS_INVAL_NONE);
>
> AFTER
> Assert(!slot->data.invalidated);
>
I find the current code style more intuitive.
>
> 5. synchronize_slots
>
> + /*
> + * The primary_slot_name is not set yet or WALs not received yet.
> + * Synchronization is not possible if the walreceiver is not started.
> + */
> + latestWalEnd = GetWalRcvLatestWalEnd();
> + SpinLockAcquire(&WalRcv->mutex);
> + if ((WalRcv->slotname[0] == '\0') ||
> + XLogRecPtrIsInvalid(latestWalEnd))
> + {
> + SpinLockRelease(&WalRcv->mutex);
> + return;
> + }
> + SpinLockRelease(&WalRcv->mutex);
>
> The comment talks about the GUC "primary_slot_name", but the code is
> checking the WalRcv's slotname. It may be the same, but the difference
> is confusing.
>
Yeah, in this case, it would be the same because we don't allow slot
sync worker unless primary_slot_name is configured in which case
WalRcv->slotname refers to primary_slot_name. However, I think it is
better to explain here why slot synchronization is not possible or
doesn't make sense till walreceiver starts streaming and in which
case, won't it be sufficient to just check latestWalEnd?
--
With Regards,
Amit Kapila.
