Fix WITHOUT OVERLAPS multirange with location replication

On Fri, Oct 9, 2026 at 9:50 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

Hi Hackers,

Andres reported that a WITHOUT OVERLAPS replica identity using
multiranges can update/delete the wrong row, because a recheck is
needed.[0] This patch fixes that problem by performing a recheck when
the scan indicates one is needed.

RelationFindReplTupleByIndex() and
RelationFindDeletedTupleInfoByIndex() skipped the tuples_equal() check
whenever the scan used the primary key or replica identity index,
assuming any match is exact. But a GiST index on a multirange is
lossy: two multiranges with the same bounding range, like {[1,5)} and
{[1,2),[3,5)}, look the same to the index. Now we recheck the
candidate whenever the scan sets xs_recheck, like
check_exclusion_or_unique_constraint() does. Btree and range-type GiST
indexes never set it, so they keep the fast path. When we do recheck,
we compare only the key columns, since those are all the search slot
has (and we only look up that bitmap if we need it).

The second function is used for conflict detection with
retain_dead_tuples. There the bug can't modify the wrong row, but it
can report update_deleted (blaming the transaction that deleted an
unrelated row) instead of update_missing.[1]

The patch adds a TAP test to 034_temporal.pl using two multiranges
with the same bounding range.

Hi Paul,

Thanks for the patch. I reviewed v1 and tested it manually. I was able
to reproduce both the wrong-row update/delete and the update_deleted
misreport (update_deleted reported instead of update_missing with
retain_dead_tuples) using a multirange key, and the patch fixes both.

I didn't find any critical issues. Just a few small suggestions:
1) The change to RelationFindDeletedTupleInfoByIndex() doesn't seem to
be covered by a test. Maybe we could add a case to 035_conflicts.pl?

2) The header comment of RelationFindReplTupleByIndex() needs an
update. It still says:

* 'skipduplicates' specifies whether the first matching tuple can be used
* without comparing it against 'searchslot'.
-- This no longer holds when the index scan requires a recheck.

3) The commit message could also mention the update_deleted vs.
update_missing impact, and that this patch fixes it.

--
Thanks,
Nisha

On Fri, Oct 9, 2026, Nisha Moond <nisha.moond412@gmail.com> wrote:

Thanks for the patch. I reviewed v1 and tested it manually. I was able
to reproduce both the wrong-row update/delete and the update_deleted
misreport (update_deleted reported instead of update_missing with
retain_dead_tuples) using a multirange key, and the patch fixes both.

Thanks for the review!

1) The change to RelationFindDeletedTupleInfoByIndex() doesn't seem to
be covered by a test. Maybe we could add a case to 035_conflicts.pl?

This was tricky, but I figured out a way to do it. My test adds a row
on the publisher before creating the subscription, then uses
copy_data=false. Then I add & delete a "decoy" row with a different
multirange but the same lossy bounding range. When the publisher
updates the first row, it gets update_deleted before the fix and
update_missing after.

2) The header comment of RelationFindReplTupleByIndex() needs an
update. It still says:

* 'skipduplicates' specifies whether the first matching tuple can be used
* without comparing it against 'searchslot'.
-- This no longer holds when the index scan requires a recheck.

Done.

3) The commit message could also mention the update_deleted vs.
update_missing impact, and that this patch fixes it.

Done.

v2 attached, rebased on current master.

Yours,

--
Paul ~{:-)
pj@illuminatedcomputing.com

v2 attached, rebased on current master.

Hi Paul,
Thank you for the patches.
I tested v2-0001 and have two results and one question.

1. Lossy REPLICA IDENTITY index (034_temporal.pl scenario)
I used a multirange WITHOUT OVERLAPS identity index with two rows
whose bounding ranges are the same.

Without the patch, the apply worker reports conflict=update_exists.
With the patch, it reports conflict=conflict=update_origin_differs.

2. update_missing vs update_deleted (035_conflicts.pl scenario)

Without the patch, the conflict is reported as update_deleted.
With the patch, it is reported as update_missing.
That matches what the test expects. t/034_temporal.pl and
t/035_conflicts.pl both pass for me.

Question about RelationFindReplTupleByIndex()
The scan uses the idxoid passed in. The recheck bitmap comes from
RelationGetIndexAttrBitmap(rel, ...), which describes the relation's
currently configured identity or PK index. This might not be the index
that was scanned.

worker.c (around line 3250) mentions that a concurrent index update
can change the index's validity or identity while the worker still
holds a cached index OID. If that happens, the bitmap may no longer
describe the scanned index. The recheck would then compare unrelated
columns, or all columns if no bitmap is available. The incoming
old-key tuple doesn't necessarily contain those other values. The real
candidate could then be rejected, which would give a missing-row
result or the wrong conflict classification. The same applies to
RelationFindDeletedTupleInfoByIndex().

I haven't reproduced this concurrency case yet. as much as I have
tried, ordinary DDL is not triggering it for a GiST WITHOUT OVERLAPS
index.

Would it make sense to build the recheck bitmap from the scanned
index's own key columns (idxrel->rd_index->indkey, up to indnkeyatts)?
That would remove the dependency on the relation's current identity.
Or do you think the current approach is safe for a reason I'm missing?

Thanks,
Kedar

On Fri, Oct 9, 2026 at 4:55 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

On Fri, Oct 9, 2026, Nisha Moond <nisha.moond412@gmail.com> wrote:

1) The change to RelationFindDeletedTupleInfoByIndex() doesn't seem to
be covered by a test. Maybe we could add a case to 035_conflicts.pl?

This was tricky, but I figured out a way to do it. My test adds a row
on the publisher before creating the subscription, then uses
copy_data=false. Then I add & delete a "decoy" row with a different
multirange but the same lossy bounding range. When the publisher
updates the first row, it gets update_deleted before the fix and
update_missing after.

I think this test is timing dependent because if Delete happens before
Update, the conflict slot could be updated and the row could be
removed. If it is too tricky/tough to write this test, we can even
skip it.

--
With Regards,
Amit Kapila.

On Sat, Oct 10, 2026 at 9:31 PM kedar anavardekar
<kedar.anavardekar@gmail.com> wrote:

v2 attached, rebased on current master.

Hi Paul,
Thank you for the patches.
I tested v2-0001 and have two results and one question.

1. Lossy REPLICA IDENTITY index (034_temporal.pl scenario)
I used a multirange WITHOUT OVERLAPS identity index with two rows
whose bounding ranges are the same.

Without the patch, the apply worker reports conflict=update_exists.
With the patch, it reports conflict=conflict=update_origin_differs.

2. update_missing vs update_deleted (035_conflicts.pl scenario)

Without the patch, the conflict is reported as update_deleted.
With the patch, it is reported as update_missing.
That matches what the test expects. t/034_temporal.pl and
t/035_conflicts.pl both pass for me.

Question about RelationFindReplTupleByIndex()
The scan uses the idxoid passed in. The recheck bitmap comes from
RelationGetIndexAttrBitmap(rel, ...), which describes the relation's
currently configured identity or PK index. This might not be the index
that was scanned.

worker.c (around line 3250) mentions that a concurrent index update
can change the index's validity or identity while the worker still
holds a cached index OID. If that happens, the bitmap may no longer
describe the scanned index. The recheck would then compare unrelated
columns, or all columns if no bitmap is available. The incoming
old-key tuple doesn't necessarily contain those other values. The real
candidate could then be rejected, which would give a missing-row
result or the wrong conflict classification. The same applies to
RelationFindDeletedTupleInfoByIndex().

I haven't reproduced this concurrency case yet. as much as I have
tried, ordinary DDL is not triggering it for a GiST WITHOUT OVERLAPS
index.

Would it make sense to build the recheck bitmap from the scanned
index's own key columns (idxrel->rd_index->indkey, up to indnkeyatts)?
That would remove the dependency on the relation's current identity.
Or do you think the current approach is safe for a reason I'm missing?

IIUC, the current approach is safe because concurrent operations won't
change index columns. Having said that, I think your suggested
approach sounds more robust. Additionally, I think we can build the
bitmap inside (if (eq == NULL)) check to avoid building it again and
again for each tuple scan.

--
With Regards,
Amit Kapila.

Hi,

On Sun, Oct 11, 2026 at 1:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Sat, Oct 10, 2026 at 9:31 PM kedar anavardekar
<kedar.anavardekar@gmail.com> wrote:

v2 attached, rebased on current master.

Would it make sense to build the recheck bitmap from the scanned
index's own key columns (idxrel->rd_index->indkey, up to indnkeyatts)?
That would remove the dependency on the relation's current identity.
Or do you think the current approach is safe for a reason I'm missing?

IIUC, the current approach is safe because concurrent operations won't
change index columns. Having said that, I think your suggested
approach sounds more robust. Additionally, I think we can build the
bitmap inside (if (eq == NULL)) check to avoid building it again and
again for each tuple scan.

I think that instead of building a bitmap and passing it to tuples_equal(), we
can write a new helper function that directly traverses the index columns in
the slot and compares each of them, similar to what identity_key_equal() does
in pg_repack. I think that's a more standard approach, because we can use the
index's opclass equality function and collation for the comparison rather than
reusing the table attributes'. It is also more efficient, since it avoids
building a new bitmap and avoids the typecache lookup. It may not show a
performance difference, but I feel it's worth doing better in a common code
path (update/delete replay) like this one.

And based on the above, testing further, I realized this can fix a real issue:
when the index's operator class and collation differ from the table's default
ones, the patch's code (and the code on HEAD) uses the table's pg_attribute
collation to compare the tuple, which is inconsistent with the index's. For the
WITHOUT OVERLAPS index, we need to create a new extension with a new operator
class to reproduce the issue (I did not find a way to use WITHOUT OVERLAPS
together with a index specific collation, otherwise setting a different index
collation can also reproduce it); see 0003 (generated with AI assistance),
which reproduces it that way (it fails with v2 and passes with v3). I don't
think we should commit this heavy test patch, so I'm just sharing it for
reference.

The same issue exists on HEAD as well for the update_deleted detection code
path. I tried writing a patch to replace the update_deleted code path first in
0001 (since it's an existing issue, I've kept it in a separate patch for now to
make it easier to review; we can merge the two patches and commit them together
later if we agree). 0001 includes a test for the collation inconsistency issue.

And I rebased the WITHOUT OVERLAPS fix on top of it, modifying it to use the
new approach for tuple comparison and slightly adjusting some commit messages
and comments.

I'm sharing both patches here since both touch the same code path
and overlap. Hope it helps move the thread forward.

Best Regards,
Zhijie Hou

Hi Zhijie Hou,
Thank you for the patches,
I built and tested the patches locally, and they work as expected.

I think that instead of building a bitmap and passing it to tuples_equal(), we
can write a new helper function that directly traverses the index columns in
the slot and compares each of them, similar to what identity_key_equal() does
in pg_repack. I think that's a more standard approach, because we can use the
index's opclass equality function and collation for the comparison rather than
reusing the table attributes'. It is also more efficient, since it avoids
building a new bitmap and avoids the typecache lookup. It may not show a
performance difference, but I feel it's worth doing better in a common code
path (update/delete replay) like this one.

+1

The same issue exists on HEAD as well for the update_deleted detection code
path. I tried writing a patch to replace the update_deleted code path first in
0001 (since it's an existing issue, I've kept it in a separate patch for now to
make it easier to review; we can merge the two patches and commit them together
later if we agree). 0001 includes a test for the collation inconsistency issue.

And I rebased the WITHOUT OVERLAPS fix on top of it, modifying it to use the
new approach for tuple comparison and slightly adjusting some commit messages
and comments.

I'm sharing both patches here since both touch the same code path
and overlap. Hope it helps move the thread forward.

v30001 +v30002 LGTM.

On Sun, Oct 11, 2026 at 8:58 PM Zhijie Hou <houzhijie22@gmail.com> wrote:

On Sun, Oct 11, 2026 at 1:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Sat, Oct 10, 2026 at 9:31 PM kedar anavardekar
<kedar.anavardekar@gmail.com> wrote:

v2 attached, rebased on current master.

Would it make sense to build the recheck bitmap from the scanned
index's own key columns (idxrel->rd_index->indkey, up to indnkeyatts)?
That would remove the dependency on the relation's current identity.
Or do you think the current approach is safe for a reason I'm missing?

IIUC, the current approach is safe because concurrent operations won't
change index columns. Having said that, I think your suggested
approach sounds more robust. Additionally, I think we can build the
bitmap inside (if (eq == NULL)) check to avoid building it again and
again for each tuple scan.

I think that instead of building a bitmap and passing it to tuples_equal(), we
can write a new helper function that directly traverses the index columns in
the slot and compares each of them, similar to what identity_key_equal() does
in pg_repack. I think that's a more standard approach, because we can use the
index's opclass equality function and collation for the comparison rather than
reusing the table attributes'. It is also more efficient, since it avoids
building a new bitmap and avoids the typecache lookup. It may not show a
performance difference, but I feel it's worth doing better in a common code
path (update/delete replay) like this one.

About 0001 patch:
The index is created after the conflict detection
+# slot's xmin is pinned, so the sequential scan is used; the locally deleted
+# row 'ABC' must still be reported as update_deleted for the publisher's key
+# 'abc', exactly as if the index had been used.

It is not clear to me why here the behaviour should be similar to
index scan? As index is created after the slot is pinned, so isn't it
okay to behave as there is no index at all? I see the point that for
live tuples scan we may have used index but for conflicts as the index
could be created after the tuple we are searching is deleted, we are
doing sequence scan and comparing just indexed columns and that could
be seen as inconsistent. As this is sort of a grey area and we are not
sure whether to change the code or not, I feel we can fix Paul's
reported issue now as that is a clear bug.

--
With Regards,
Amit Kapila.

Hi,

On Mon, Oct 12, 2026 at 10:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Sun, Oct 11, 2026 at 8:58 PM Zhijie Hou <houzhijie22@gmail.com> wrote:

I think that instead of building a bitmap and passing it to tuples_equal(), we
can write a new helper function that directly traverses the index columns in
the slot and compares each of them, similar to what identity_key_equal() does
in pg_repack. I think that's a more standard approach, because we can use the
index's opclass equality function and collation for the comparison rather than
reusing the table attributes'. It is also more efficient, since it avoids
building a new bitmap and avoids the typecache lookup. It may not show a
performance difference, but I feel it's worth doing better in a common code
path (update/delete replay) like this one.

About 0001 patch:
The index is created after the conflict detection
+# slot's xmin is pinned, so the sequential scan is used; the locally deleted
+# row 'ABC' must still be reported as update_deleted for the publisher's key
+# 'abc', exactly as if the index had been used.

It is not clear to me why here the behaviour should be similar to
index scan? As index is created after the slot is pinned, so isn't it
okay to behave as there is no index at all? I see the point that for
live tuples scan we may have used index but for conflicts as the index
could be created after the tuple we are searching is deleted, we are
doing sequence scan and comparing just indexed columns and that could
be seen as inconsistent. As this is sort of a grey area and we are not
sure whether to change the code or not, I feel we can fix Paul's
reported issue now as that is a clear bug.

Agreed. We can fix the reported issue for now. I'm attaching a merged patch
that uses the index comparison approach to fix only the reported issue here. I
also improved the test for the update_deleted detection code path to hold
slot.xmin stable, and removed some unnecessary instance creation/deletion to
make it lighter. Please see the updated patch.

Best Regards,
Zhijie Hou

On Mon, Oct 12, 2026 at 7:50 AM Zhijie Hou <houzhijie22@gmail.com> wrote:

Agreed. We can fix the reported issue for now. I'm attaching a merged patch
that uses the index comparison approach to fix only the reported issue here. I
also improved the test for the update_deleted detection code path to hold
slot.xmin stable, and removed some unnecessary instance creation/deletion to
make it lighter. Please see the updated patch.

This v4 looks good to me. Thank you for helping with this! Maybe one
minor change:

src/test/subscription/t/035_conflicts.pl
+# Publisher table holding the row that will be updated.
+$node_B->safe_psql(
+   'postgres', "
+   CREATE TABLE tab_mr (id int4multirange, valid_at daterange, v int);
+   ALTER TABLE tab_mr REPLICA IDENTITY FULL;
+   INSERT INTO tab_mr VALUES ('{[1,5)}', '[2000-01-01,2010-01-01)', 1);
+   ALTER PUBLICATION tap_pub_B ADD TABLE tab_mr;
+");

If we say PRIMARY KEY (id, valid_at WITHOUT OVERLAPS) then we don't
need REPLICA IDENTITY FULL.

Yours,

--
Paul ~{:-)
pj@illuminatedcomputing.com

This v4 looks good to me. Thank you for helping with this! Maybe one
minor change:

+1
I couldn't find the CF entry for this, could you please point me to it?

On Tue, Oct 13, 2026 at 5:37 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

This v4 looks good to me. Thank you for helping with this! Maybe one
minor change:

src/test/subscription/t/035_conflicts.pl
+# Publisher table holding the row that will be updated.
+$node_B->safe_psql(
+   'postgres', "
+   CREATE TABLE tab_mr (id int4multirange, valid_at daterange, v int);
+   ALTER TABLE tab_mr REPLICA IDENTITY FULL;
+   INSERT INTO tab_mr VALUES ('{[1,5)}', '[2000-01-01,2010-01-01)', 1);
+   ALTER PUBLICATION tap_pub_B ADD TABLE tab_mr;
+");

If we say PRIMARY KEY (id, valid_at WITHOUT OVERLAPS) then we don't
need REPLICA IDENTITY FULL.

Yeah, but keeping RI FULL also doesn't harm; rather it tests something
extra which is that it can extract required values from the full
tuple. So, I am okay either way. I have modified the comments at
multiple places in the patch as I find there is a lot of redundancy in
the comments. See attached top-up patch. BTW, we need a separate patch
for v18 as well.

--
With Regards,
Amit Kapila.

Hi,

On Tue, Oct 13, 2026 at 7:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Oct 13, 2026 at 5:37 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

This v4 looks good to me. Thank you for helping with this! Maybe one
minor change:

src/test/subscription/t/035_conflicts.pl
+# Publisher table holding the row that will be updated.
+$node_B->safe_psql(
+   'postgres', "
+   CREATE TABLE tab_mr (id int4multirange, valid_at daterange, v int);
+   ALTER TABLE tab_mr REPLICA IDENTITY FULL;
+   INSERT INTO tab_mr VALUES ('{[1,5)}', '[2000-01-01,2010-01-01)', 1);
+   ALTER PUBLICATION tap_pub_B ADD TABLE tab_mr;
+");

If we say PRIMARY KEY (id, valid_at WITHOUT OVERLAPS) then we don't
need REPLICA IDENTITY FULL.

Yeah, but keeping RI FULL also doesn't harm; rather it tests something
extra which is that it can extract required values from the full
tuple. So, I am okay either way. I have modified the comments at
multiple places in the patch as I find there is a lot of redundancy in
the comments. See attached top-up patch. BTW, we need a separate patch
for v18 as well.

Thanks for the diff. I have merged and prepared the patches for
back branches as well.

Best Regards,
Zhijie Hou