Dear Hou,
Thanks for updating the patch! Here are my comments.
My comments do not take care which file contains the change, and the ordering may
be random.
1.
```
+ and <link
linkend="sql-createsubscription-params-with-detect-update-deleted"><literal>detect_conflict</literal></link>
+ are enabled.
```
"detect_conflict" still exists, it should be "detect_update_deleted".
2. maybe_advance_nonremovable_xid
```
+ /* Send a wal position request message to the server */
+ walrcv_send(LogRepWorkerWalRcvConn, "x", sizeof(uint8))
```
I think the character is used for PoC purpose, so it's about time we change it.
How about:
- 'W', because it requests the WAL location, or
- 'S', because it is accosiated with 's' message.
3. maybe_advance_nonremovable_xid
```
+ if (!AllTablesyncsReady())
+ return;
```
If we do not update oldest_nonremovable_xid during the sync, why do we send
the status message? I feel we can return in any phases if !AllTablesyncsReady().
4. advance_conflict_slot_xmin
```
+ ReplicationSlotCreate(CONFLICT_DETECTION_SLOT, false,
+ RS_PERSISTENT, false, false, false);
```
Hmm. You said the slot would be logical, but now it is physical. Which is correct?
5. advance_conflict_slot_xmin
```
+ xmin_horizon = GetOldestSafeDecodingTransactionId(true);
```
Since the slot won't do the logical decoding, we do not have to use oldest-safe-decoding
xid. I feel it is OK to use the latest xid.
6. advance_conflict_slot_xmin
```
+ /* No need to update xmin if the slot has been invalidated */
+ if (MyReplicationSlot->data.invalidated != RS_INVAL_NONE)
```
I feel the slot won't be invalidated. According to
InvalidatePossiblyObsoleteSlot(), the physical slot cannot be invalidated if it
has invalid restart_lsn.
7. ApplyLauncherMain
```
+ retain_dead_tuples |= sub->detectupdatedeleted;
```
Can you tell me why it must be updated even if the sub is disabled?
8. ApplyLauncherMain
If the subscription which detect_update_deleted = true exists but wal_receiver_status_interval = 0,
the slot won't be advanced and dead tuple retains forever... is it right? Can we
avoid it anyway?
9. FindMostRecentlyDeletedTupleInfo
It looks for me that the scan does not use indexes even if exists, but I feel it should use.
Am I missing something or is there a reason?
[1]:
https://www.postgresql.org/message-id/OS0PR01MB5716E0A283D1B66954CDF5A694682%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Best regards,
Hayato Kuroda
FUJITSU LIMITED
