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:
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.
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.
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;
}
3. It's unnecessary to set stattargets = NIL when you'll overwrite it right
after anyway.
props->stattargets = NIL;
...
props->stattargets = GetIndexStatTargets(leafIndexOid);
Best Regards,
Zhijie Hou
