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.
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"
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.
And probably a test which will fail if the user only fetches the first
dead tuple?
4.
executor\execReplication.c(671) : warning C4700: uninitialized local
variable 'eq' used
Please fix this warning.
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.
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.
--
With Regards,
Amit Kapila.
