Re: erroneous restore into pg_catalog schema
Re: erroneous restore into pg_catalog schema
От:
Alvaro Herrera <alvherre@2ndquadrant.com>
Дата:
Tom Lane wrote: > Alvaro Herrera writes: > > Tom Lane escribi�: > >> I will bet that this is more breakage from the DDL-code refactoring that > >> has been going on. I am getting closer and closer to wanting that > >> reverted. KaiGai-san seems to have been throwing out lots of special > >> cases that were there for good reasons. > > > Isn't this just a475c6036? > > Ah ... well, at least it was intentional. But still wrongheaded, > as this example shows. What we should have done was what the commit > message suggests, ie place a replacement check somewhere "upstream" > where it would apply to all object types. First thought that comes to > mind is to add a hack to pg_namespace_aclcheck, or maybe at some call > site(s). The attached patch seems to work: alvherre=# create table pg_catalog.foo (a int); ERROR: permission denied for schema pg_catalog It passes regression tests for both core and contrib. I notice that contrib/adminpack now fails, though (why doesn't this module have a regression test?): alvherre=# create extension adminpack; ERROR: permission denied for schema pg_catalog It sounds hard to support that without some other special hack. Not sure what to do here. Have adminpack set allowSystemTableMods somehow? I grepped for other occurences of "pg_catalog" in contrib SQL scripts, and all other modules seem to work (didn't try sepgsql): $ rgrep -l pg_catalog */*sql | cut -d/ -f1 | while read module; do echo module: $module; psql -c "create extension $module"; done module: adminpack ERROR: permission denied for schema pg_catalog module: btree_gist CREATE EXTENSION module: btree_gist ERROR: extension "btree_gist" already exists module: citext CREATE EXTENSION module: citext ERROR: extension "citext" already exists module: intarray CREATE EXTENSION module: isn CREATE EXTENSION module: lo CREATE EXTENSION module: pg_trgm CREATE EXTENSION module: pg_trgm ERROR: extension "pg_trgm" already exists module: sepgsql ERROR: could not open extension control file "/home/alvherre/Code/pgsql/install/HEAD/share/extension/sepgsql.control": No such file or directory module: tcn CREATE EXTENSION module: test_parser CREATE EXTENSION module: tsearch2 CREATE EXTENSION module: tsearch2 ERROR: extension "tsearch2" already exists -- Alvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: erroneous restore into pg_catalog schema
От:
Robert Haas <robertmhaas@gmail.com>
Дата:
On Tue, Jan 29, 2013 at 6:00 PM, Tom Lane wrote: > Robert Haas writes: >> On Tue, Jan 29, 2013 at 2:30 PM, Alvaro Herrera >>> Robert, are you working on this? > >> I wasn't, but I can, if we agree on it. > > I think we need to do *something* (and accordingly have added this to > the 9.3 open items page so we don't forget about it). Whether Robert's > idea is the best one probably depends in part on how clean the patch > turns out to be. The attached patch attempts to implement this. I discovered that, in fact, we have a number of places in our initdb-time scripts that rely on the current behavior, but they weren't hard to fix; and in fact I think the extra verbosity is probably not a bad thing here. See what you think. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: erroneous restore into pg_catalog schema
От:
Robert Haas <robertmhaas@gmail.com>
Дата:
On Sat, May 4, 2013 at 3:59 PM, Tom Lane wrote: > Given the lack of any good alternative proposals, I think we should > press ahead with this approach. We still need doc updates and fixes > for the affected contrib module(s), though. Also, in view of point 2, > it seems like the extensions code should test for and reject an attempt > to set a relocatable extension's schema to pg_catalog. Otherwise you'd > be likely to get not-too-intelligible errors from the extension script. I looked into this a bit more. It seems that adminpack is OK: it already qualifies all of the objects it creates with the pg_catalog schema. With the previous patch applied, all of the built-in extensions seem to install OK (except for uuid-ossp which I'm not set up to build, but it doesn't look like a problem) make check-world also passes. (adminpack actually has no regression tests, not even a test that the extension installs, but it does.) I looked for a suitable place to update the documentation and I don't have any brilliant ideas. The point that we're going to forbid relocatable extensions from being created in pg_catalog seems too minor to be worth noting; we don't document every trivial error message that can occur for every command in the manual. The point that we won't create ANY objects in pg_catalog unless the CREATE statement schema-qualifies the object name seems like it would be worth pointing out, but I don't see an obvious place to do it. Suggestions? In the attached new version of the patch, I added an explicit check to prevent relocatable extensions from being created in pg_catalog. Along the way, I noticed something else: these changes mean that fetch_search_path's includeImplicit flag doesn't really quite do what it says any more. I'm not sure whether we should change the behavior or rename the flag to, say, creationTargetsOnly. For the extension machinery's purpose, the existing meaning of the flag matches what it wants, but I replaced a couple of elog() calls with ereport(), using an existing message text; I suspect you could hit one or both of these before, and you certainly can with these changes. The other places where fetch_search is used with an argument of false are in the SQL functions current_schema() and current_schemas(). This change will cause them not to return pg_catalog in some cases where they currently would. It is unclear to me whether that is a good thing or a bad thing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
