RE: Conflict detection for update_deleted in logical replication

On Wednesday, October 30, 2024 4:34 PM Kuroda, Hayato/黒田 隼人 <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Thanks for updating the patch! Here are my comments.

Thanks for giving comments !

>
> 01. CreateSubscription
> ```
> + if (opts.detectupdatedeleted && !track_commit_timestamp)
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("detecting update_deleted conflicts requires \"%s\"
> to be enabled",
> + "track_commit_timestamp"));
> ```
>
> I don't think this guard is sufficient. I found two cases:
>
> * Creates a subscription with detect_update_deleted = false and
> track_commit_timestamp = false,
> then alters detect_update_deleted to true.

I think this is not allowed, as we already have a check in AlterSubscription().

> * Creates a subscription with detect_update_deleted = true and
> track_commit_timestamp = true,
> then update track_commit_timestamp to true and restart the instance.
>
> Based on that, how about detecting the inconsistency on the apply worker? It
> check
> the parameters and do error out when it starts or re-reads a catalog.

I agree to add the check in apply worker. But I would prefer to keep the check in
DDL as well, as I think It's better to prevent the bad configuration in the
beginning.

> If we want
> to detect in SQL commands, this can do in parse_subscription_options().

I personally feel parse_subscription_options() is not an appropriate place for this kind of
configuration error. So, I prefer to keep the check in CreateSubscription and
AlterSubscription.

> 03. build_index_column_bitmap()
> ```
> + for (int i = 0; i < indexinfo->ii_NumIndexAttrs; i++)
> + {
> + int keycol = indexinfo->ii_IndexAttrNumbers[i];
> +
> + index_bitmap = bms_add_member(index_bitmap, keycol);
> + }
> ```
>
> I feel we can assert the ii_IndexAttrNumbers is valid, because the passed index
> is a replica identity key.

I changed to directly get the replica identity index in the
FindMostRecentlyDeletedTupleInfo instead of passing by parameter. After that, I
think the assert is not necessary as the code is straightforward.

>
> 05. LogicalRepApplyLoop()
> ```
> + UpdateWorkerStats(last_received, timestamp,
> false);
> ```
>
> The statistics seems not correct. last_received is not sent at "timestamp", it
> had
> already been sent earlier. Do we really have to update here?

The timestamp updated here is used in the view
pg_stat_subscription.last_msg_send_time, which is a general timestamp that is
not directly related to the LSN received. See the desc in doc:

"""
last_msg_send_time

Send time of last message received from origin WAL sender; NULL for
parallel apply workers
"""

So, I think it's correct to update the timestamp here while keep last received lsn
unchanged.

>
> 06. ErrorOnReservedSlotName()
>
> I feel we should document that the slot name 'pg_conflict_detection' cannot be
> specified
> unconditionally.

I am a bit not sure if it's necessary, as the error message is clear that this
is a reserved name. The doc of the detect_update_delete also mentioned the name
of this slot.

When checking other options, we have reserved 'replication origin' name as well
"NONE,ANY" which are not explicitly mentioned in the doc. So, it seems fine.

>
> 07. General
>
> update_deleted can happen without DELETE commands. Should we rename
> the conflict
> reason, like 'update_target_modified'?

I am not sure if 'update_target_modified' is appropriate, as it's too similar to
the existing 'update_origin_change' conflict ("Updating a row that was
previously modified by another origin.").

Since the UPDATE can also be considered as DELETE + INSERT, so the current name
looks fine to me.

Attach the V7 patch set which address all other comments not mentioned.

Best Regards,
Hou zj