Dear Shlok,
Thanks for creating the patch. Personally I prefer approach2; approach1 cannot
indicate the current status of synchronization, it just shows the history.
I feel approach2 has more information than approach1.
Below are my comments for your patch.
01.
```
+ Number of times the slot sync is skipped.
```
"slot sync" should be "slot synchronization". Same thing can be saied for other
attributes.
02.
```
+ Reason of the last slot sync skip.
```
Possible values must be clarified.
03.
```
+ s.slot_sync_skip_count,
+ s.last_slot_sync_skip,
+ s.slot_sync_skip_reason,
```
Last line has tab-blank but others have space-blank.
04.
```
+typedef enum SlotSyncSkipReason
+{
+ SLOT_SYNC_SKIP_NONE, /* No skip */
+ SLOT_SYNC_SKIP_STANDBY_BEHIND, /* Standby is behind the remote slot */
+ SLOT_SYNC_SKIP_REMOTE_BEHIND, /* Remote slot is behind the local slot
*/
+ SLOT_SYNC_SKIP_NO_CONSISTENT_SNAPSHOT /* Standby could not reach a
+
* consistent snapshot */
+} SlotSyncSkipReason
```
a.
Can we add comment atop the enum?
b.
SLOT_SYNC_SKIP_STANDBY_BEHIND is misleading; it indicates the standby server has
not received WALs required by slots, but enum does not clarify it.
How about SLOT_SYNC_SKIP_MISSING_WAL_RECORDS or something? Better naming are
very
welcome.
c
s/reach/build/.
05.
```
@@ -646,11 +652,13 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid
remote_dbid)
remote_slot->name,
LSN_FORMAT_ARGS(latestFlushPtr)));
- return false;
+ /* If the slot is not present on the local */
+ if (!(slot = SearchNamedReplicationSlot(remote_slot->name,
true)))
+ return false;
}
```
Looks like if users try to sync slots via SQL interfaces, the statistics cannot
be updated.
06. update_slot_sync_skip_stats
```
+ /* Update the slot sync reason */
+ SpinLockAcquire(&slot->mutex);
+ slot->slot_sync_skip_reason = skip_reason;
+ SpinLockRelease(&slot->mutex);
```
I feel no need to update the reason if the slot->slot_sync_skip_reason
and skip_reason are the same.
07. synchronize_one_slot
```
+ /*
+ * If standby is behind remote slot and the synced slot is
present on
+ * local.
+ */
+ if (remote_slot->confirmed_lsn > latestFlushPtr)
+ {
+ if (synced)
+ update_slot_sync_skip_stats(slot, skip_reason);
+ return false;
+ }
```
This condition exist in the same function; can we combine?
08. GetSlotSyncSkipReason()
Do we have to do pstrdup() here? I found a similar function
get_snapbuild_state_desc(),
and it does not use.
09.
Can you consider a test for added codes?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
