tablecmds: fix bug where index rebuild loses replica identity on partitions
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.
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:t53183psql -h localhost -U postgresBuilt from patchset v41 (message #41), August 30, 2026 at 03:16 AM.
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 t53183_41 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 t53183_41 && git checkout t53183_41Patchset v41 (message #41) is on t53183_41
Hi Hackers,
I found this bug while working on a related patch [1]/messages/by-id/CAEoWx2nJ71hy8R614HQr7vQhkBReO9AANPODPg0aSQs74eOdLQ@mail.gmail.com.
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and that index is used as REPLICA IDENTITY on a partitioned table, the replica
identity marking on partitions can be silently lost after the rebuild.
Below is a simple reproduction:
```
-- create a partitioned table and a parition
create table parent (id int not null, val int not null) partition by range (id);
create table child partition of parent for values from (1) to (100);
-- create an index on parent
create unique index indx_1 on parent(id, val);
-- the index is auto created on child, and both indexes’ indisreplident are false
select c.relname as index_name, c.oid as index_oid, i.indisreplident from pg_class c join pg_index i on c.oid = i.indexrelid where (c.relname = 'indx_1' or c.relname = 'child_id_val_idx’);
index_name | index_oid | indisreplident
------------------+-----------+----------------
indx_1 | 24594 | f
child_id_val_idx | 24595 | f
(2 rows)
-- as replica identity doesn’t recurse, set it on parent and child individually
alter table parent replica identity using index indx_1;
alter table child replica identity using index child_id_val_idx;
-- now both indexes are marked as replica identity
select c.relname as index_name, c.oid as index_oid, i.indisreplident from pg_class c join pg_index i on c.oid = i.indexrelid where (c.relname = 'indx_1' or c.relname = ‘child_id_val_idx');
index_name | index_oid | indisreplident
------------------+-----------+----------------
indx_1 | 24594 | t
child_id_val_idx | 24595 | t
(2 rows)
-- alter a column type, the column is part of the index, it will cause the index to rebuid
alter table parent alter val type bigint;
-- from the OIDs, we can see both indexes are rebuilt, but the child partition loses its replica identity marking
select c.relname as index_name, c.oid as index_oid, i.indisreplident from pg_class c join pg_index i on c.oid = i.indexrelid where (c.relname = 'indx_1' or c.relname = ‘child_id_val_idx');
index_name | index_oid | indisreplident
------------------+-----------+----------------
child_id_val_idx | 24597 | f
indx_1 | 24596 | t
(2 rows)
```
This patch fixes the bug by tracking replica identity indexes across partition hierarchies and restoring replica identity markings on all affected partitions after index rebuilds. Regression tests are added.
[1]: /messages/by-id/CAEoWx2nJ71hy8R614HQr7vQhkBReO9AANPODPg0aSQs74eOdLQ@mail.gmail.com
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v1-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patchapplication/octet-stream; name=v1-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch; x-unix-mode=0644Download+196-30
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.
I am slightly confused by the tests included in the proposed patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass. These tests don't check what you
want them to.
- if (tab->replicaIdentityIndex)
+ if (tab->replicaIdentityIndexOids != NIL)
elog(ERROR, "relation %u has multiple indexes marked as replica identity", tab->relid);This looks wrong to me. This new list tracks the OIDs of indexes
where you'd want to make sure that relreplident is updated, and it
could be possible, based on your proposal, that multiple indexes are
stored in this list. The error message is at least not in line
anymore. Actually, do we really need this extra list at all? The
list of indexes to rebuild are tracked already in changedIndexOids,
and the partitioned indexes seem to be in it, no?
--
Michael
On Jan 27, 2026, at 15:39, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.I am slightly confused by the tests included in the proposed patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass.
Oops, that isn’t supposed to be so. I’ll check the test.
These tests don't check what you
want them to.- if (tab->replicaIdentityIndex) + if (tab->replicaIdentityIndexOids != NIL) elog(ERROR, "relation %u has multiple indexes marked as replica identity", tab->relid);This looks wrong to me. This new list tracks the OIDs of indexes
where you'd want to make sure that relreplident is updated, and it
could be possible, based on your proposal, that multiple indexes are
stored in this list. The error message is at least not in line
anymore. Actually, do we really need this extra list at all? The
list of indexes to rebuild are tracked already in changedIndexOids,
and the partitioned indexes seem to be in it, no?
No, changedIndexOids only tracks the root index OID. DefineIndex() will automatically rebuild indexes on all partitions, and won’t return the rebuilt index OIDs to “alter table”.
So, before index rebuild, this patch records potential affected indexes in replicaIdentityIndexOids, and after index rebuild, restore replica identity on them.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Jan 27, 2026, at 15:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:39, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.I am slightly confused by the tests included in the proposed patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass.Oops, that isn’t supposed to be so. I’ll check the test.
Okay, I see the problem is here:
```
+CREATE UNIQUE INDEX test_replica_identity_partitioned_pkey ON test_replica_identity_partitioned (id);
```
I missed to add column “val” into the index, so that alter type of val didn’t cause index rebuild.
Ideally, it’s better to also verify that index OIDs should have changed before and after alter column type, but I haven’t figured out how to do so. Do you have an idea?
Please see v2, the test should fail on master now.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v2-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patchapplication/octet-stream; name=v2-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch; x-unix-mode=0644Download+196-30
On Jan 27, 2026, at 16:30, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:39, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.I am slightly confused by the tests included in the proposed patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass.Oops, that isn’t supposed to be so. I’ll check the test.
Okay, I see the problem is here:
```
+CREATE UNIQUE INDEX test_replica_identity_partitioned_pkey ON test_replica_identity_partitioned (id);
```I missed to add column “val” into the index, so that alter type of val didn’t cause index rebuild.
Ideally, it’s better to also verify that index OIDs should have changed before and after alter column type, but I haven’t figured out how to do so. Do you have an idea?
I just updated the test to store index OIDs before and after rebuild into 2 temp tables, so that we can compare the OIDs to verify rebuild happens and replica identity preserved.
I tried to port the test to master branch, and the test failed. From the test diff file, we can see replica identity lost on 3 leaf partitions:
```
@@ -360,9 +360,9 @@
ORDER BY b.index_name;
index_name | rebuilt | ri_lost
---------------------------------------------------+---------+---------
- test_replica_identity_partitioned_p1_id_val_idx | t | f
- test_replica_identity_partitioned_p2_1_id_val_idx | t | f
- test_replica_identity_partitioned_p2_2_id_val_idx | t | f
+ test_replica_identity_partitioned_p1_id_val_idx | t | t
+ test_replica_identity_partitioned_p2_1_id_val_idx | t | t
+ test_replica_identity_partitioned_p2_2_id_val_idx | t | t
test_replica_identity_partitioned_p2_id_val_idx | t | f
test_replica_identity_partitioned_pkey | t | f
(5 rows)
```With this patch, the test passes and all replica identity are preserved.
PFA v3:
* Enhanced the test.
* A small change in find_partition_replica_identity_indexes(): if we will not update a partition, then unlock it.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v3-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patchapplication/octet-stream; name=v3-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch; x-unix-mode=0644Download+227-30
On Jan 28, 2026, at 10:49, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 16:30, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:39, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.I am slightly confused by the tests included in the proposed patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass.Oops, that isn’t supposed to be so. I’ll check the test.
Okay, I see the problem is here:
```
+CREATE UNIQUE INDEX test_replica_identity_partitioned_pkey ON test_replica_identity_partitioned (id);
```I missed to add column “val” into the index, so that alter type of val didn’t cause index rebuild.
Ideally, it’s better to also verify that index OIDs should have changed before and after alter column type, but I haven’t figured out how to do so. Do you have an idea?
I just updated the test to store index OIDs before and after rebuild into 2 temp tables, so that we can compare the OIDs to verify rebuild happens and replica identity preserved.
I tried to port the test to master branch, and the test failed. From the test diff file, we can see replica identity lost on 3 leaf partitions: ``` @@ -360,9 +360,9 @@ ORDER BY b.index_name; index_name | rebuilt | ri_lost ---------------------------------------------------+---------+--------- - test_replica_identity_partitioned_p1_id_val_idx | t | f - test_replica_identity_partitioned_p2_1_id_val_idx | t | f - test_replica_identity_partitioned_p2_2_id_val_idx | t | f + test_replica_identity_partitioned_p1_id_val_idx | t | t + test_replica_identity_partitioned_p2_1_id_val_idx | t | t + test_replica_identity_partitioned_p2_2_id_val_idx | t | t test_replica_identity_partitioned_p2_id_val_idx | t | f test_replica_identity_partitioned_pkey | t | f (5 rows) ```With this patch, the test passes and all replica identity are preserved.
PFA v3:
* Enhanced the test.
* A small change in find_partition_replica_identity_indexes(): if we will not update a partition, then unlock it.Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v3-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch>
The CF asked for a rebase, thus rebased as v4.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v4-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patchapplication/octet-stream; name=v4-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch; x-unix-mode=0644Download+227-30
On Feb 26, 2026, at 14:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 28, 2026, at 10:49, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 16:30, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:39, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.I am slightly confused by the tests included in the proposed patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass.Oops, that isn’t supposed to be so. I’ll check the test.
Okay, I see the problem is here:
```
+CREATE UNIQUE INDEX test_replica_identity_partitioned_pkey ON test_replica_identity_partitioned (id);
```I missed to add column “val” into the index, so that alter type of val didn’t cause index rebuild.
Ideally, it’s better to also verify that index OIDs should have changed before and after alter column type, but I haven’t figured out how to do so. Do you have an idea?
I just updated the test to store index OIDs before and after rebuild into 2 temp tables, so that we can compare the OIDs to verify rebuild happens and replica identity preserved.
I tried to port the test to master branch, and the test failed. From the test diff file, we can see replica identity lost on 3 leaf partitions: ``` @@ -360,9 +360,9 @@ ORDER BY b.index_name; index_name | rebuilt | ri_lost ---------------------------------------------------+---------+--------- - test_replica_identity_partitioned_p1_id_val_idx | t | f - test_replica_identity_partitioned_p2_1_id_val_idx | t | f - test_replica_identity_partitioned_p2_2_id_val_idx | t | f + test_replica_identity_partitioned_p1_id_val_idx | t | t + test_replica_identity_partitioned_p2_1_id_val_idx | t | t + test_replica_identity_partitioned_p2_2_id_val_idx | t | t test_replica_identity_partitioned_p2_id_val_idx | t | f test_replica_identity_partitioned_pkey | t | f (5 rows) ```With this patch, the test passes and all replica identity are preserved.
PFA v3:
* Enhanced the test.
* A small change in find_partition_replica_identity_indexes(): if we will not update a partition, then unlock it.Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v3-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch>
The CF asked for a rebase, thus rebased as v4.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v4-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch>
Rebased, and a gentle ping.
Since this is a bug fix and the issue is easy to reproduce, I’m hoping it can still make it into v19.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v5-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patchapplication/octet-stream; name=v5-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch; x-unix-mode=0644Download+227-30
On Thu, Mar 19, 2026 at 1:07 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Feb 26, 2026, at 14:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 28, 2026, at 10:49, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 16:30, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:39, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.I am slightly confused by the tests included in the proposed patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass.Oops, that isn’t supposed to be so. I’ll check the test.
Okay, I see the problem is here:
```
+CREATE UNIQUE INDEX test_replica_identity_partitioned_pkey ON test_replica_identity_partitioned (id);
```I missed to add column “val” into the index, so that alter type of val didn’t cause index rebuild.
Ideally, it’s better to also verify that index OIDs should have changed before and after alter column type, but I haven’t figured out how to do so. Do you have an idea?
I just updated the test to store index OIDs before and after rebuild into 2 temp tables, so that we can compare the OIDs to verify rebuild happens and replica identity preserved.
I tried to port the test to master branch, and the test failed. From the test diff file, we can see replica identity lost on 3 leaf partitions: ``` @@ -360,9 +360,9 @@ ORDER BY b.index_name; index_name | rebuilt | ri_lost ---------------------------------------------------+---------+--------- - test_replica_identity_partitioned_p1_id_val_idx | t | f - test_replica_identity_partitioned_p2_1_id_val_idx | t | f - test_replica_identity_partitioned_p2_2_id_val_idx | t | f + test_replica_identity_partitioned_p1_id_val_idx | t | t + test_replica_identity_partitioned_p2_1_id_val_idx | t | t + test_replica_identity_partitioned_p2_2_id_val_idx | t | t test_replica_identity_partitioned_p2_id_val_idx | t | f test_replica_identity_partitioned_pkey | t | f (5 rows) ```With this patch, the test passes and all replica identity are preserved.
PFA v3:
* Enhanced the test.
* A small change in find_partition_replica_identity_indexes(): if we will not update a partition, then unlock it.Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v3-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch>
The CF asked for a rebase, thus rebased as v4.
Hi, I reproduced this with the test case, and the patch appears
to resolve it.
Some comments on v5:
-- Whether it makes sense to use a single list of pair structs instead
of two parallel OID lists (replicaIdentityIndexOids +
replicaIdentityTableOids) to avoid accidental desync.
-- It would be better to make lock handling in
find_partition_replica_identity_indexes() consistent
(relation_open(..., NoLock) if child is already locked, and avoid
mixed relation_close(..., lockmode)/NoLock behavior).
-- Some typos in comments/tests (partion/parition).
--
Best,
Xuneng
On Mar 21, 2026, at 18:29, Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Thu, Mar 19, 2026 at 1:07 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Feb 26, 2026, at 14:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 28, 2026, at 10:49, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 16:30, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:39, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.I am slightly confused by the tests included in the proposed patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass.Oops, that isn’t supposed to be so. I’ll check the test.
Okay, I see the problem is here:
```
+CREATE UNIQUE INDEX test_replica_identity_partitioned_pkey ON test_replica_identity_partitioned (id);
```I missed to add column “val” into the index, so that alter type of val didn’t cause index rebuild.
Ideally, it’s better to also verify that index OIDs should have changed before and after alter column type, but I haven’t figured out how to do so. Do you have an idea?
I just updated the test to store index OIDs before and after rebuild into 2 temp tables, so that we can compare the OIDs to verify rebuild happens and replica identity preserved.
I tried to port the test to master branch, and the test failed. From the test diff file, we can see replica identity lost on 3 leaf partitions: ``` @@ -360,9 +360,9 @@ ORDER BY b.index_name; index_name | rebuilt | ri_lost ---------------------------------------------------+---------+--------- - test_replica_identity_partitioned_p1_id_val_idx | t | f - test_replica_identity_partitioned_p2_1_id_val_idx | t | f - test_replica_identity_partitioned_p2_2_id_val_idx | t | f + test_replica_identity_partitioned_p1_id_val_idx | t | t + test_replica_identity_partitioned_p2_1_id_val_idx | t | t + test_replica_identity_partitioned_p2_2_id_val_idx | t | t test_replica_identity_partitioned_p2_id_val_idx | t | f test_replica_identity_partitioned_pkey | t | f (5 rows) ```With this patch, the test passes and all replica identity are preserved.
PFA v3:
* Enhanced the test.
* A small change in find_partition_replica_identity_indexes(): if we will not update a partition, then unlock it.Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v3-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch>
The CF asked for a rebase, thus rebased as v4.
Hi, I reproduced this with the test case, and the patch appears
to resolve it.Some comments on v5:
Thanks a lot for your review.
-- Whether it makes sense to use a single list of pair structs instead
of two parallel OID lists (replicaIdentityIndexOids +
replicaIdentityTableOids) to avoid accidental desync.
I don’t think that helps much. The current code of rebuilding index uses two lists changedIndexOids and changedIndexDefs. So, this patch matches the pattern of the existing code.
-- It would be better to make lock handling in
find_partition_replica_identity_indexes() consistent
(relation_open(..., NoLock) if child is already locked, and avoid
mixed relation_close(..., lockmode)/NoLock behavior).
That’s because if we are going to update a partition, then we need to hold the lock on the partition.
-- Some typos in comments/tests (partion/parition).
Fixed.
PFA v6: fixed a typo in comment.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v6-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patchapplication/octet-stream; name=v6-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch; x-unix-mode=0644Download+227-30
Hi,
On Mon, Mar 23, 2026 at 3:57 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Mar 21, 2026, at 18:29, Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Thu, Mar 19, 2026 at 1:07 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Feb 26, 2026, at 14:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 28, 2026, at 10:49, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 16:30, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:39, Michael Paquier <michael@paquier.xyz>
wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild,
and
that index is used as REPLICA IDENTITY on a partitioned table,
the
replica identity marking on partitions can be silently lost
after the
rebuild.
I am slightly confused by the tests included in the proposed
patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass.Oops, that isn’t supposed to be so. I’ll check the test.
Okay, I see the problem is here:
```
+CREATE UNIQUE INDEX test_replica_identity_partitioned_pkey ON
test_replica_identity_partitioned (id);
```
I missed to add column “val” into the index, so that alter type of
val didn’t cause index rebuild.
Ideally, it’s better to also verify that index OIDs should have
changed before and after alter column type, but I haven’t figured out how
to do so. Do you have an idea?
I just updated the test to store index OIDs before and after rebuild
into 2 temp tables, so that we can compare the OIDs to verify rebuild
happens and replica identity preserved.
I tried to port the test to master branch, and the test failed. From
the test diff file, we can see replica identity lost on 3 leaf partitions:
```
@@ -360,9 +360,9 @@
ORDER BY b.index_name;
index_name | rebuilt | ri_lost
---------------------------------------------------+---------+---------
- test_replica_identity_partitioned_p1_id_val_idx | t | f - test_replica_identity_partitioned_p2_1_id_val_idx | t | f - test_replica_identity_partitioned_p2_2_id_val_idx | t | f + test_replica_identity_partitioned_p1_id_val_idx | t | t + test_replica_identity_partitioned_p2_1_id_val_idx | t | t + test_replica_identity_partitioned_p2_2_id_val_idx | t | t test_replica_identity_partitioned_p2_id_val_idx | t | f test_replica_identity_partitioned_pkey | t | f (5 rows) ```With this patch, the test passes and all replica identity are
preserved.
PFA v3:
* Enhanced the test.
* A small change in find_partition_replica_identity_indexes(): if we
will not update a partition, then unlock it.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v3-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch>
The CF asked for a rebase, thus rebased as v4.
Hi, I reproduced this with the test case, and the patch appears
to resolve it.Some comments on v5:
Thanks a lot for your review.
-- Whether it makes sense to use a single list of pair structs instead
of two parallel OID lists (replicaIdentityIndexOids +
replicaIdentityTableOids) to avoid accidental desync.I don’t think that helps much. The current code of rebuilding index uses
two lists changedIndexOids and changedIndexDefs. So, this patch matches the
pattern of the existing code.
-- It would be better to make lock handling in
find_partition_replica_identity_indexes() consistent
(relation_open(..., NoLock) if child is already locked, and avoid
mixed relation_close(..., lockmode)/NoLock behavior).That’s because if we are going to update a partition, then we need to
hold the lock on the partition.
There is one locking cleanup in find_partition_replica_identity_indexes().
find_inheritance_children(relId, lockmode) already acquires lockmode on
every partition it returns, so I think the later relation_open() should
use
NoLock, not lockmode. For the same reason, all relation_close() calls in
this function should use NoLock as well.
Today the code does:
partRel = relation_open(partRelOid, lockmode);
...
relation_close(partRel, lockmode);
That does not cause a correctness issue, because the lock manager
reference-counts same-transaction acquisitions, so the lock remains held
either way. But it is misleading: it suggests that relation_open() is
where
the partition lock is taken, and that the early relation_close(...,
lockmode)
is intentionally releasing it. Neither is actually true here, because the
lock
was already acquired by find_inheritance_children().
So I think this should be adjusted to:
partRel = relation_open(partRelOid, NoLock);
and all close sites in this function should be:
relation_close(partRel, NoLock);
The comment on the early-close path should also be updated, since it is
not
really unlocking the partition. Something like "No matching partition
index;
just close the relcache entry" would match the actual behavior better.
-- Some typos in comments/tests (partion/parition).
Fixed.
PFA v6: fixed a typo in comment.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
--
Best,
Xuneng
On Mar 23, 2026, at 16:41, Xuneng Zhou <xunengzhou@gmail.com> wrote:
Hi,
On Mon, Mar 23, 2026 at 3:57 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Mar 21, 2026, at 18:29, Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Thu, Mar 19, 2026 at 1:07 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Feb 26, 2026, at 14:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 28, 2026, at 10:49, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 16:30, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:39, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.I am slightly confused by the tests included in the proposed patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass.Oops, that isn’t supposed to be so. I’ll check the test.
Okay, I see the problem is here:
```
+CREATE UNIQUE INDEX test_replica_identity_partitioned_pkey ON test_replica_identity_partitioned (id);
```I missed to add column “val” into the index, so that alter type of val didn’t cause index rebuild.
Ideally, it’s better to also verify that index OIDs should have changed before and after alter column type, but I haven’t figured out how to do so. Do you have an idea?
I just updated the test to store index OIDs before and after rebuild into 2 temp tables, so that we can compare the OIDs to verify rebuild happens and replica identity preserved.
I tried to port the test to master branch, and the test failed. From the test diff file, we can see replica identity lost on 3 leaf partitions: ``` @@ -360,9 +360,9 @@ ORDER BY b.index_name; index_name | rebuilt | ri_lost ---------------------------------------------------+---------+--------- - test_replica_identity_partitioned_p1_id_val_idx | t | f - test_replica_identity_partitioned_p2_1_id_val_idx | t | f - test_replica_identity_partitioned_p2_2_id_val_idx | t | f + test_replica_identity_partitioned_p1_id_val_idx | t | t + test_replica_identity_partitioned_p2_1_id_val_idx | t | t + test_replica_identity_partitioned_p2_2_id_val_idx | t | t test_replica_identity_partitioned_p2_id_val_idx | t | f test_replica_identity_partitioned_pkey | t | f (5 rows) ```With this patch, the test passes and all replica identity are preserved.
PFA v3:
* Enhanced the test.
* A small change in find_partition_replica_identity_indexes(): if we will not update a partition, then unlock it.Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v3-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch>
The CF asked for a rebase, thus rebased as v4.
Hi, I reproduced this with the test case, and the patch appears
to resolve it.Some comments on v5:
Thanks a lot for your review.
-- Whether it makes sense to use a single list of pair structs instead
of two parallel OID lists (replicaIdentityIndexOids +
replicaIdentityTableOids) to avoid accidental desync.I don’t think that helps much. The current code of rebuilding index uses two lists changedIndexOids and changedIndexDefs. So, this patch matches the pattern of the existing code.
-- It would be better to make lock handling in
find_partition_replica_identity_indexes() consistent
(relation_open(..., NoLock) if child is already locked, and avoid
mixed relation_close(..., lockmode)/NoLock behavior).That’s because if we are going to update a partition, then we need to hold the lock on the partition.
There is one locking cleanup in find_partition_replica_identity_indexes().
find_inheritance_children(relId, lockmode) already acquires lockmode on
every partition it returns, so I think the later relation_open() should use
NoLock, not lockmode. For the same reason, all relation_close() calls in
this function should use NoLock as well.Today the code does:
partRel =relation_open(partRelOid, lockmode);
...
relation_close(partRel, lockmode);That does not cause a correctness issue, because the lock manager
reference-counts same-transaction acquisitions, so the lock remains held
either way. But it is misleading: it suggests that relation_open() is where
the partition lock is taken, and that the early relation_close(..., lockmode)
is intentionally releasing it. Neither is actually true here, because the lock
was already acquired by find_inheritance_children().So I think this should be adjusted to:
partRel = relation_open(partRelOid, NoLock);
and all close sites in this function should be:
relation_close(partRel, NoLock);
The comment on the early-close path should also be updated, since it is not
really unlocking the partition. Something like "No matching partition index;
just close the relcache entry" would match the actual behavior better.
Okay, in find_partition_replica_identity_indexes, we can use NOLOCK to open partitions as they have been locked by find_inheritance_children. But for those partitions that we won’t touch, we still want to unlock them.
PFA v7.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v7-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patchapplication/octet-stream; name=v7-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch; x-unix-mode=0644Download+227-30
On Tue, Mar 24, 2026 at 3:26 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Mar 23, 2026, at 16:41, Xuneng Zhou <xunengzhou@gmail.com> wrote:
Hi,
On Mon, Mar 23, 2026 at 3:57 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Mar 21, 2026, at 18:29, Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Thu, Mar 19, 2026 at 1:07 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Feb 26, 2026, at 14:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 28, 2026, at 10:49, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 16:30, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:39, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.I am slightly confused by the tests included in the proposed patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass.Oops, that isn’t supposed to be so. I’ll check the test.
Okay, I see the problem is here:
```
+CREATE UNIQUE INDEX test_replica_identity_partitioned_pkey ON test_replica_identity_partitioned (id);
```I missed to add column “val” into the index, so that alter type of val didn’t cause index rebuild.
Ideally, it’s better to also verify that index OIDs should have changed before and after alter column type, but I haven’t figured out how to do so. Do you have an idea?
I just updated the test to store index OIDs before and after rebuild into 2 temp tables, so that we can compare the OIDs to verify rebuild happens and replica identity preserved.
I tried to port the test to master branch, and the test failed. From the test diff file, we can see replica identity lost on 3 leaf partitions: ``` @@ -360,9 +360,9 @@ ORDER BY b.index_name; index_name | rebuilt | ri_lost ---------------------------------------------------+---------+--------- - test_replica_identity_partitioned_p1_id_val_idx | t | f - test_replica_identity_partitioned_p2_1_id_val_idx | t | f - test_replica_identity_partitioned_p2_2_id_val_idx | t | f + test_replica_identity_partitioned_p1_id_val_idx | t | t + test_replica_identity_partitioned_p2_1_id_val_idx | t | t + test_replica_identity_partitioned_p2_2_id_val_idx | t | t test_replica_identity_partitioned_p2_id_val_idx | t | f test_replica_identity_partitioned_pkey | t | f (5 rows) ```With this patch, the test passes and all replica identity are preserved.
PFA v3:
* Enhanced the test.
* A small change in find_partition_replica_identity_indexes(): if we will not update a partition, then unlock it.Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v3-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch>
The CF asked for a rebase, thus rebased as v4.
Hi, I reproduced this with the test case, and the patch appears
to resolve it.Some comments on v5:
Thanks a lot for your review.
-- Whether it makes sense to use a single list of pair structs instead
of two parallel OID lists (replicaIdentityIndexOids +
replicaIdentityTableOids) to avoid accidental desync.I don’t think that helps much. The current code of rebuilding index uses two lists changedIndexOids and changedIndexDefs. So, this patch matches the pattern of the existing code.
-- It would be better to make lock handling in
find_partition_replica_identity_indexes() consistent
(relation_open(..., NoLock) if child is already locked, and avoid
mixed relation_close(..., lockmode)/NoLock behavior).That’s because if we are going to update a partition, then we need to hold the lock on the partition.
There is one locking cleanup in find_partition_replica_identity_indexes().
find_inheritance_children(relId, lockmode) already acquires lockmode on
every partition it returns, so I think the later relation_open() should use
NoLock, not lockmode. For the same reason, all relation_close() calls in
this function should use NoLock as well.Today the code does:
partRel =relation_open(partRelOid, lockmode);
...
relation_close(partRel, lockmode);That does not cause a correctness issue, because the lock manager
reference-counts same-transaction acquisitions, so the lock remains held
either way. But it is misleading: it suggests that relation_open() is where
the partition lock is taken, and that the early relation_close(..., lockmode)
is intentionally releasing it. Neither is actually true here, because the lock
was already acquired by find_inheritance_children().So I think this should be adjusted to:
partRel = relation_open(partRelOid, NoLock);
and all close sites in this function should be:
relation_close(partRel, NoLock);
The comment on the early-close path should also be updated, since it is not
really unlocking the partition. Something like "No matching partition index;
just close the relcache entry" would match the actual behavior better.Okay, in find_partition_replica_identity_indexes, we can use NOLOCK to open partitions as they have been locked by find_inheritance_children. But for those partitions that we won’t touch, we still want to unlock them.
PFA v7.
v7 LGTM.
--
Best,
Xuneng
On Apr 6, 2026, at 14:04, Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Tue, Mar 24, 2026 at 3:26 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Mar 23, 2026, at 16:41, Xuneng Zhou <xunengzhou@gmail.com> wrote:
Hi,
On Mon, Mar 23, 2026 at 3:57 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Mar 21, 2026, at 18:29, Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Thu, Mar 19, 2026 at 1:07 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Feb 26, 2026, at 14:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 28, 2026, at 10:49, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 16:30, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:39, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.I am slightly confused by the tests included in the proposed patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass.Oops, that isn’t supposed to be so. I’ll check the test.
Okay, I see the problem is here:
```
+CREATE UNIQUE INDEX test_replica_identity_partitioned_pkey ON test_replica_identity_partitioned (id);
```I missed to add column “val” into the index, so that alter type of val didn’t cause index rebuild.
Ideally, it’s better to also verify that index OIDs should have changed before and after alter column type, but I haven’t figured out how to do so. Do you have an idea?
I just updated the test to store index OIDs before and after rebuild into 2 temp tables, so that we can compare the OIDs to verify rebuild happens and replica identity preserved.
I tried to port the test to master branch, and the test failed. From the test diff file, we can see replica identity lost on 3 leaf partitions: ``` @@ -360,9 +360,9 @@ ORDER BY b.index_name; index_name | rebuilt | ri_lost ---------------------------------------------------+---------+--------- - test_replica_identity_partitioned_p1_id_val_idx | t | f - test_replica_identity_partitioned_p2_1_id_val_idx | t | f - test_replica_identity_partitioned_p2_2_id_val_idx | t | f + test_replica_identity_partitioned_p1_id_val_idx | t | t + test_replica_identity_partitioned_p2_1_id_val_idx | t | t + test_replica_identity_partitioned_p2_2_id_val_idx | t | t test_replica_identity_partitioned_p2_id_val_idx | t | f test_replica_identity_partitioned_pkey | t | f (5 rows) ```With this patch, the test passes and all replica identity are preserved.
PFA v3:
* Enhanced the test.
* A small change in find_partition_replica_identity_indexes(): if we will not update a partition, then unlock it.Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v3-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch>
The CF asked for a rebase, thus rebased as v4.
Hi, I reproduced this with the test case, and the patch appears
to resolve it.Some comments on v5:
Thanks a lot for your review.
-- Whether it makes sense to use a single list of pair structs instead
of two parallel OID lists (replicaIdentityIndexOids +
replicaIdentityTableOids) to avoid accidental desync.I don’t think that helps much. The current code of rebuilding index uses two lists changedIndexOids and changedIndexDefs. So, this patch matches the pattern of the existing code.
-- It would be better to make lock handling in
find_partition_replica_identity_indexes() consistent
(relation_open(..., NoLock) if child is already locked, and avoid
mixed relation_close(..., lockmode)/NoLock behavior).That’s because if we are going to update a partition, then we need to hold the lock on the partition.
There is one locking cleanup in find_partition_replica_identity_indexes().
find_inheritance_children(relId, lockmode) already acquires lockmode on
every partition it returns, so I think the later relation_open() should use
NoLock, not lockmode. For the same reason, all relation_close() calls in
this function should use NoLock as well.Today the code does:
partRel =relation_open(partRelOid, lockmode);
...
relation_close(partRel, lockmode);That does not cause a correctness issue, because the lock manager
reference-counts same-transaction acquisitions, so the lock remains held
either way. But it is misleading: it suggests that relation_open() is where
the partition lock is taken, and that the early relation_close(..., lockmode)
is intentionally releasing it. Neither is actually true here, because the lock
was already acquired by find_inheritance_children().So I think this should be adjusted to:
partRel = relation_open(partRelOid, NoLock);
and all close sites in this function should be:
relation_close(partRel, NoLock);
The comment on the early-close path should also be updated, since it is not
really unlocking the partition. Something like "No matching partition index;
just close the relcache entry" would match the actual behavior better.Okay, in find_partition_replica_identity_indexes, we can use NOLOCK to open partitions as they have been locked by find_inheritance_children. But for those partitions that we won’t touch, we still want to unlock them.
PFA v7.
v7 LGTM.
--
Best,
Xuneng
Rebased as v8. Nothing changed.
Per [1]/messages/by-id/9dd5b2f4-fba6-416b-b732-45f284d4097b@eisentraut.org, to participate Peter E.’s “in-person commitfest” session, I am adding tag “PGConf.dev” to the CF entry: https://commitfest.postgresql.org/patch/6440/
[1]: /messages/by-id/9dd5b2f4-fba6-416b-b732-45f284d4097b@eisentraut.org
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Apr 14, 2026, at 11:13, Chao Li <li.evan.chao@gmail.com> wrote:
On Apr 6, 2026, at 14:04, Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Tue, Mar 24, 2026 at 3:26 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Mar 23, 2026, at 16:41, Xuneng Zhou <xunengzhou@gmail.com> wrote:
Hi,
On Mon, Mar 23, 2026 at 3:57 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Mar 21, 2026, at 18:29, Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Thu, Mar 19, 2026 at 1:07 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Feb 26, 2026, at 14:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 28, 2026, at 10:49, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 16:30, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Jan 27, 2026, at 15:39, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.I am slightly confused by the tests included in the proposed patch.
On HEAD, if I undo the proposed changes of tablecmds.c, the tests
pass. If I run the tests of the patch with the changes of
tablecmds.c, the tests also pass.Oops, that isn’t supposed to be so. I’ll check the test.
Okay, I see the problem is here:
```
+CREATE UNIQUE INDEX test_replica_identity_partitioned_pkey ON test_replica_identity_partitioned (id);
```I missed to add column “val” into the index, so that alter type of val didn’t cause index rebuild.
Ideally, it’s better to also verify that index OIDs should have changed before and after alter column type, but I haven’t figured out how to do so. Do you have an idea?
I just updated the test to store index OIDs before and after rebuild into 2 temp tables, so that we can compare the OIDs to verify rebuild happens and replica identity preserved.
I tried to port the test to master branch, and the test failed. From the test diff file, we can see replica identity lost on 3 leaf partitions: ``` @@ -360,9 +360,9 @@ ORDER BY b.index_name; index_name | rebuilt | ri_lost ---------------------------------------------------+---------+--------- - test_replica_identity_partitioned_p1_id_val_idx | t | f - test_replica_identity_partitioned_p2_1_id_val_idx | t | f - test_replica_identity_partitioned_p2_2_id_val_idx | t | f + test_replica_identity_partitioned_p1_id_val_idx | t | t + test_replica_identity_partitioned_p2_1_id_val_idx | t | t + test_replica_identity_partitioned_p2_2_id_val_idx | t | t test_replica_identity_partitioned_p2_id_val_idx | t | f test_replica_identity_partitioned_pkey | t | f (5 rows) ```With this patch, the test passes and all replica identity are preserved.
PFA v3:
* Enhanced the test.
* A small change in find_partition_replica_identity_indexes(): if we will not update a partition, then unlock it.Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v3-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch>
The CF asked for a rebase, thus rebased as v4.
Hi, I reproduced this with the test case, and the patch appears
to resolve it.Some comments on v5:
Thanks a lot for your review.
-- Whether it makes sense to use a single list of pair structs instead
of two parallel OID lists (replicaIdentityIndexOids +
replicaIdentityTableOids) to avoid accidental desync.I don’t think that helps much. The current code of rebuilding index uses two lists changedIndexOids and changedIndexDefs. So, this patch matches the pattern of the existing code.
-- It would be better to make lock handling in
find_partition_replica_identity_indexes() consistent
(relation_open(..., NoLock) if child is already locked, and avoid
mixed relation_close(..., lockmode)/NoLock behavior).That’s because if we are going to update a partition, then we need to hold the lock on the partition.
There is one locking cleanup in find_partition_replica_identity_indexes().
find_inheritance_children(relId, lockmode) already acquires lockmode on
every partition it returns, so I think the later relation_open() should use
NoLock, not lockmode. For the same reason, all relation_close() calls in
this function should use NoLock as well.Today the code does:
partRel =relation_open(partRelOid, lockmode);
...
relation_close(partRel, lockmode);That does not cause a correctness issue, because the lock manager
reference-counts same-transaction acquisitions, so the lock remains held
either way. But it is misleading: it suggests that relation_open() is where
the partition lock is taken, and that the early relation_close(..., lockmode)
is intentionally releasing it. Neither is actually true here, because the lock
was already acquired by find_inheritance_children().So I think this should be adjusted to:
partRel = relation_open(partRelOid, NoLock);
and all close sites in this function should be:
relation_close(partRel, NoLock);
The comment on the early-close path should also be updated, since it is not
really unlocking the partition. Something like "No matching partition index;
just close the relcache entry" would match the actual behavior better.Okay, in find_partition_replica_identity_indexes, we can use NOLOCK to open partitions as they have been locked by find_inheritance_children. But for those partitions that we won’t touch, we still want to unlock them.
PFA v7.
v7 LGTM.
--
Best,
XunengRebased as v8. Nothing changed.
Per [1], to participate Peter E.’s “in-person commitfest” session, I am adding tag “PGConf.dev” to the CF entry: https://commitfest.postgresql.org/patch/6440/
[1] /messages/by-id/9dd5b2f4-fba6-416b-b732-45f284d4097b@eisentraut.org
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v8-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch>
PFA v9:
* Rebased to resolve new conflicts
* Fixed a missed case where a partitioned table itself doesn’t use index as RI but some partitions use.
* Simplified the new tests by using \gset to avoid creating temp tables.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v9-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patchapplication/octet-stream; name=v9-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch; x-unix-mode=0644Download+272-39
On Thu Jul 16, 2026 at 4:49 AM CEST, Chao Li wrote:
On Tue, Jan 27, 2026 at 01:13:32PM +0800, Chao Li wrote:
I found this bug while working on a related patch [1].
When ALTER TABLE ... ALTER COLUMN TYPE causes an index rebuild, and
that index is used as REPLICA IDENTITY on a partitioned table, the
replica identity marking on partitions can be silently lost after the
rebuild.
I was going to review the patch, but I am not sure I understand the root
cause.
I tried to reproduce the problem with inheritance, which I would expect
to behave the same as partitions in this case:
create table p (a int not null, b int not null);
create table c () inherits (p);
create unique index p_b_idx on p(b);
create unique index c_b_idx on c(b);
alter table p replica identity using index p_b_idx;
alter table c replica identity using index c_b_idx;
Surprisingly though, the replica identity is not lost in this case:
alter table p alter b type bigint;
select c.relname as index_name, c.oid as index_oid, i.indisreplident
from pg_class c join pg_index i on c.oid = i.indexrelid
where (c.relname in ('p_b_idx', 'c_b_idx'));
index_name | index_oid | indisreplident
------------+-----------+----------------
p_b_idx | 24773 | t
c_b_idx | 24774 | t
For both inheritance and partitions, RememberAllDependentForRebuilding()
is called twice, first for the parent and then for the child table.
In the inheritance case, RememberAllDependentForRebuilding() finds a
dependency on the respective index both for the parent and the child
table. For each of the indexes, it calls RememberIndexForRebuilding()
which calls RememberReplicaIdentityForRebuilding().
In the partitions case though, RememberAllDependentForRebuilding does
not find a dependency on the index when it gets called on the child
table (partition).
I think what's happening here is that ATPostAlterTypeCleanup()
is invoked on the parent first: it tries to drop the index (which will
be recreated later in phase 3) using performMultipleDeletions(), which
will call findDependentObjects() and drop those too.
For partitions, I see a dependency of type 'P'
(DEPENDENCY_PARTITION_PRI) which is not there for plain inheritance.
select c1.relname as obj_name,
c2.relname as refobj_name,
d.deptype
from pg_depend d
join pg_class c1 on c1.oid = d.objid
join pg_class c2 on c2.oid = d.refobjid
where refobjid = 'part_a_b_idx'::regclass;
obj_name | refobj_name | deptype
-----------------+--------------+---------
part_p1_a_b_idx | part_a_b_idx | P
Could this be the reason why the partition index is being dropped
so early, thus never getting a chance to be recorded as "restore replica
identity"?
Seeing how things work for inheritance, if we could somehow avoid this
drop, the index might be dropped/rebuilt correctly later, when the
command recurses to the child partition.
For completeness, my partitioned setup for the queries above was:
create table part (a int not null, b int not null) partition by list(a);
create table part_p1 partition of part for values in (1);
create unique index part_a_b_idx on part(a, b);
alter table part replica identity using index part_a_b_idx;
alter table part_p1 replica identity using index part_p1_a_b_idx;
Regards,
Alberto
--
Alberto Piai
Sensational AG
Zürich, Switzerland
Hi,
I spent some time looking at this, and I think there are still some
issues with v9.
For example:
```
CREATE TABLE p (id int NOT NULL, val int NOT NULL)
PARTITION BY RANGE (id);
CREATE TABLE c1 (id int NOT NULL, val int NOT NULL);
CREATE UNIQUE INDEX my_custom_ri ON c1 (id, val);
ALTER TABLE c1 REPLICA IDENTITY USING INDEX my_custom_ri;
CREATE UNIQUE INDEX p_idx ON p (id, val);
ALTER TABLE p ATTACH PARTITION c1
FOR VALUES FROM (0) TO (100);
ALTER TABLE p ALTER COLUMN val TYPE bigint;
ERROR: index "my_custom_ri" for table "c1" does not exist
```
This is because generateClonedIndexStmt() sets idxname = NULL when building
the partition indexes.
```
IndexStmt *
generateClonedIndexStmt(RangeVar *heapRel, Relation source_idx, ...
/*
* We don't try to preserve the name of the source index; instead,
* just let DefineIndex() choose a reasonable name. (If we tried to
* preserve the name, we'd get duplicate-relation-name failures
* unless the source table was in a different schema.)
*/
index->idxname = NULL;
```
This causes ChooseIndexName() to generate a new default name. v9 then
tries to restore replica identity by the old name, which no longer
exists.
This index name-change behavior during ALTER TABLE ... ALTER COLUMN
TYPE is existing and undocumented, and I would argue is wrong. The
user expects index names to be stable at the end of this operation.
CREATE TABLE ... LIKE (INCLUDING INDEXES) is a case where we should not
create an index with the same name as the source and choosing a default name
makes sense, but it should not apply to all callers.
Perhaps callers like ALTER TABLE ... ALTER COLUMN TYPE should track
the old index names and be allowed to pass them to generateClonedIndexStmt?
That sounds like a broader improvement, and one that the changes in v9
can inherit.
What do you think?
The current v9 tests happen to work only because they use default names that
ChooseIndexName() re-generates the same way.
Also, this issue is not limited to replica identity. CLUSTER ON does
not copy/restore indisclustered on partition indexes at all, and this
should be addressed as well, right?
--
Sami Imseih
Amazon Web Services (AWS)
Also, this issue is not limited to replica identity. CLUSTER ON does
not copy/restore indisclustered on partition indexes at all, and this
should be addressed as well, right?
Robert (cc'd) reminded me offline that REPACK ... USING INDEX ==
CLUSTER ON.
Here is a repro, and CLUSTER ON also shows the same behavior.
```
postgres=# CREATE TABLE test_cluster_multi (a int NOT NULL, b int NOT
NULL) PARTITION BY RANGE (a);
CREATE TABLE
postgres=# CREATE TABLE test_cluster_multi_p1 PARTITION OF test_cluster_multi
postgres-# FOR VALUES FROM (0) TO (100);
CREATE TABLE
postgres=# CREATE INDEX test_cluster_multi_idx ON test_cluster_multi (a, b);
CREATE INDEX
postgres=# CREATE INDEX test_cluster_multi_other ON test_cluster_multi (b);
CREATE INDEX
postgres=# --CLUSTER test_cluster_multi_p1 USING test_cluster_multi_p1_a_b_idx;
postgres=# REPACK test_cluster_multi_p1 USING INDEX
test_cluster_multi_p1_a_b_idx;
REPACK
postgres=# SELECT tc.relname AS table_name, ic.relname AS index_name,
i.indisclustered
postgres-# FROM pg_index i
postgres-# JOIN pg_class tc ON i.indrelid = tc.oid
postgres-# JOIN pg_class ic ON i.indexrelid = ic.oid
postgres-# WHERE tc.relname = 'test_cluster_multi_p1'
postgres-# ORDER BY ic.relname;
table_name | index_name | indisclustered
-----------------------+-------------------------------+----------------
test_cluster_multi_p1 | test_cluster_multi_p1_a_b_idx | t
test_cluster_multi_p1 | test_cluster_multi_p1_b_idx | f
(2 rows)
postgres=# ALTER TABLE test_cluster_multi ALTER COLUMN b TYPE bigint;
ALTER TABLE
postgres=# SELECT tc.relname AS table_name, ic.relname AS index_name,
i.indisclustered
postgres-# FROM pg_index i
postgres-# JOIN pg_class tc ON i.indrelid = tc.oid
postgres-# JOIN pg_class ic ON i.indexrelid = ic.oid
postgres-# WHERE tc.relname = 'test_cluster_multi_p1'
postgres-# ORDER BY ic.relname;
table_name | index_name | indisclustered
-----------------------+-------------------------------+----------------
test_cluster_multi_p1 | test_cluster_multi_p1_a_b_idx | f
test_cluster_multi_p1 | test_cluster_multi_p1_b_idx | f
(2 rows)
postgres=# DROP TABLE test_cluster_multi;
DROP TABLE
```
--
Sami Imseih
Amazon Web Services (AWS)
On Tue Jul 28, 2026 at 1:11 AM CEST, Sami Imseih wrote:
Also, this issue is not limited to replica identity. CLUSTER ON does
not copy/restore indisclustered on partition indexes at all, and this
should be addressed as well, right?Robert (cc'd) reminded me offline that REPACK ... USING INDEX ==
CLUSTER ON.
Hi Sami,
have you seen my email from a few days ago?
/messages/by-id/DK5H6VM4U4J4.C8APP2ZH2CAT@gmail.com
I'm pretty sure the root cause is not addressed by patch v9.
If my analysis is correct (RememberIndexForRebuilding not being called
on the child index because of an earlier drop), that would also explain
the issues with CLUSTER (As RememberIndexForRebuilding calls both
RememberReplicaIdentityForRebuilding and
RememberClusterOnForRebuilding).
In that case, rather than tracking and restoring more state, I think a
better fix would be to make sure that ATPostAlterTypeCleanup doesn't
find the child table in a half-cleaned-up state. Then
RememberIndexForRebuilding would be called and the replica identity and
cluster state would be restored correctly.
Regards,
Alberto
--
Alberto Piai
Sensational AG
Zürich, Switzerland
On Jul 28, 2026, at 14:50, Alberto Piai <alberto.piai@gmail.com> wrote:
On Tue Jul 28, 2026 at 1:11 AM CEST, Sami Imseih wrote:
Also, this issue is not limited to replica identity. CLUSTER ON does
not copy/restore indisclustered on partition indexes at all, and this
should be addressed as well, right?Robert (cc'd) reminded me offline that REPACK ... USING INDEX ==
CLUSTER ON.Hi Sami,
have you seen my email from a few days ago?
/messages/by-id/DK5H6VM4U4J4.C8APP2ZH2CAT@gmail.com
I'm pretty sure the root cause is not addressed by patch v9.
If my analysis is correct (RememberIndexForRebuilding not being called
on the child index because of an earlier drop), that would also explain
the issues with CLUSTER (As RememberIndexForRebuilding calls both
RememberReplicaIdentityForRebuilding and
RememberClusterOnForRebuilding).In that case, rather than tracking and restoring more state, I think a
better fix would be to make sure that ATPostAlterTypeCleanup doesn't
find the child table in a half-cleaned-up state. Then
RememberIndexForRebuilding would be called and the replica identity and
cluster state would be restored correctly.
Hi Alberto,
Thanks for your review and suggestion. I think your analysis is correct, and your suggested direction sounds reasonable. However, the current ALTER TABLE infra doesn't seem well suited to it. ATPostAlterTypeCleanup() for the parent calls performMultipleDeletions(), which also deletes the dependent child partition indexes, while DefineIndex() for the parent automatically recreates the parent index and all child indexes.
Your direction would allow each child partition to remember its own index properties so that they can be restored afterward. I can see two possible approaches:
1) Make the parent and each child responsible for deleting and rebuilding their own indexes.
2) Move performMultipleDeletions() out of ATPostAlterTypeCleanup() to a later stage, after all children have recorded their dependent indexes, and coordinate recreation to avoid rebuilding child indexes twice.
I’m afraid both approaches would require significant refactoring of the current ALTER TABLE infra, which would go beyond the scope of this patch, as it only aims to fix a bug.
If we want to pursue the direction you suggested, I think that would be better handled as a separate refactoring patch.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Jul 28, 2026, at 06:10, Sami Imseih <samimseih@gmail.com> wrote:
Hi,
I spent some time looking at this, and I think there are still some
issues with v9.
Hi Sami, thank you very much for reviewing.
For example:
```
CREATE TABLE p (id int NOT NULL, val int NOT NULL)
PARTITION BY RANGE (id);
CREATE TABLE c1 (id int NOT NULL, val int NOT NULL);
CREATE UNIQUE INDEX my_custom_ri ON c1 (id, val);
ALTER TABLE c1 REPLICA IDENTITY USING INDEX my_custom_ri;
CREATE UNIQUE INDEX p_idx ON p (id, val);
ALTER TABLE p ATTACH PARTITION c1
FOR VALUES FROM (0) TO (100);
ALTER TABLE p ALTER COLUMN val TYPE bigint;
ERROR: index "my_custom_ri" for table "c1" does not exist
```This is because generateClonedIndexStmt() sets idxname = NULL when building
the partition indexes.```
IndexStmt *
generateClonedIndexStmt(RangeVar *heapRel, Relation source_idx, ...
/*
* We don't try to preserve the name of the source index; instead,
* just let DefineIndex() choose a reasonable name. (If we tried to
* preserve the name, we'd get duplicate-relation-name failures
* unless the source table was in a different schema.)
*/
index->idxname = NULL;
```This causes ChooseIndexName() to generate a new default name. v9 then
tries to restore replica identity by the old name, which no longer
exists.This index name-change behavior during ALTER TABLE ... ALTER COLUMN
TYPE is existing and undocumented, and I would argue is wrong. The
user expects index names to be stable at the end of this operation.
Agreed. So, this is a separate bug.
CREATE TABLE ... LIKE (INCLUDING INDEXES) is a case where we should not
create an index with the same name as the source and choosing a default name
makes sense, but it should not apply to all callers.Perhaps callers like ALTER TABLE ... ALTER COLUMN TYPE should track
the old index names and be allowed to pass them to generateClonedIndexStmt?
That sounds like a broader improvement, and one that the changes in v9
can inherit.
What do you think?The current v9 tests happen to work only because they use default names that
ChooseIndexName() re-generates the same way.Also, this issue is not limited to replica identity. CLUSTER ON does
not copy/restore indisclustered on partition indexes at all, and this
should be addressed as well, right?
I didn’t notice that issue.
PFA v10:
0001- preserve index name for partition indexes
0002 - preserve replica identity and cluster on index
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
