Hello! FindLogicalRepLocalIndex() chooses the index used to look up the local tuple. It can either choose the relation's replica identity / primary key, or an index usable when the remote relation has REPLICA IDENTITY FULL.
That distinction matters later: for a replica identity / primary key
index, the first index match is enough; otherwise every match has to
be compared with the search slot.
At the moment we only keep the index OID, so
RelationFindReplTupleByIndex() derives that distinction again:
isIdxSafeToSkipDuplicates = (GetRelationIdentityOrPK(rel) == idxoid);
The problem is that the answer can change between those two points.
Apply only holds RowExclusiveLock. That doesn't conflict with REINDEX
CONCURRENTLY swapping in the rebuilt index, or with DROP INDEX
CONCURRENTLY marking an index invalid. The resulting relcache
invalidation can be processed by ExecOpenIndices().
If that happens, RelationFindReplTupleByIndex() might decide that the
index is no longer the replica identity / primary key index and start
doing whole-row comparisons. But the search slot contains a whole row
only for REPLICA IDENTITY FULL. Nothing matches, and the change is
reported as missing:
LOG: conflict detected on relation "public.t": conflict=update_missing
Replication then continues and the subscriber silently diverges.
With assertions enabled this instead trips the identity check just
above, which is how I found the problem (working on the stress-suite
for different CONCURRENTY scenarios).
Only one change is lost per occurrence. The same invalidation also
invalidates the relation map entry, so the next change rebuilds it and
gets the right state again.
The attached fix stores, together with the OID, whether the selected
index was chosen as replica identity/primary key. That information is
then passed to the scan instead of being derived a second time.
The issue affects v16 and later, starting with 89e46da5e51.
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.
Regards,
Mikhail Nikalayeu
v1-0001-Don-t-re-derive-in-the-scan-what-the-apply-worker.patch
Description: Binary data
nocfbot-17-v1-0001-Don-t-re-derive-in-the-scan-what-the-apply-worker.patch
Description: Binary data
nocfbot-16-v1-0001-Don-t-re-derive-in-the-scan-what-the-apply-worker.patch
Description: Binary data
nocfbot-18-v1-0001-Don-t-re-derive-in-the-scan-what-the-apply-worker.patch
Description: Binary data
