Re: Allowing REINDEX to have an optional name

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/