On Monday, July 28, 2025 5:54 PM Amit Kapila <[email protected]> wrote:
>
> On Fri, Jul 25, 2025 at 4:38 PM Zhijie Hou (Fujitsu) <[email protected]>
> 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
