On Sun, 2022-12-04 at 13:09 -0600, Justin Pryzby wrote: > > This beavior is fixed. I re-factored and re-implented to use > DefineIndex() for building indexes concurrently rather than > reindexing. > That makes the patch smaller, actually, and has the added benefit of > splitting off the "Concurrently" part of DefineIndex() into a > separate > function.
Nice, I think it turned out pretty concise. I played around with the patch quite a bit, didn't find any major problems, the only minor thing that I can note is that we should skip the top parent index itself in the loop not to increment the pg_stat counter, something like this: diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index cfab45b999..9049540b5b 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -1515,6 +1515,9 @@ DefineIndex(Oid relationId, Oid indrelid = lfirst_oid(lc); Oid tabrelid = IndexGetRelation(indrelid, false); + if (indrelid == indexRelationId) + continue; + if (RELKIND_HAS_STORAGE(get_rel_relkind(indrelid)) && !get_index_isvalid(indrelid)) { > > BTW, it causes the patch to fail to apply in cfbot when you send an > additional (002) supplementary patch without including the original > (001) patch. You can name it *.txt to avoid the issue. > > https://wiki.postgresql.org/wiki/Cfbot#Which_attachments_are_considered_to_be_patches.3F > > Thanks for looking. > My bad, didn't know about this, thanks for the link. On a side note, I noticed that reindex behaviour is strange on partitioned tables, it doesn't mark partitioned tables as valid after reindexing children, as I could understand from the code and mailing lists, this is the intended behaviour, but I can't quite understand the rationale for it, do you know why it is done this way?