RE: Conflict detection for update_deleted in logical replication

On Wednesday, October 2, 2024 2:34 PM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> wrote:
> 
> 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.

Thanks for the comments!

> 
> 1.
> ```
> +       and <link
> linkend="sql-createsubscription-params-with-detect-update-deleted"><lite
> ral>detect_conflict</literal></link>
> +       are enabled.
> ```
> "detect_conflict" still exists, it should be "detect_update_deleted".

Fixed.

> 
> 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.

Thanks for the suggestions. I preferred 'W'.

> 
> 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().

It's possible the table sync start before sending the request and stop before
entering DTR_WAIT_FOR_LOCAL_FLUSH phase, in which case we would be able to
advance xid immediately. So, I think there is no harm to send the request when
table sync is in progress and it could increase the possibility of advancing
the xid.


> 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?
 
The statement (slot would be logical) was true for the original design, which
is to let each apply worker maintain a different replication slot. But now
since we have decided to have the launcher maintain a single slot, it would be
unnecessary to make it logical one.

> 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.

I think using latest xid(nextXid) means that we would ignore all the running transactions.
E.g. All the dead tuples deleted by current running transactions will not be
retained. Since the apply worker chooses the oldest running xid as the
non-removable xid, so it's natrual for the launcher to be consistent by taking
the running transactions into account(use the lowest xid not affected by vacuum
as the initial xmin).

> 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.
 
I was considering it as a sanity check. And it seems possible for user to drop
this slot and create a slot with the same name, in which case we may face the
invalidation and it maybe helpful to report some message in this case.

But after thinking again, it seems better to just disallow user from using
this reserved replication slot name and remove this check, so changed like
that.

> 
> 7. ApplyLauncherMain
> ```
> +            retain_dead_tuples |= sub->detectupdatedeleted;
> ```
> Can you tell me why it must be updated even if the sub is disabled?

I think we won't update the xmin if all the subscriptions are disabled.
Because the launcher cannot get a valid xmin to update.

But when checking this part, I found a bug where it could update the xmin
When some of the subscriptions are disabled, and fixed it in this version.

> 
> 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?

I think the same is true for the hot_standby_feedback feature. We can try to
avoid it but not sure if it's worth it. Anyway, user have options to either adjust
wal_receiver_status_interval or disable detect_update_deleted in this case.

> 
> 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?

I missed that it's possible to use the index to fetch dead tuples. I have added it
In this version. Thanks a lot to Kuroda-san for contributing codes off-list.

> 
> [1]:
> https://www.postgresql.org/message-id/OS0PR01MB5716E0A283D1B66954
> CDF5A694682%40OS0PR01MB5716.jpnprd01.prod.outlook.com


Attach the V4 patch set which addressed above comments.

Best Regards,
Hou zj