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