Apply worker can pick an invalid index for REPLICA IDENTITY FULL lookups
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253523psql -h localhost -U postgresBuilt from patchset v11 (message #11), August 26, 2026 at 09:20 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253523_11 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253523_11 && git checkout t253523_11Patchset v11 (message #11) is on t253523_11
Hello,
When the remote relation has REPLICA IDENTITY FULL and the local one
has no primary key or replica identity, the apply worker looks for any
index it can search by.
FindUsableIndexForReplicaIdentityFull() walks RelationGetIndexList()
and takes the first index of a suitable shape without checking if it
is valid.
That list omits only indexes that are not indislive, so the leftover
from a failed CREATE INDEX CONCURRENTLY is eligible -- and such an
index need not contain every row.
A failed CIC validation leaves an index that scans cleanly but lacks
exactly the rows validation would have added. Consequently, updates
and deletes for those rows are dropped as update_missing conflicts and
the subscriber quietly diverges.
A failed build leaves an index that is empty down to the metapage,
which parks the subscription in a permanent error:
ERROR: could not read blocks 0..0 in file "base/5/16433": read only 0
of 8192 bytes
The fix and a reproducer are in the attachments.
Affects 16 and up, from 89e46da5e51.
Regards,
Mikhail Nikalayeu
Hi Mikhail
Any thoughts on moving the 'idxRel->rd_index->indisvalid' check into 'IsIndexUsableForReplicaIdentityFull()' itself?
As I understand it, an index MUST be valid to be usable for REPLICA IDENTITY FULL lookups, so it seems more natural to have that check live alongside the other usability checks in `IsIndexUsableForReplicaIdentityFull()`, rather than in the caller.
Kind regards,
Miłosz Bieniek
Hello, Miłosz!
Any thoughts on moving the 'idxRel->rd_index->indisvalid' check into 'IsIndexUsableForReplicaIdentityFull()' itself?
I initially thought about that approach too, but an assertion also
uses that check.
This is explained in the commit message:
Skip invalid indexes, as the planner does. The test belongs here rather
than in IsIndexUsableForReplicaIdentityFull(), whose other caller is an
assertion that would then trip over an index DROP INDEX CONCURRENTLY has
just invalidated but that still finds tuples fine.
Regards,
Mikhail
I initially thought about that approach too, but an assertion also
uses that check.
This is explained in the commit message:Skip invalid indexes, as the planner does. The test belongs here rather
than in IsIndexUsableForReplicaIdentityFull(), whose other caller is an
assertion that would then trip over an index DROP INDEX CONCURRENTLY has
just invalidated but that still finds tuples fine.
Oh sorry, I didn't notice it. Make sense.
I was wondering if there is a way (and whether it even makes sens
to make this test fail faster?
Right now, if we remove `idxRel->rd_index->indisvalid &&` the test
just hangs until the overall timeout is hit and we're not getting
any meaningful message.
Kind regards,
Miłosz Bieniek
Hi!
I was wondering if there is a way (and whether it even makes sens
to make this test fail faster?
I was thinking about that too, so I decided to keep it that way
(aligned with other tests).
Adding some special tricks will make the test too narrow for a
particular scenario.
Hi!
I was wondering if there is a way (and whether it even makes sens
to make this test fail faster?I was thinking about that too, so I decided to keep it that way
(aligned with other tests).
Adding some special tricks will make the test too narrow for a
particular scenario.
Understandable. LGTM!
Kind regards,
Miłosz Bieniek
On Mon, Aug 24, 2026 at 9:23 PM Mihail Nikalayeu
<mihailnikalayeu@gmail.com> wrote:
Any thoughts on moving the 'idxRel->rd_index->indisvalid' check into 'IsIndexUsableForReplicaIdentityFull()' itself?
I initially thought about that approach too, but an assertion also
uses that check.
This is explained in the commit message:Skip invalid indexes, as the planner does. The test belongs here rather
than in IsIndexUsableForReplicaIdentityFull(), whose other caller is an
assertion that would then trip over an index DROP INDEX CONCURRENTLY has
just invalidated but that still finds tuples fine.
I think it is better to add some comments in the code as well. See
attached. I have added # in the test comments to make it similar to
the previous test. Otherwise, the patch LGTM.
--
With Regards,
Amit Kapila.
Attachments:
add_comments_1.patchapplication/octet-stream; name=add_comments_1.patchDownload+10-1
On Mon, 24 Aug 2026 at 21:23, Mihail Nikalayeu
<mihailnikalayeu@gmail.com> wrote:
Hello, Miłosz!
Any thoughts on moving the 'idxRel->rd_index->indisvalid' check into 'IsIndexUsableForReplicaIdentityFull()' itself?
I initially thought about that approach too, but an assertion also
uses that check.
This is explained in the commit message:Skip invalid indexes, as the planner does. The test belongs here rather
than in IsIndexUsableForReplicaIdentityFull(), whose other caller is an
assertion that would then trip over an index DROP INDEX CONCURRENTLY has
just invalidated but that still finds tuples fine.
Hi Mihail,
I am able to reproduce the issue on HEAD and from PG16 to PG19.
I tested the patch on HEAD and it resolves the issue.
I also agree that the function 'FindUsableIndexForReplicaIdentityFull'
is appropriate. I have manually verified that adding the check in
'IsIndexUsableForReplicaIdentityFull' can cause the Assert to trigger,
when DROP INDEX CONCURRENTLY is run concurrently.
The patch along with changes provided by Amit, looks good to me.
Thanks,
Shlok Kyal
On Sun, 23 Aug 2026 at 01:26, Mihail Nikalayeu
<mihailnikalayeu@gmail.com> wrote:
Hello,
When the remote relation has REPLICA IDENTITY FULL and the local one
has no primary key or replica identity, the apply worker looks for any
index it can search by.
FindUsableIndexForReplicaIdentityFull() walks RelationGetIndexList()
and takes the first index of a suitable shape without checking if it
is valid.
That list omits only indexes that are not indislive, so the leftover
from a failed CREATE INDEX CONCURRENTLY is eligible -- and such an
index need not contain every row.A failed CIC validation leaves an index that scans cleanly but lacks
exactly the rows validation would have added. Consequently, updates
and deletes for those rows are dropped as update_missing conflicts and
the subscriber quietly diverges.A failed build leaves an index that is empty down to the metapage,
which parks the subscription in a permanent error:
ERROR: could not read blocks 0..0 in file "base/5/16433": read only 0
of 8192 bytesThe fix and a reproducer are in the attachments.
Affects 16 and up, from 89e46da5e51.
Thanks for the patch, the patch does not apply on PG17 version, you
might have to provide a separate patch for PG17 & below version:
patch -p1 < v1-0001-Don-t-choose-an-invalid-index-for-REPLICA-IDENTIT.patch
patching file 'src/backend/replication/logical/relation.c'
1 out of 1 hunks failed--saving rejects to
'src/backend/replication/logical/relation.c.rej'
patching file 'src/test/subscription/t/032_subscribe_use_index.pl'
Regards,
Vignesh
On Sun, Aug 23, 2026 at 5:56 AM Mihail Nikalayeu
<mihailnikalayeu@gmail.com> wrote:
Hello,
The fix and a reproducer are in the attachments.
Affects 16 and up, from 89e46da5e51.
The patch looks good, just a small suggestion that the function header
of IsIndexUsableForReplicaIdentityFull() probably could do with a
comment so that future callers are aware of this potential shortfall.
Something like:
* Note that this function does not check indisvalid. Callers that are
* selecting an index to use for future lookups must check indisvalid
* themselves and reject invalid indexes
regards,
Ajin Cherian
Fujitsu Australia
Hello!
The patch looks good, just a small suggestion that the function header
of IsIndexUsableForReplicaIdentityFull() probably could do with a
comment so that future callers are aware of this potential shortfall.
Done.
Thanks for the patch, the patch does not apply on PG17 version, you
might have to provide a separate patch for PG17 & below version:
Done.
I think it is better to add some comments in the code as well. See
attached. I have added # in the test comments to make it similar to
the previous test. Otherwise, the patch LGTM.
Applied, thanks.
Regards,
Mikhail.
Attachments:
t253523_11nocfbot-v1-REL_16-0001-Don-t-choose-an-invalid-index-for-REPLICA-IDENTITY-FULL.patchapplication/octet-stream; name=nocfbot-v1-REL_16-0001-Don-t-choose-an-invalid-index-for-REPLICA-IDENTITY-FULL.patchDownload+73-2
v1-0001-Don-t-choose-an-invalid-index-for-REPLICA-IDENTITY-FULL.patchapplication/octet-stream; name=v1-0001-Don-t-choose-an-invalid-index-for-REPLICA-IDENTITY-FULL.patchDownload+73-2
nocfbot-v1-REL_17-0001-Don-t-choose-an-invalid-index-for-REPLICA-IDENTITY-FULL.patchapplication/octet-stream; name=nocfbot-v1-REL_17-0001-Don-t-choose-an-invalid-index-for-REPLICA-IDENTITY-FULL.patchDownload+73-2
On Thu, Aug 27, 2026 at 2:37 AM Mihail Nikalayeu
<mihailnikalayeu@gmail.com> wrote:
I think it is better to add some comments in the code as well. See
attached. I have added # in the test comments to make it similar to
the previous test. Otherwise, the patch LGTM.Applied, thanks.
Pushed.
--
With Regards,
Amit Kapila.