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
