> On Aug 27, 2026, at 11:42, Zhijie Hou (Fujitsu) <[email protected]> > wrote: > > > Hi, > > On Wednesday, August 26, 2026 1:30 PM Chao Li <[email protected]> wrote: >> I reviewed v14 and found that a leaf index’s tablespace can also be lost and >> needs to be restored. I fixed that in v15-0002. >> >> I also addressed Sami’s review comments in v15, except for the hash-table >> suggestion. I tried using an HTAB, but ran into several build failures. I >> didn’t >> want to spend more time on it, so I gave up on that approach. > > I have few thoughts for the patch:
Hi Zhijie,
Thank you very much for the review.
>
> 1.
>
> I noticed that the code updating idxisclustered and idxisreplident looks a bit
> unusual to me. These updates are placed inside DefineIndex(), where a fresh
> pg_index and pg_class row are already being created for the new index. It
> seems
> a bit confusing that the patch also performs post-updates for these rows
> within
> the same function.
>
> Perhaps we could add a comment explaining why these values can't be set
> directly
> when creating the new rows - that would help future readers understand the
> reasoning.
Updated the comment in v15.
>
> I'm also concerned that updating the flag this way won't fire the
> ObjectPostAlterHook (as Sami also mentioned). I think some analysis or
> comments
> are worth adding here.
Added the hook call in v15.
>
> Few nits:
>
> 2. I think this should report an ERROR, since we don't expect the tuple to be
> concurrently updated, IIUC.
>
> classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(leafIndexOid));
> if (!HeapTupleIsValid(classtup))
> {
> ReleaseSysCache(idxtup);
> continue;
> }
Agreed and fixed in v15.
>
> 3. It's unnecessary to set stattargets = NIL when you'll overwrite it right
> after anyway.
>
> props->stattargets = NIL;
> ...
> props->stattargets = GetIndexStatTargets(leafIndexOid);
>
Agreed and fixed in v15.
BTW, I have run a check-world test with v15.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
v15-0001-Preserve-index-per-column-statistics-targets-acr.patch
Description: Binary data
v15-0002-Preserve-leaf-partition-index-properties-across-.patch
Description: Binary data
