Re: tablecmds: fix bug where index rebuild loses replica identity on partitions - Mailing list pgsql-hackers

> 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,
butI 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
comparethe 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
identitylost 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], 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/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/