Re: Fuzzy thinking in is_publishable_class

On 09/05/2019 04:37, Tom Lane wrote:
> I wrote:
>> is_publishable_class has a test "relid >= FirstNormalObjectId",
>> which I think we should drop, for two reasons:
>> ...
>> So what is the motivation for this test?  If there's an important
>> reason for it, we need to find a less fragile way to express it.
> 
> I tried removing the FirstNormalObjectId check, and found that the
> reason for it seems to be "the subscription/t/004_sync.pl test
> falls over without it".  That's because that test supposes that
> the *only* entry in pg_subscription_rel will be for the test table
> that it creates.  Without the FirstNormalObjectId check, the
> information_schema relations also show up in pg_subscription_rel,
> confusing the script's simplistic status check.
> 
> I'm of two minds what to do about that.  One approach is to just
> define a "FOR ALL TABLES" publication as including the information_schema
> tables, in which case 004_sync.pl is wrong and we should fix it by
> adding a suitable WHERE restriction to its pg_subscription_rel check.
> However, possibly that would break some applications that are likewise
> assuming that no built-in tables appear in pg_subscription_rel.
> 

I was and still am worried that including information_schema in "FOR ALL
TABLES" will result in breakage or at least unexpected behavior in case
user adjusts anything in the information_schema catalogs.

IMHO only user created tables should be part of "FOR ALL TABLES" hence
the FirstNormalObjectId check.

The fact that information_schema can be recreated and is not considered
system catalog by some commands but kind of is by others is more problem
of how we added information_schema and it's definitely not ideal, we
should either consider it system schema like pg_catalog is or consider
it everywhere an user catalog. For me the latter makes little sense
given that it comes with the database.

> But, if what we want is the definition that "information_schema is
> excluded from publishable tables", I'm not satisfied with this
> implementation of that rule.  Dropping/recreating information_schema
> would cause the behavior to change.  We could, at the cost of an
> additional syscache lookup, check the name of the schema that a
> potentially publishable table belongs to and exclude information_schema
> by name.  I don't have much idea about how performance-critical
> is_publishable_class is, so I don't know how acceptable that seems.
> 

I think we need a better way of identifying what's part of system and
what's user created in general. The FirstNormalObjectId seems somewhat
okay approximation, but then we have plenty of other ways for checking,
maybe it's time to consolidate it into some extra column in pg_class?

-- 
Petr Jelinek
2ndQuadrant - PostgreSQL Solutions
https://www.2ndQuadrant.com/