Subject: Re: tablecmds: fix bug where index rebuild loses replica identity on partitions

From: Chao Li <[email-masked]>
Date: Tue, 27 Jan 2026 16:30:10 +0800

> On Jan 27, 2026, at 15:59, Chao Li <[email-masked]> wrote:
> 
> 
> 
>> On Jan 27, 2026, at 15:39, Michael Paquier <[email-masked]> 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/

---

From: Chao Li <[email-masked]>
Date: Wed, 28 Jan 2026 10:49:45 +0800

> On Jan 27, 2026, at 16:30, Chao Li <[email-masked]> wrote:
> 
> 
> 

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,

---

From: Chao Li <[email-masked]>
Date: Thu, 26 Feb 2026 14:59:58 +0800

> On Jan 28, 2026, at 10:49, Chao Li <[email-masked]> wrote:
> 
> 
> 
> <v3-0001-tablecmds-fix-bug-where-index-rebuild-loses-repli.patch>

The CF asked for a rebase, thus rebased as v4.

Best regards,

---

From: Xuneng Zhou <[email-masked]>
Date: Sat, 21 Mar 2026 18:29:15 +0800

On Thu, Mar 19, 2026 at 1:07 PM Chao Li <[email-masked]> wrote:
>
>
>
> > On Feb 26, 2026, at 14:59, Chao Li <[email-masked]> wrote:
> >
> >
> >

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

---

From: Chao Li <[email-masked]>
Date: Mon, 23 Mar 2026 15:56:25 +0800

> On Mar 21, 2026, at 18:29, Xuneng Zhou <[email-masked]> wrote:
> 

Thanks a lot for your review.

> 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.

> 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,

---

From: Xuneng Zhou <[email-masked]>
Date: Mon, 23 Mar 2026 16:41:53 +0800

Hi,

On Mon, Mar 23, 2026 at 3:57 PM Chao Li <[email-masked]> wrote:
> >>>>> 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
> >>>> * A small change in find_partition_replica_identity_indexes(): if we
will not update a partition, then unlock it.
> 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.
> 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.

-- 
Best,
Xuneng

---

From: Chao Li <[email-masked]>
Date: Tue, 24 Mar 2026 15:25:44 +0800

> On Mar 23, 2026, at 16:41, Xuneng Zhou <[email-masked]> wrote:
> 
> Hi,
> 
>   every partition it returns, so I think the later relation_open() should use
>   partRel =relation_open(partRelOid, lockmode);
>   ...
>   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
>   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;

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,

---

From: Xuneng Zhou <[email-masked]>
Date: Mon, 6 Apr 2026 14:04:14 +0800

On Tue, Mar 24, 2026 at 3:26 PM Chao Li <[email-masked]> wrote:
> > > >>>> I tried to port the test to master branch, and the test failed.

---

From: Chao Li <[email-masked]>
Date: Tue, 14 Apr 2026 11:13:40 +0800

> On Apr 6, 2026, at 14:04, Xuneng Zhou <[email-masked]> wrote:
> 
> v7 LGTM.

Rebased 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] https://postgr.es/m/[email-masked]

Best regards,