erroneous restore into pg_catalog schema

"Erik Rijkers" <er@xs4all.nl> writes:

(and yes, I did restore a 65 GB table into the pg_catalog schema of a dev machine; how can I
remove it? I could initdb, but it's 200+ GB; I'd rather not have to rebuild it)

See allow_system_table_mods

http://www.postgresql.org/docs/9.2/static/runtime-config-developer.html

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

"Erik Rijkers" <er@xs4all.nl> writes:

If you dump a table with -t schema.table, and in the receiving database that schema does not
exist, pg_restore-9.3devel will restore into the pg_catalog schema:
...
Off course the workaround is obvious, but shouldn't this be prevented from happening in the first
place? 9.2 refuses to do such a restore, which seems much better.

I said to myself "huh? surely we did not change pg_dump's behavior
here". But actually it's true, and the culprit is commit 880bfc328,
"Silently ignore any nonexistent schemas that are listed in
search_path". What pg_dump is emitting is

SET search_path = s, pg_catalog;
CREATE TABLE t (...);

and in HEAD the SET throws no error and instead establishes pg_catalog
as the target schema for object creation. Oops.

So obviously, 880bfc328 was several bricks shy of a load. The arguments
for that change in behavior still seem good for schemas *after* the
first one; but the situation is entirely different for the first schema,
because that's what the command is attempting to establish as the
creation schema.

In hindsight it might've been better if we'd used a separate GUC for the
object creation schema, but it's much too late to change that.

When we're dealing with a client-supplied SET command, I think it'd be
okay to just throw an error if the first schema doesn't exist. However,
the main issue in the discussion leading up to 880bfc328 was how to
behave for search_path values coming from noninteractive sources such as
postgresql.conf. In such cases we really don't have much choice about
whether to adopt the value in some sense.

I think maybe what we should do is have namespace.c retain an explicit
notion that "the first schema listed in search_path didn't exist", and
then throw errors if any attempt is made to create objects without an
explicitly specified namespace.

If we did that then we'd have a choice about whether to throw error in
the interactive-SET case. I'm a bit inclined to let it pass with no
error for consistency with the non-interactive case; IIRC such
consistency was one of the arguments made in the previous discussion.
But certainly there's an argument to be made the other way, too.

Thoughts?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane <tgl@sss.pgh.pa.us> writes:

I think maybe what we should do is have namespace.c retain an explicit
notion that "the first schema listed in search_path didn't exist", and
then throw errors if any attempt is made to create objects without an
explicitly specified namespace.

I don't much like this.

SET search_path TO dontexist, a, b;
CREATE TABLE foo();

And the table foo is now in a (provided it exists). Your proposal would
break that case, right? The problem is that the search_path could come
from too many places: postgresql.conf, ALTER ROLE, ALTER DATABASE etc.

And I have seen roles setup with some search_path containing schema that
will only exist in some of the database they can connect to…

--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

I think maybe what we should do is have namespace.c retain an explicit
notion that "the first schema listed in search_path didn't exist", and
then throw errors if any attempt is made to create objects without an
explicitly specified namespace.

I don't much like this.

SET search_path TO dontexist, a, b;
CREATE TABLE foo();

And the table foo is now in a (provided it exists). Your proposal would
break that case, right?

"Break"? You can't possibly think that's a good idea.

The problem is that the search_path could come
from too many places: postgresql.conf, ALTER ROLE, ALTER DATABASE etc.
And I have seen roles setup with some search_path containing schema that
will only exist in some of the database they can connect to…

Right, that is the argument for ignoring missing schemas, and I think it
is entirely sensible for *search* activities. But allowing *creation*
to occur in an indeterminate schema is a horrid idea.

BTW, although Erik claimed this behaved more sanely in 9.2, a closer
look at the commit logs says that the bogus commit shipped in 9.2,
so AFAICS it's broken there too. But earlier releases would have
rejected the SET as expected. I think we should assume that existing
code is expecting the pre-9.2 behavior.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane <tgl@sss.pgh.pa.us> writes:

"Break"? You can't possibly think that's a good idea.

I don't think it is. It's been used as a hack mainly before we had
per-user and per-database settings, from what I've seen.

Right, that is the argument for ignoring missing schemas, and I think it
is entirely sensible for *search* activities. But allowing *creation*
to occur in an indeterminate schema is a horrid idea.

It's not so much indeterminate for the user, even if I understand why
you say that. Creating new schemas is not done lightly in such cases…

But well, the solution is simple enough in that case. Use the newer form

ALTER ROLE foo IN DATABASE db1 SET search_path TO some, value;

So I'm fine with that change in fact. Is it possible to extend the
release notes to include so many details about it, as I don't think
anyone will get much excited to report that as a HINT when the
conditions are met… (although it might be simple enough thanks to the
pg_setting view).

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, January 13, 2013 22:09, Tom Lane wrote:

BTW, although Erik claimed this behaved more sanely in 9.2, a closer
look at the commit logs says that the bogus commit shipped in 9.2,
so AFAICS it's broken there too. But earlier releases would have
rejected the SET as expected. I think we should assume that existing
code is expecting the pre-9.2 behavior.

$ psql
psql (9.2.2-REL9_2_STABLE-20130113_1054-4ae5ee6c9b4dd7cd7e4471a44d371b228a9621c3)

Running the same on 9.2.2 (with latest patches):

$ ../../pgsql.HEAD/bug/test.sh
DROP DATABASE backupbug;
CREATE DATABASE backupbug;
drop schema if exists s cascade;
DROP SCHEMA
create schema s;
CREATE SCHEMA
create table s.t as select i from generate_series(1,1000) as f(i);
SELECT 1000
\dt+ pg_catalog.t
No matching relations found.
drop schema if exists s cascade;
DROP SCHEMA
pg_restore: connecting to database for restore
pg_restore: creating TABLE t
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 169; 1259 26204 TABLE t aardvark
pg_restore: [archiver (db)] could not execute query: ERROR: permission denied to create
"pg_catalog.t"
DETAIL: System catalog modifications are currently disallowed.
Command was: CREATE TABLE t (
i integer
);

pg_restore: restoring data for table "t"
pg_restore: [archiver (db)] Error from TOC entry 2780; 0 26204 TABLE DATA t aardvark
pg_restore: [archiver (db)] could not execute query: ERROR: relation "t" does not exist
Command was: COPY t (i) FROM stdin;

pg_restore: setting owner and privileges for TABLE t
pg_restore: setting owner and privileges for TABLE DATA t
WARNING: errors ignored on restore: 2
\dn
List of schemas
Name | Owner
--------+----------
public | aardvark
(1 row)

\dt+ s.
No matching relations found.
\dt+ pg_catalog.t
No matching relations found.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2013-01-13 12:29:08 -0500, Tom Lane wrote:

"Erik Rijkers" <er@xs4all.nl> writes:

If you dump a table with -t schema.table, and in the receiving database that schema does not
exist, pg_restore-9.3devel will restore into the pg_catalog schema:
...
Off course the workaround is obvious, but shouldn't this be prevented from happening in the first
place? 9.2 refuses to do such a restore, which seems much better.

I said to myself "huh? surely we did not change pg_dump's behavior
here". But actually it's true, and the culprit is commit 880bfc328,
"Silently ignore any nonexistent schemas that are listed in
search_path". What pg_dump is emitting is

SET search_path = s, pg_catalog;
CREATE TABLE t (...);

and in HEAD the SET throws no error and instead establishes pg_catalog
as the target schema for object creation. Oops.

So obviously, 880bfc328 was several bricks shy of a load. The arguments
for that change in behavior still seem good for schemas *after* the
first one; but the situation is entirely different for the first schema,
because that's what the command is attempting to establish as the
creation schema.

There also is the seemingly independent question of why the heck its
suddently allowed to create (but not drop?) tables in pg_catalog.
9.2 does:
andres=# CREATE TABLE pg_catalog.c AS SELECT 1;
ERROR: permission denied to create "pg_catalog.c"
DETAIL: System catalog modifications are currently disallowed.
Time: 54.180 ms

HEAD:
postgres=# CREATE TABLE pg_catalog.test AS SELECT 1;
SELECT 1
Time: 124.112 ms
postgres=# DROP TABLE pg_catalog.test;
ERROR: permission denied: "test" is a system catalog
Time: 0.461 ms

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

"Erik Rijkers" <er@xs4all.nl> writes:

On Sun, January 13, 2013 22:09, Tom Lane wrote:

BTW, although Erik claimed this behaved more sanely in 9.2, a closer
look at the commit logs says that the bogus commit shipped in 9.2,
so AFAICS it's broken there too.

[ not so ]

Hm, you are right, there's another problem that's independent of
search_path. In 9.2,

regression=# create table pg_catalog.t(f1 int);
ERROR: permission denied to create "pg_catalog.t"
DETAIL: System catalog modifications are currently disallowed.

but HEAD is missing that error check:

regression=# create table pg_catalog.t(f1 int);
CREATE TABLE

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.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

I will have a look.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane escribió:

"Erik Rijkers" <er@xs4all.nl> writes:

On Sun, January 13, 2013 22:09, Tom Lane wrote:

BTW, although Erik claimed this behaved more sanely in 9.2, a closer
look at the commit logs says that the bogus commit shipped in 9.2,
so AFAICS it's broken there too.

[ not so ]

Hm, you are right, there's another problem that's independent of
search_path. In 9.2,

regression=# create table pg_catalog.t(f1 int);
ERROR: permission denied to create "pg_catalog.t"
DETAIL: System catalog modifications are currently disallowed.

but HEAD is missing that error check:

regression=# create table pg_catalog.t(f1 int);
CREATE TABLE

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?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera <alvherre@2ndquadrant.com> 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).

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> 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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

The attached patch seems to work:

alvherre=# create table pg_catalog.foo (a int);
ERROR: permission denied for schema pg_catalog

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

Um. I knew that that module's desire to shove stuff into pg_catalog
would bite us someday. But now that I think about it, I'm pretty sure
I recall discussions to the effect that there are other third-party
modules doing similar things.

Anyway, this seems to answer Robert's original question about why
relations were special-cased: there are too many special cases around
this behavior. I think we should seriously consider just reverting
a475c6036.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane <tgl@sss.pgh.pa.us> writes:

Um. I knew that that module's desire to shove stuff into pg_catalog
would bite us someday. But now that I think about it, I'm pretty sure
I recall discussions to the effect that there are other third-party
modules doing similar things.

Yes, and some more have been starting to do that now that they have
proper DROP EXTENSION support to clean themselves up. At least that's
what I think the reason for doing so is…

--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane escribió:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

alvherre=# create extension adminpack;
ERROR: permission denied for schema pg_catalog

Um. I knew that that module's desire to shove stuff into pg_catalog
would bite us someday. But now that I think about it, I'm pretty sure
I recall discussions to the effect that there are other third-party
modules doing similar things.

How about we provide a superuser-only function that an extension can
call which will set enableSystemTableMods? It would get back
automatically to the default value on transaction end. That way,
extensions that wish to install stuff in pg_catalog can explicitely
declare it, i, and the rest of the world enjoys consistent protection.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Jan 13, 2013 at 4:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Right, that is the argument for ignoring missing schemas, and I think it
is entirely sensible for *search* activities. But allowing *creation*
to occur in an indeterminate schema is a horrid idea.

But the default search path is $user, public; and of those two, only
the latter exists by default.

--
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

On Mon, Jan 14, 2013 at 2:07 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Tom Lane escribió:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

alvherre=# create extension adminpack;
ERROR: permission denied for schema pg_catalog

Um. I knew that that module's desire to shove stuff into pg_catalog
would bite us someday. But now that I think about it, I'm pretty sure
I recall discussions to the effect that there are other third-party
modules doing similar things.

How about we provide a superuser-only function that an extension can
call which will set enableSystemTableMods? It would get back
automatically to the default value on transaction end. That way,
extensions that wish to install stuff in pg_catalog can explicitely
declare it, i, and the rest of the world enjoys consistent protection.

Or just document the existing GUC and make it something less than
PGC_POSTMASTER, like maybe PGC_SUSER.

But, really, I think allow_system_table_mods paints with too broad a
brush. It allows both things that are relatively OK (like creating a
function in pg_catalog) and things that are rampantly insane (like
dropping a column from pg_proc). It might be a good idea to make
those things controlled by two different switches.

Or perhaps there is some other way to make sure that the user "really
meant it", like refusing to create in pg_catalog unless the schema
name is given explicitly. I kind of like that idea, actually.

--
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

Robert Haas <robertmhaas@gmail.com> writes:

Or perhaps there is some other way to make sure that the user "really
meant it", like refusing to create in pg_catalog unless the schema
name is given explicitly. I kind of like that idea, actually.

That does seem attractive at first glance. Did you have an
implementation in mind? The idea that comes to mind for me is to hack
namespace.c, either to prevent activeCreationNamespace from getting set
to "pg_catalog" in the first place, or to throw error in
LookupCreationNamespace and friends. I am not sure though if
LookupCreationNamespace et al ever get called in contexts where no
immediate object creation is intended (and thus maybe an error wouldn't
be appropriate).

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jan 15, 2013 at 3:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Or perhaps there is some other way to make sure that the user "really
meant it", like refusing to create in pg_catalog unless the schema
name is given explicitly. I kind of like that idea, actually.

That does seem attractive at first glance. Did you have an
implementation in mind? The idea that comes to mind for me is to hack
namespace.c, either to prevent activeCreationNamespace from getting set
to "pg_catalog" in the first place, or to throw error in
LookupCreationNamespace and friends. I am not sure though if
LookupCreationNamespace et al ever get called in contexts where no
immediate object creation is intended (and thus maybe an error wouldn't
be appropriate).

As far as I can see, the principle place we'd want to hack would be
recomputeNamespacePath(), so that activeCreationNamespace never ends
up pointing to pg_catalog even if that's explicitly listed in
search_path. The places where we actually work out what schema to use
are RangeVarGetCreationNamespace() and
QualifiedNameGetCreationNamespace(), but those don't seem like they'd
need any adjustment, unless perhaps we wish to whack around the "no
schema has been selected to create in" error message in some way.

--
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