On Sun, 17 Jul 2022 at 07:19, Michael Paquier <mich...@paquier.xyz> wrote: > > On Fri, Jul 15, 2022 at 06:21:22PM +0100, Simon Riggs wrote: > > That's fixed it on the CFbot. Over to you, Michael. Thanks. > > Sure. I have looked over that, and this looks fine overall. I have > made two changes though. > > if (objectKind == REINDEX_OBJECT_SYSTEM && > - !IsSystemClass(relid, classtuple)) > + !IsCatalogRelationOid(relid)) > + continue; > + else if (objectKind == REINDEX_OBJECT_DATABASE && > + IsCatalogRelationOid(relid)) > > The patch originally relied on IsSystemClass() to decide if a relation > is a catalog table or not. This is not wrong in itself because > ReindexMultipleTables() discards RELKIND_TOAST a couple of lines > above, but I think that we should switch to IsCatalogRelationOid() as > that's the line drawn to check for the catalog-ness of a relation. > > The second thing is test coverage. Using a REINDEX DATABASE/SYSTEM > within the main regression test suite is not a good idea, but we > already have those commands running in the reindexdb suite so I could > not resist expanding the test section to track and check relfilenode > changes through four relations for these cases: > - Catalog index. > - Catalog toast index. > - User table index. > - User toast index. > The relfilenodes of those relations are saved in a table and > cross-checked with the contents of pg_class after each REINDEX, on > SYSTEM or DATABASE. There are no new heavy commands, so it does not > make the test longer. > > With all that, I finish with the attached. Does that look fine to > you?
Sounds great, looks fine. Thanks for your review. -- Simon Riggs http://www.EnterpriseDB.com/
