Re: Allowing REINDEX to have an optional name

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?
--
Michael