MIME-Version: 1.0
References: <DB533C25-C6BA-4C0F-8046-96168E9CDD72@gmail.com>
 <aXhruFBS8ZZgKolq@paquier.xyz>
 <0B38B9DC-A53C-437C-B9BD-807F30A032BA@gmail.com>
 <F42E6546-E46B-46FE-9C21-30BD289259AE@gmail.com>
 <424A3799-D7FD-4691-BD10-6CBDACB14938@gmail.com>
 <43825F3D-6156-4030-B5E1-9A0841969022@gmail.com>
 <260E4616-43B7-4C24-BF53-352DC8DEB7AA@gmail.com>
In-Reply-To: <260E4616-43B7-4C24-BF53-352DC8DEB7AA@gmail.com>
From: Xuneng Zhou <xunengzhou@gmail.com>
Date: Sat, 21 Mar 2026 18:29:15 +0800
Message-ID: 
 <CABPTF7U_eJ6yeiGQcBPr5SVQS8wvGoo6j7b+6=_Kq3VTk3wSeg@mail.gmail.com>
Subject: Re: tablecmds: fix bug where index rebuild loses replica identity on
 partitions
To: Chao Li <li.evan.chao@gmail.com>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CABPTF7U_eJ6yeiGQcBPr5SVQS8wvGoo6j7b%2B6%3D_Kq3VTk3wSeg%40mail.gmail.com>
Precedence: bulk

On Thu, Mar 19, 2026 at 1:07=E2=80=AFPM Chao Li <li.evan.chao@gmail.com> wr=
ote:
>
>
>
> > 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> wr=
ote:
> >>>>>
> >>>>> 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, an=
d
> >>>>>> 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=E2=80=99t supposed to be so. I=E2=80=99ll check the t=
est.
> >>>>
> >>>
> >>> Okay, I see the problem is here:
> >>> ```
> >>> +CREATE UNIQUE INDEX test_replica_identity_partitioned_pkey ON test_r=
eplica_identity_partitioned (id);
> >>> ```
> >>>
> >>> I missed to add column =E2=80=9Cval=E2=80=9D into the index, so that =
alter type of val didn=E2=80=99t cause index rebuild.
> >>>
> >>> Ideally, it=E2=80=99s better to also verify that index OIDs should ha=
ve changed before and after alter column type, but I haven=E2=80=99t figure=
d out how to do so. Do you have an idea?
> >>
> >> I just updated the test to store index OIDs before and after rebuild i=
nto 2 temp tables, so that we can compare the OIDs to verify rebuild happen=
s and replica identity preserved.
> >>
> >> I tried to port the test to master branch, and the test failed. From t=
he 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 preserve=
d.
> >>
> >> PFA v3:
> >> * Enhanced the test.
> >> * A small change in find_partition_replica_identity_indexes(): if we w=
ill 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