On Tue, Nov 12, 2013 at 12:50 PM, Andres Freund <and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers