> On Aug 27, 2026, at 13:38, Chao Li <[email protected]> wrote: > > > >> 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><v15-0002-Preserve-leaf-partition-index-properties-across-.patch>
Rebased to v16 due to a conflict in typedefs.list introduced by 7612ae8ed8c941985d9cbf60a0674c48f5b0abe5. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
v16-0001-Preserve-index-per-column-statistics-targets-acr.patch
Description: Binary data
v16-0002-Preserve-leaf-partition-index-properties-across-.patch
Description: Binary data
