On Friday, February 9, 2024 12:27 PM Peter Smith <smithpb2...@gmail.com> wrote: > > Here are some review comments for patch v81-0001.
Thanks for the comments. > . 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). I think the current code is better, so didn't change this. > 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. This part has been removed. > 7. > + /* > + * Use shared lock to prevent a conflict with > + * ReplicationSlotsDropDBSlots(), trying to drop the same slot during > + * a drop-database operation. > + */ > + LockSharedObject(DatabaseRelationId, remote_dbid, 0, AccessShareLock); > + > + synchronize_one_slot(remote_slot, remote_dbid); > + > + UnlockSharedObject(DatabaseRelationId, remote_dbid, 0, > + AccessShareLock); > > IMO remove the blank lines (e.g., you don't use this kind of formatting for > spin > locks) I am not sure if it will look better, so didn't change this. Other comments look good. Best Regards, Hou zj