RE: Conflict detection for update_deleted in logical replication

On Monday, July 28, 2025 5:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Fri, Jul 25, 2025 at 4:38 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> > Right, I think it makes sense to do with the index scan when the
> > index's xmin is less than the conflict detection xmin, as that can
> > ensure that all the tuples deleted before the index creation or
> > re-indexing are irrelevant for conflict detection.
> >
> > I have implemented in the V53 patch set and improved the test to
> > verify both index and seq scan for dead tuples.
> >
> 
> Thanks. Following are a few comments on 0001 patch:
> 
> 1.
> --- a/src/backend/catalog/system_views.sql
> +++ b/src/backend/catalog/system_views.sql
> @@ -1397,6 +1397,7 @@ CREATE VIEW pg_stat_subscription_stats AS
>          ss.apply_error_count,
>          ss.sync_error_count,
>          ss.confl_insert_exists,
> +        ss.confl_update_deleted,
> …
> Datum
>  pg_stat_get_subscription_stats(PG_FUNCTION_ARGS)
>  {
> -#define PG_STAT_GET_SUBSCRIPTION_STATS_COLS 11
> +#define PG_STAT_GET_SUBSCRIPTION_STATS_COLS 12
> 
> Can we consider splitting stats into a separate patch? It will help us to first
> focus on core functionality of detecting update_delete conflict.

OK, splitted.

> 
> 2.
> While this approach may be slow on large tables,
> + * it is considered acceptable because it is only used in rare conflict
> + cases
> + * where the target row for an update cannot be found.
> 
> Here we should add at end "and no usable index is found"

Added.

> 
> 3.
> + * We scan all matching dead tuples in the relation to find the most
> + recently
> + * deleted one, rather than stopping at the first match. This is
> + because only
> + * the latest deletion information is relevant for resolving conflicts.
> + * Returning solely the first, potentially outdated tuple can lead
> + users to
> + * mistakenly apply remote changes using a last-update-win strategy,
> even when a
> + * more recent deleted tuple is available. See comments atop worker.c
> + for
> + * details.
> 
> I think we can share a short example of cases when this can happen.

Added the comments.

> And probably a test which will fail if the user only fetches the first dead tuple?

I am still thinking how to write a test that can distinguish two different
dead tuples and will add in next version if possible.

> 
> 4.
> executor\execReplication.c(671) : warning C4700: uninitialized local variable
> 'eq' used
> 
> Please fix this warning.

Fixed. Sorry for the miss.

> 
> 5.
> + /* Build scan key. */
> + skey_attoff = build_replindex_scan_key(skey, rel, idxrel, searchslot);
> +
> + /* Start an index scan. */
> + scan = index_beginscan(rel, idxrel, SnapshotAny, NULL, skey_attoff,
> + 0);
> 
> While scanning with SnapshotAny, isn't it possible that we find some tuple for
> which the xact is still not committed or are inserted successfully just before the
> scan is started?
> 
> I think such tuples shouldn't be considered for giving update_deleted.
> It seems the patch will handle it later during
> update_recent_dead_tuple_info() where it uses following check: "if
> (HeapTupleSatisfiesVacuum(tuple, oldestxmin, buf) ==
> HEAPTUPLE_RECENTLY_DEAD)", is my understanding correct? If so, we
> should add some comments for it.

Added.

> 
> 6.
> FindRecentlyDeletedTupleInfoSeq()
> {
> …
> + /* Get the index column bitmap for tuples_equal */ indexbitmap =
> + RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_IDENTITY_KEY);
> +
> + /* fallback to PK if no replica identity */ if (!indexbitmap)
> + indexbitmap = RelationGetIndexAttrBitmap(rel,
> + INDEX_ATTR_BITMAP_PRIMARY_KEY);
> …
> ...
> + if (!tuples_equal(scanslot, searchslot, eq, indexbitmap)) continue;
> 
> We don't do any such thing in RelationFindReplTupleSeq(), so, if we do
> something differently here, it should be explained in the comments.

Added.

Best Regards,
Hou zj