On Sun, 17 Jul 2022 at 07:19, Michael Paquier <michael@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/