On Tue, Nov 12, 2013 at 12:50 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> Completely agreed. As evidenced by the fact that the current change
> doesn't update all relevant comments & code. I wonder if we shouldn't
> leave the function the current way and just add a new function for the
> new behaviour.
> The hard thing with that would be coming up with a new
> name. IsSystemRelationId() having a different behaviour than
> IsSystemRelation() seems strange to me, so just keeping that and
> adapting the callers seems wrong to me.
> IsInternalRelation()? IsCatalogRelation()?
Well, I went through and looked at the places that were affected by
this and I tend to think that most places will be happier with the new
definition. Picking one at random, consider the calls in cluster.c.
The first is used to set the is_system_catalog flag that is passed to
finish_heap_swap(), which controls whether we queue invalidation
messages after doing the CLUSTER. Well, unless I'm quite mistaken,
user-defined relations in pg_catalog will not have catalog caches and
thus don't need invalidations. The second call in that file is used
to decide whether to warn about inserts or deletes that appear to be
in progress on a table that we have x-locked; that should only apply
to "real" system catalogs, because other things we create in
pg_catalog won't have short-duration locks. (Maybe the
user-catalog-tables patch will modify this test; I'm not sure, but if
this needs to work differently it seems that it should be conditional
on that, not what schema the table lives in.)
If there are call sites that want the existing test, maybe we should
have IsRelationInSystemNamespace() for that, and reserve
IsSystemRelation() for the test as to whether it's a bona fide system
catalog.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company