On Sun, May 05, 2019 at 05:45:53PM -0400, Tom Lane wrote:
> In the other place, checking IsSystemNamespace isn't even
> approximately the correct way to proceed, since it fails to reject
> reindexing system catalogs' toast tables.
Good point. I overlooked that part. It is easy enough to have a test
which fails for a catalog index, a catalog table, a toast table on a
system catalog and a toast index on a system catalog. However I don't
see a way to test directly that a toast relation or index on a
non-catalog relation works as we cannot run REINDEX CONCURRENTLY
within a function, and it is not possible to save the toast relation
name as a psql variable. Perhaps somebody has a trick?x
> After looking around a bit, I propose that we invent
> "IsCatalogRelationOid(Oid reloid)" (not wedded to that name), which
> is a wrapper around IsCatalogClass() that does the needful syscache
> lookup for you. Aside from this use-case, it could be used in
> sepgsql/dml.c, which I see is also using
> IsSystemNamespace(get_rel_namespace(oid)) for the wrong thing.
Hmmm. A wrapper on top of IsCatalogClass() implies that we would need
to open the related relation directly in the new function so as it is
possible to grab its pg_class entry. We could imply that the function
takes a ShareLock all the time, but that's not going to be true most
of the time and the recent discussions around lock upgrades stress me
a bit, and I'd rather not add new race conditions or upgrade hazards.
We should have an extra argument with the lock mode, but we have
nothing in catalog.c of that kind, and that does not feel consistent
with the current interface. At the end I have made the choice to not
reinvent the world, and just get a Relation from the parent table when
looking after an index relkind so as IsCatalogRelation() is used for
the check.
What do you think about the updated patch attached? I have removed
the tests from reindex_catalog.sql, and added more coverage into
create_index.sql.
--
Michael