Logical replication can lose an update after concurrent index invalidation

On Fri, 28 Aug 2026 at 03:05, Mihail Nikalayeu
<mihailnikalayeu@gmail.com> wrote:

Attached are patches for master (which also applies to v19 as-is),
v18, v17 and v16. The back-branch patches contain the code change
only. The test requires the injection point added by bc32a12e0db; v18
and v17 don't have it, and it can't be used on v16.

RelationFindReplTupleByIndex() is an extern function declared in a
core executor header (executor.h). Adding a new parameter
(isIdxSafeToSkipDuplicates) changes its calling convention. Any
third-party extension or out-of-tree apply worker compiled against an
earlier minor version like PG16 branch that calls this function will
pass incorrect arguments on the stack/registers, leading to stack
corruption or crashes when running on a patched minor release.
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -652,6 +652,7 @@ extern void check_exclusion_constraint(Relation
heap, Relation index,
  * prototypes from functions in execReplication.c
  */
 extern bool RelationFindReplTupleByIndex(Relation rel, Oid idxoid,
+ bool isIdxSafeToSkipDuplicates,
  LockTupleMode lockmode,
  TupleTableSlot *searchslot,
  TupleTableSlot *outslot);

Similarly I'm not sure of adding isidentity to LogicalRepRelMapEntry
in back branches as it will change its size and memory layout.
External extensions that embed, allocate, or iterate over arrays of
LogicalRepRelMapEntry across minor releases will experience struct
size mismatches and potential memory misalignment.
diff --git a/src/include/replication/logicalrelation.h
b/src/include/replication/logicalrelation.h
index 3f4d906d741..4f98b1ea789 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -33,6 +33,11 @@ typedef struct LogicalRepRelMapEntry
  AttrMap    *attrmap; /* map of local attributes to remote ones */
  bool updatable; /* Can apply updates/deletes? */
  Oid localindexoid; /* which index to use, or InvalidOid if none */
+ bool isidentity; /* localindexoid was chosen as the
+ * relation's replica identity or
+ * primary key, rather than as one
+ * usable for a REPLICA IDENTITY
+ * FULL remote relation */

Regards,
Vignesh

Hello!

RelationFindReplTupleByIndex() is an extern function declared in a
core executor header (executor.h). Adding a new parameter
(isIdxSafeToSkipDuplicates) changes its calling convention.
Similarly I'm not sure of adding isidentity to LogicalRepRelMapEntry
in back branches as it will change its size and memory layout.

Yep, fair, thanks.

In v2 I put a new flag into padding and used the same pattern for the
function as in 9e47718250d.

abidiff now shows that:

'struct LogicalRepRelMapEntry' changed:
type size hasn't changed
1 data member insertion:
'bool isidentity', at offset 89 (in bytes)

I think it is ok for such a type. I couldn't find any usage in extensions.

Should we update .abi-compliance-history in such a case?

Best regards,
Mikhail.

On Friday, August 28, 2026 8:15 PM Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:

In v2 I put a new flag into padding and used the same pattern for the function
as in 9e47718250d.

Thanks for sharing the patch.

The patch looks mostly good to me.

One nit for back-branch patches: I see you've created a new API that accepts
isIdxSafeToSkipDuplicates. In that case, we shall mention in the comments
atop the old function that it should not be used in new code anymore, otherwise
extensions might also encounter the same issue. We've added something like
below comments for other commits:

* ABI-compatible wrapper to emulate old version of the
* xxx function. Do not call this version in new code.

BTW, just to share, I'm reviewing this with an eye for whether other similar
code paths have the same problem. The other two GetRelationIdentityOrPK callers
in logicalrep also look worth adjusting on top of the current patch's new flag
(maybe in followup patches):

The one in check_relation_updatable() may cause the apply worker to report the
wrong error message.

The one in conflict.c could output wrong identity information.

Best Regards,
Zhijie Hou

On Fri, Aug 28, 2026 at 4:45 PM Mihail Nikalayeu
<mihailnikalayeu@gmail.com> wrote:

In v2 I put a new flag into padding and used the same pattern for the
function as in 9e47718250d.

Few comments:
=============
* The existing calls that use "Oid localindexoid" parameter grew a
parallel "bool isidentity" beside it. Can we consider passing the
LogicalRepRelMapEntry * instead that collapses two parameters into
one?

* How about naming struct member as idxisreplident instead of
isidentity as that reads closer to the catalog vocabulary?

*
FindReplTupleInLocalRel(ApplyExecutionData *edata, Relation localrel,
LogicalRepRelation *remoterel,
Oid localidxoid,
+ bool isidentity,
TupleTableSlot *remoteslot,
TupleTableSlot **localslot)
{
@@ -3239,14 +3252,14 @@ FindReplTupleInLocalRel(ApplyExecutionData
*edata, Relation localrel,
Relation idxrel = index_open(localidxoid, AccessShareLock);

  /* Index must be PK, RI, or usable for REPLICA IDENTITY FULL tables */
- Assert(GetRelationIdentityOrPK(localrel) == localidxoid ||
+ Assert(isidentity ||

The Assert(isidentity ...) part actually verifies nothing when
isidentity is true, previously the assert was checking it against
catalog value due to which it trips, so we need to change this assert
but I think we can do better than proposed. Two ideas:

a. Something on the lines of:
#ifdef USE_ASSERT_CHECKING
if (!isidentity)
{
Relation idxrel =
index_open(localidxoid, AccessShareLock);

/* Not the identity, so it must be usable for
REPLICA IDENTITY FULL */
Assert(remoterel->replident == REPLICA_IDENTITY_FULL);
Assert(IsIndexUsableForReplicaIdentityFull(idxrel,

edata->targetRel->attrmap));
index_close(idxrel, AccessShareLock);
}
#endif

b. An RI or PK index is unique and non-partial, and neither DIC nor
REINDEX CONCURRENTLY changes that, so something like the following
should also work:
#ifdef USE_ASSERT_CHECKING
Relation idxrel = index_open(localidxoid,
AccessShareLock);

if (isidentity)
{
/* A replica identity or primary key index
identifies the row. */
Assert(idxrel->rd_index->indisunique);
Assert(heap_attisnull(idxrel->rd_indextuple,

Anum_pg_index_indpred, NULL));
}
else
{
/* Otherwise every match is compared, so we
need a whole row. */
Assert(remoterel->replident == REPLICA_IDENTITY_FULL);
Assert(IsIndexUsableForReplicaIdentityFull(idxrel,

edata->targetRel->attrmap));
}
index_close(idxrel, AccessShareLock);
#endif

abidiff now shows that:

'struct LogicalRepRelMapEntry' changed:
type size hasn't changed
1 data member insertion:
'bool isidentity', at offset 89 (in bytes)

I think it is ok for such a type. I couldn't find any usage in extensions.

Should we update .abi-compliance-history in such a case?

Yes, I think this deserves a update in .abi-compliance-history but as
noted in the file comments (In general, entries should be added
reactively after an abi-compliance-check buildfarm failure.), we can
add it afterwards but certainly note down the need of same in the
commit message. For example, see commits: 1cd020324ef5 and a0f98b2755.

BTW, I have a question, how can we ascertain that we need a new API
for RelationFindReplTupleByIndex in back-branches? Yes, it is an
exposed API but on a quick search via AI, I couldn't find any of its
usage, can you once re-verify and share your opinion on the same?

--
With Regards,
Amit Kapila.

Zhijie, Amit, thanks for the reviews!

we shall mention in the comments atop the old function that it should
not be used in new code anymore

Done.

The other two GetRelationIdentityOrPK callers in logicalrep also look
worth adjusting on top of the current patch's new flag (maybe in
followup patches)

check_relation_updatable() is in this patch - it is a one-line change
conflict.c is not so fixing it means much more work to do. It gets an
XXX comment for now.

Can we consider passing the LogicalRepRelMapEntry * instead that
collapses two parameters into one?

Done for FindReplTupleInLocalRel(), FindDeletedTupleInLocalRel() and
apply_handle_delete_internal().
apply_handle_update_internal() just loses the Oid instead: its only
caller is the non-partitioned path, where the entry is
edata->targetRel.

How about naming struct member as idxisreplident

Done.

[the Assert] verifies nothing when isidentity is true

I took your (b) as is.

I think this deserves a update in .abi-compliance-history but [...] we
can add it afterwards but certainly note down the need of same in the
commit message

The commit message now says so.

how can we ascertain that we need a new API for
RelationFindReplTupleByIndex in back-branches?

spock does call it - [0]https://github.com/pgEdge/spock/blob/main/src/spock_apply_heap.c#L417.

I haven't checked back-branch commits (AI did them + ran the tests),
but here is one note from that process:

The back-branch patches are against the current tips. One porting note
for 16 and 17: their cross-partition UPDATE path has a second
apply_handle_delete_internal() call that master no longer has; it is
converted as well.

[0]: https://github.com/pgEdge/spock/blob/main/src/spock_apply_heap.c#L417

Regards,
Mikhail Nikalayeu

On Thu, 3 Sept 2026 at 16:15, Mihail Nikalayeu
<mihailnikalayeu@gmail.com> wrote:

Zhijie, Amit, thanks for the reviews!

we shall mention in the comments atop the old function that it should
not be used in new code anymore

Done.

Couple of minor comments:
1) I was able to compile without this header inclusion:
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -249,6 +249,7 @@

#include "access/genam.h"
#include "access/commit_ts.h"
+#include "access/htup_details.h"
#include "access/table.h"

2) pgindent needs to be run for the modified changes:
a) pgindent shows this needs adjustment:
+                    * A replica identity or primary key index
identifies the row on
+                        * its own.  The catalogs might no longer call
it the identity,
+                        * as DROP INDEX CONCURRENTLY or REINDEX
CONCURRENTLY can commit

b) similarly here too:
+       bool            idxisreplident; /* whether localindexoid is
the relation's
+                                                                *
replica identity or primary key, rather
+                                                                *
than an index usable for a REPLICA
+                                                                *
IDENTITY FULL remote relation.  Placed
+                                                                *
here to fit in existing padding, which
+                                                                *
leaves the struct layout unchanged in
+                                                                * the
back branches. */

Regards,
Vignesh

On Mon, Sep 7, 2026 at 9:04 PM vignesh C <vignesh21@gmail.com> wrote:

On Thu, 3 Sept 2026 at 16:15, Mihail Nikalayeu
<mihailnikalayeu@gmail.com> wrote:

Zhijie, Amit, thanks for the reviews!

we shall mention in the comments atop the old function that it should
not be used in new code anymore

Done.

Couple of minor comments:
1) I was able to compile without this header inclusion:
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -249,6 +249,7 @@

#include "access/genam.h"
#include "access/commit_ts.h"
+#include "access/htup_details.h"
#include "access/table.h"

Fixed in the attached. Apart from this I changed multiple comments to
make those clear. One notable change is, I moved the newly added
boolean after localindexoid as it reads better there because then we
don't need to forward reference the fields. For back-branches, if it
needs to be moved to an earlier location then we can do that in those
versions but for HEAD and 19, the new location seems better.

Also, shall we keep just one test, say Drop Index Concurrently instead
of two as both tests do the same thing in a slightly different way? I
have not done that but if you agree please update the patch
accordingly.

--
With Regards,
Amit Kapila.

Hi,

On Fri, Sep 18, 2026 at 8:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Fixed in the attached. Apart from this I changed multiple comments to
make those clear. One notable change is, I moved the newly added
boolean after localindexoid as it reads better there because then we
don't need to forward reference the fields. For back-branches, if it
needs to be moved to an earlier location then we can do that in those
versions but for HEAD and 19, the new location seems better.

Also, shall we keep just one test, say Drop Index Concurrently instead
of two as both tests do the same thing in a slightly different way? I
have not done that but if you agree please update the patch
accordingly.

I also think one DROP INDEX CONCURRENTLY test is sufficient to cover this bug.

The patch looks mostly good to me, I just have a few minor comments:

1.

extern bool RelationFindReplTupleByIndex(Relation rel, Oid idxoid,
+ bool isIdxSafeToSkipDuplicates,

Shall we use a shorter parameter name, like skipduplicates, to stay consistent
with the other parameters of this function?

2.

+ * XXX This can disagree with the index the apply worker searched by,
+ * see FindReplTupleInLocalRel(). It may not even be one that
+ * ExecOpenIndices() locked.
  */
  Oid replica_index = GetRelationIdentityOrPK(localrel);

Just in case we want to fix this in the current patch, I think one option that
avoids adding a new parameter to multiple public functions would be to cache the
replica identity index OID in remote_ctx and expose a function to retrieve it.
We could then call that function here instead of GetRelationIdentityOrPK().

Otherwise, we would need to add a new index OID parameter to
ExecSimpleRelationUpdate, even though the identity index isn't otherwise used
there - and ReportApplyConflict would need a new parameter as well.

Best Regards,
Zhijie Hou

On Fri, 18 Sept 2026 at 18:25, Amit Kapila <amit.kapila16@gmail.com> wrote:

Also, shall we keep just one test, say Drop Index Concurrently instead
of two as both tests do the same thing in a slightly different way? I
have not done that but if you agree please update the patch
accordingly.

I also felt one test should be fine.

The patch mostly looks good. I have one small comment: the tests use a
different ordering for wakeup and detach. If we retain the second
test, we could keep the ordering consistent.
+       # Release the worker.  The index it holds is no longer the identity.
+       $node_subscriber->safe_psql(
+               'postgres',
+               "SELECT
injection_points_wakeup('apply-update-before-open-indices');
+                SELECT
injection_points_detach('apply-update-before-open-indices');"
+       );

+       $node_subscriber->safe_psql(
+               'postgres',
+               "SELECT
injection_points_detach('apply-update-before-open-indices');
+                SELECT
injection_points_wakeup('apply-update-before-open-indices');"
+       );

Regards,
Vignesh

On Mon, Sep 21, 2026 at 2:03 PM Zhijie Hou <houzhijie22@gmail.com> wrote:

On Fri, Sep 18, 2026 at 8:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Fixed in the attached. Apart from this I changed multiple comments to
make those clear. One notable change is, I moved the newly added
boolean after localindexoid as it reads better there because then we
don't need to forward reference the fields. For back-branches, if it
needs to be moved to an earlier location then we can do that in those
versions but for HEAD and 19, the new location seems better.

Also, shall we keep just one test, say Drop Index Concurrently instead
of two as both tests do the same thing in a slightly different way? I
have not done that but if you agree please update the patch
accordingly.

I also think one DROP INDEX CONCURRENTLY test is sufficient to cover this bug.

The patch looks mostly good to me, I just have a few minor comments:

1.

extern bool RelationFindReplTupleByIndex(Relation rel, Oid idxoid,
+ bool isIdxSafeToSkipDuplicates,

Shall we use a shorter parameter name, like skipduplicates, to stay consistent
with the other parameters of this function?

LGTM.

2.

+ * XXX This can disagree with the index the apply worker searched by,
+ * see FindReplTupleInLocalRel(). It may not even be one that
+ * ExecOpenIndices() locked.
*/
Oid replica_index = GetRelationIdentityOrPK(localrel);

Just in case we want to fix this in the current patch, I think one option that
avoids adding a new parameter to multiple public functions would be to cache the
replica identity index OID in remote_ctx and expose a function to retrieve it.
We could then call that function here instead of GetRelationIdentityOrPK().

Otherwise, we would need to add a new index OID parameter to
ExecSimpleRelationUpdate, even though the identity index isn't otherwise used
there - and ReportApplyConflict would need a new parameter as well.

I think this is a lower priority case. Let's do this separately.

--
With Regards,
Amit Kapila.

On Fri, 18 Sept 2026 at 18:25, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Sep 7, 2026 at 9:04 PM vignesh C <vignesh21@gmail.com> wrote:

On Thu, 3 Sept 2026 at 16:15, Mihail Nikalayeu
<mihailnikalayeu@gmail.com> wrote:

Zhijie, Amit, thanks for the reviews!

we shall mention in the comments atop the old function that it should
not be used in new code anymore

Done.

Couple of minor comments:
1) I was able to compile without this header inclusion:
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -249,6 +249,7 @@

#include "access/genam.h"
#include "access/commit_ts.h"
+#include "access/htup_details.h"
#include "access/table.h"

Fixed in the attached. Apart from this I changed multiple comments to
make those clear. One notable change is, I moved the newly added
boolean after localindexoid as it reads better there because then we
don't need to forward reference the fields. For back-branches, if it
needs to be moved to an earlier location then we can do that in those
versions but for HEAD and 19, the new location seems better.

I felt we will have to use the earlier location in case of
back-branches to avoid offset changing.

Also, shall we keep just one test, say Drop Index Concurrently instead
of two as both tests do the same thing in a slightly different way? I
have not done that but if you agree please update the patch
accordingly.

Here is an updated version including the back-branches version.
This version also addresses Hou-san's agreed 1st comment from [1]/messages/by-id/CAFvd2n-NBbpLM5yhfxLdmpLy5vshpBNqbJ_vX4cj6Z3Wx5BwFw@mail.gmail.com.

[1]: /messages/by-id/CAFvd2n-NBbpLM5yhfxLdmpLy5vshpBNqbJ_vX4cj6Z3Wx5BwFw@mail.gmail.com

Regards,
Vignesh

On Mon, Sep 21, 2026 at 6:01 PM vignesh C <vignesh21@gmail.com> wrote:

Here is an updated version including the back-branches version.
This version also addresses Hou-san's agreed 1st comment from [1].

We should include the test till the version where required Injection
Point functionality is present.

--
With Regards,
Amit Kapila.

On Tue, 22 Sept 2026 at 15:33, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Sep 21, 2026 at 6:01 PM vignesh C <vignesh21@gmail.com> wrote:

Here is an updated version including the back-branches version.
This version also addresses Hou-san's agreed 1st comment from [1].

We should include the test till the version where required Injection
Point functionality is present.

Here is an updated version with the injection point and its test added
up to the PG17 version, where injection points were introduced.
I have also retained the DROP INDEX CONCURRENTLY test instead of the
REINDEX CONCURRENTLY test, as the former requires only one injection
point (apply-update-before-open-indices), whereas the latter requires
two injection points: reindex-relation-concurrently-before-swap and
apply-update-before-open-indices.

Regards,
Vignesh

On Wed, 23 Sept 2026 at 15:26, vignesh C <vignesh21@gmail.com> wrote:

On Tue, 22 Sept 2026 at 15:33, Amit Kapila <amit.kapila16@gmail.com> wrote:

The ABI compliance check in buildfarm failed at [1]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=akepa&amp;dt=2026-09-28%2011%3A27%3A04, [2]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=akepa&amp;dt=2026-09-28%2011%3A26%3A06, and [3]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&amp;dt=2026-09-28%2011%3A47%3A02
after the patch was committed because the LogicalRepRelMapEntry
structure added the idxisreplident field. However, this does not cause
an ABI break, as the new bool uses existing alignment padding, so the
offsets of existing fields and sizeof(LogicalRepRelMapEntry) remain
unchanged. Here is a patch to document this in
.abi-compliance-history.

[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=akepa&amp;dt=2026-09-28%2011%3A27%3A04
[2]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=akepa&amp;dt=2026-09-28%2011%3A26%3A06
[3]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&amp;dt=2026-09-28%2011%3A47%3A02

Regards,
Vignesh

On Mon, Sep 28, 2026 at 8:20 AM vignesh C <vignesh21@gmail.com> wrote:

The ABI compliance check in buildfarm failed at [1], [2], and [3]
after the patch was committed because the LogicalRepRelMapEntry
structure added the idxisreplident field. However, this does not cause
an ABI break, as the new bool uses existing alignment padding, so the
offsets of existing fields and sizeof(LogicalRepRelMapEntry) remain
unchanged. Here is a patch to document this in
.abi-compliance-history.

Thanks, I've pushed the patches.

--
With Regards,
Amit Kapila.