Thread

Commits

Extend and improve use of EXTRA_REGRESS_OPTS.
- 4ae5ee6c9b4d 9.3.0 cited
Remove misplaced sanity check from heap_create().
- a475c6036752 9.3.0 cited
Silently ignore any nonexistent schemas that are listed in search_path.
- 880bfc3287dd 9.2.0 cited

erroneous restore into pg_catalog schema

Erik Rijkers <er@xs4all.nl> — 2013-01-13T10:17:31Z

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:

HEAD

$ cat test.sh
#!/bin/sh

db=backupbug;

dropdb   --echo $db;
createdb --echo $db;

echo "drop schema if exists s cascade;" | psql -ad $db
echo "create schema s;" | psql -ad $db
echo "create table s.t as select i from generate_series(1,10) as f(i);" | psql -ad $db

echo '\dt+ pg_catalog.t' | psql -ad $db

pg_dump -F c -t s.t -f st.dump $db

echo "drop schema if exists s cascade;" | psql -ad $db

pg_restore -xOv -F c -d $db st.dump

echo '\dn'     | psql -ad $db
echo '\dt+ s.' | psql -ad $db
echo '\dt+ pg_catalog.t' | psql -ad $db


output:

$ ./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: processing data for table "t"
pg_restore: setting owner and privileges for TABLE t
pg_restore: setting owner and privileges for TABLE DATA t
\dn
  List of schemas
  Name  |  Owner
--------+----------
 public | aardvark
(1 row)

\dt+ s.
No matching relations found.
\dt+ pg_catalog.t
                     List of relations
   Schema   | Name | Type  |  Owner   | Size  | Description
------------+------+-------+----------+-------+-------------
 pg_catalog | t    | table | aardvark | 64 kB |
(1 row)

#----------------------

And then adds insult to injury:

backupbug=# drop table pg_catalog.t;
ERROR:  permission denied: "t" is a system catalog

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.

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

Thanks,

Erik Rijkers

Re: erroneous restore into pg_catalog schema

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2013-01-13T15:53:30Z

"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

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-13T17:29:08Z

"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

Re: erroneous restore into pg_catalog schema

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2013-01-13T20:50:06Z

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

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-13T21:09:29Z

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

Re: erroneous restore into pg_catalog schema

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2013-01-13T21:18:15Z

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

Re: erroneous restore into pg_catalog schema

Erik Rijkers <er@xs4all.nl> — 2013-01-13T21:23:30Z

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.

Re: erroneous restore into pg_catalog schema

Andres Freund <andres@anarazel.de> — 2013-01-13T21:31:45Z

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

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-13T21:37:53Z

"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

Re: erroneous restore into pg_catalog schema

Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-14T15:35:57Z

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

Re: erroneous restore into pg_catalog schema

Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-14T16:02:40Z

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

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-14T16:08:43Z

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

Re: erroneous restore into pg_catalog schema

Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-14T16:55:46Z

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

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-14T17:28:58Z

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

Re: erroneous restore into pg_catalog schema

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2013-01-14T17:44:21Z

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

Re: erroneous restore into pg_catalog schema

Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-14T19:07:29Z

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

Re: erroneous restore into pg_catalog schema

Robert Haas <robertmhaas@gmail.com> — 2013-01-15T20:04:45Z

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

Re: erroneous restore into pg_catalog schema

Robert Haas <robertmhaas@gmail.com> — 2013-01-15T20:09:38Z

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

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-15T20:22:11Z

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

Re: erroneous restore into pg_catalog schema

Robert Haas <robertmhaas@gmail.com> — 2013-01-15T20:40:02Z

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

Re: erroneous restore into pg_catalog schema

Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-29T19:30:16Z

Robert Haas escribió:
> 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, are you working on this?

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

Re: erroneous restore into pg_catalog schema

Robert Haas <robertmhaas@gmail.com> — 2013-01-29T22:37:49Z

On Tue, Jan 29, 2013 at 2:30 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Robert Haas escribió:
>> 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, are you working on this?

I wasn't, but I can, if we agree on it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-29T23:00:51Z

Robert Haas <robertmhaas@gmail.com> 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.

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Robert Haas <robertmhaas@gmail.com> — 2013-04-17T14:45:35Z

On Tue, Jan 29, 2013 at 6:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> 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

Tom Lane <tgl@sss.pgh.pa.us> — 2013-04-17T18:06:12Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jan 29, 2013 at 6:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.

I think this breaks contrib/adminpack, and perhaps other extensions.
They'd not be hard to fix with script changes, but they'd be broken.

In general, we would now have a situation where relocatable extensions
could never be installed into pg_catalog.  That might be OK, but at
least it would need to be documented.

Also, I think we'd be pretty much hard-wiring the decision that pg_dump
will never dump objects in pg_catalog, because its method for selecting
the creation schema won't work in that case.  That probably is all right
too, but we need to realize it's a consequence of this.

As far as the code goes, OK except I strongly disapprove of removing
the comment about temp_missing at line 3512.  The coding is not any less
a hack in that respect for having been pushed into a subroutine.  If
you want to rewrite the comment, fine, but failing to point out that
something funny is going on is not a service to readers.

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Robert Haas <robertmhaas@gmail.com> — 2013-04-22T15:15:38Z

On Wed, Apr 17, 2013 at 2:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think this breaks contrib/adminpack, and perhaps other extensions.
> They'd not be hard to fix with script changes, but they'd be broken.
>
> In general, we would now have a situation where relocatable extensions
> could never be installed into pg_catalog.  That might be OK, but at
> least it would need to be documented.
>
> Also, I think we'd be pretty much hard-wiring the decision that pg_dump
> will never dump objects in pg_catalog, because its method for selecting
> the creation schema won't work in that case.  That probably is all right
> too, but we need to realize it's a consequence of this.

These are all good points.  I'm uncertain whether they are sufficient
justification for abandoning this idea and looking for another
solution, or whether we should live with them.  Any thoughts?

> As far as the code goes, OK except I strongly disapprove of removing
> the comment about temp_missing at line 3512.  The coding is not any less
> a hack in that respect for having been pushed into a subroutine.  If
> you want to rewrite the comment, fine, but failing to point out that
> something funny is going on is not a service to readers.

OK, how about something like this: "Choose default creation namespace
(but note that temp_missing, if set, will trump this value)."

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-05-04T19:59:49Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Apr 17, 2013 at 2:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think this breaks contrib/adminpack, and perhaps other extensions.
>> They'd not be hard to fix with script changes, but they'd be broken.
>> 
>> In general, we would now have a situation where relocatable extensions
>> could never be installed into pg_catalog.  That might be OK, but at
>> least it would need to be documented.
>> 
>> Also, I think we'd be pretty much hard-wiring the decision that pg_dump
>> will never dump objects in pg_catalog, because its method for selecting
>> the creation schema won't work in that case.  That probably is all right
>> too, but we need to realize it's a consequence of this.

> These are all good points.  I'm uncertain whether they are sufficient
> justification for abandoning this idea and looking for another
> solution, or whether we should live with them.  Any thoughts?

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.

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2013-05-04T20:57:44Z

Tom Lane <tgl@sss.pgh.pa.us> writes:
>>> In general, we would now have a situation where relocatable extensions
>>> could never be installed into pg_catalog.  That might be OK, but at
>>> least it would need to be documented.

I've been idly trying to think of examples where that would be a
problem, without success.

Other than adminpack, I know of PGQ installing their objects in
pg_catalog. They only began doing that when switching to the CREATE
EXTENSION facility. And they set relocatable to false.

> Given the lack of any good alternative proposals, I think we should
> press ahead with this approach.  We still need doc updates and fixes

I would have to re-read the whole thread and pay close attention, but I
remember having that gut feeling you have when you don't like the design
and are not able to put words on why.

Now, any idea to clean that up is bound to be a nightmare to design
properly. I still remember being burnt hard when trying to work on
extensions and seach_path…

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

Reading the code now, it seems to me that we lack a more general test
and error situation to match with the comments.

	else if (control->schema != NULL)
	{
		/*
		 * The extension is not relocatable and the author gave us a schema
		 * for it.	We create the schema here if it does not already exist.
		 */

We should probably error out when entering in that block of code if the
extension is relocatable at all, right? That would fix the pg_catalog
case as well as the general one.

I should be able to prepare a patch early next week to fix that if
you're not done by then…

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

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-05-06T14:37:33Z

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> 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.

> Reading the code now, it seems to me that we lack a more general test
> and error situation to match with the comments.

> 	else if (control->schema != NULL)
> 	{
> 		/*
> 		 * The extension is not relocatable and the author gave us a schema
> 		 * for it.	We create the schema here if it does not already exist.
> 		 */

> We should probably error out when entering in that block of code if the
> extension is relocatable at all, right? That would fix the pg_catalog
> case as well as the general one.

Huh?  According to the comment, at least, we don't get here for a
relocatable extension.  I don't see anything wrong with auto-creating
the target schema for a non-relocatable extension.

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2013-05-06T14:58:17Z

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Huh?  According to the comment, at least, we don't get here for a
> relocatable extension.  I don't see anything wrong with auto-creating
> the target schema for a non-relocatable extension.

I was not finding why I would trust the comment the other evening, hence
my proposal. I now see that parse_extension_control_file has this check:

        if (control->relocatable && control->schema != NULL)
             ereport(ERROR,
                    (errcode(ERRCODE_SYNTAX_ERROR),
                     errmsg("parameter \"schema\" cannot be specified when \"relocatable\" is true")));

So it's ok. I now wonder how do you install a relocatable extension with
schema = pg_catalog, which I assumed was possible when reading the code
the other day.

I feel like I'm missing something big for not reading the whole thread
in details. Will send the patch I just finished for some documentation
work, then have a more serious look. Sorry about sharing that much
confusion…

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

Re: erroneous restore into pg_catalog schema

Robert Haas <robertmhaas@gmail.com> — 2013-05-09T15:24:27Z

On Sat, May 4, 2013 at 3:59 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

Re: erroneous restore into pg_catalog schema

Heikki Linnakangas <hlinnakangas@vmware.com> — 2013-05-13T08:49:07Z

On 09.05.2013 18:24, Robert Haas wrote:
> On Sat, May 4, 2013 at 3:59 PM, Tom Lane<tgl@sss.pgh.pa.us>  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.

Do we really want to forbid that? What's wrong with doing e.g:

  create extension btree_gist schema pg_catalog;

I would consider btree_gist, along with many other extensions, as 
core-enough parts of the system that you'd want to install them in 
pg_catalog, to make them readily available for everyone. Besides, not 
allowing that would break backwards-compatibility; I bet there are a lot 
of people doing exactly that.

- Heikki

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-05-13T15:48:44Z

Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 09.05.2013 18:24, Robert Haas wrote:
>> In the attached new version of the patch, I added an explicit check to
>> prevent relocatable extensions from being created in pg_catalog.

> Do we really want to forbid that?

The only alternative I see is the one proposed in
http://www.postgresql.org/message-id/12365.1358098148@sss.pgh.pa.us:

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

which is also pretty grotty.  Robert pointed out that it's inconsistent
with the traditional behavior of the default setting "$user, public".
Now, we could continue to treat $user as a special case, but that's just
stacking more kluges onto the pile.

BTW, looking back over the thread, I notice we have also not done
anything about this newly-introduced inconsistency:

regression=# create table pg_catalog.t(f1 int);
CREATE TABLE
regression=# drop table pg_catalog.t;
ERROR:  permission denied: "t" is a system catalog

Surely allow_system_table_mods should allow both or neither of those.

Perhaps, if we fixed that, the need to prevent table creations in
pg_catalog via search_path semantics changes would be lessened.

I believe the DROP prohibition is mainly there to prevent
	drop table pg_catalog.pg_proc;
	ERROR:  permission denied: "pg_proc" is a system catalog
but that thinking predates the invention of pg_depend.  If this
check were removed, you'd still be prevented from dropping pg_proc
because it's pinned.

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Robert Haas <robertmhaas@gmail.com> — 2013-05-13T16:20:54Z

On Mon, May 13, 2013 at 11:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Heikki Linnakangas <hlinnakangas@vmware.com> writes:
>> On 09.05.2013 18:24, Robert Haas wrote:
>>> In the attached new version of the patch, I added an explicit check to
>>> prevent relocatable extensions from being created in pg_catalog.
>
>> Do we really want to forbid that?
>
> The only alternative I see is the one proposed in
> http://www.postgresql.org/message-id/12365.1358098148@sss.pgh.pa.us:

Let me propose another alternative: it would be relatively
straightforward to allow this to work differently in extension scripts
than it does in general; it's already contingent in whether we're in
bootstrap-processing mode, and there's no reason we couldn't add some
other flag that gets set during extension-script processing mode, if
there isn't one already, and make it contingent on that also.  I think
what we're concerned with is mostly preventing accidental object
creation in pg_catalog, and allowing extensions to be created there
wouldn't interfere with that.

> I believe the DROP prohibition is mainly there to prevent
>         drop table pg_catalog.pg_proc;
>         ERROR:  permission denied: "pg_proc" is a system catalog
> but that thinking predates the invention of pg_depend.  If this
> check were removed, you'd still be prevented from dropping pg_proc
> because it's pinned.

Well, +1 for relaxing that restriction, no matter what else we do.
But that only makes an accidental restore into pg_catalog less sucky,
not less likely.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-05-13T16:32:08Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, May 13, 2013 at 11:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Heikki Linnakangas <hlinnakangas@vmware.com> writes:
>>> Do we really want to forbid that?

>> The only alternative I see is the one proposed in
>> http://www.postgresql.org/message-id/12365.1358098148@sss.pgh.pa.us:

> Let me propose another alternative: it would be relatively
> straightforward to allow this to work differently in extension scripts
> than it does in general;

That's just making the rules even more impossibly complicated (rules
that aren't documented, and we've not found any obvious place to
document them, so people aren't going to read whatever we do come up
with...)

The original objective of commit 880bfc328 was to simplify the rules
about search_path interpretation.  I'm not really happy about adding
a bunch of different, but just as obscure, rules in the name of making
things easier to use.  We'd be better off just reverting that patch IMO.

>> I believe the DROP prohibition is mainly there to prevent
>> drop table pg_catalog.pg_proc;
>> ERROR:  permission denied: "pg_proc" is a system catalog
>> but that thinking predates the invention of pg_depend.  If this
>> check were removed, you'd still be prevented from dropping pg_proc
>> because it's pinned.

> Well, +1 for relaxing that restriction, no matter what else we do.
> But that only makes an accidental restore into pg_catalog less sucky,
> not less likely.

Another way to fix that inconsistency is to consider that
allow_system_table_mods should gate table creations not just drops in
pg_catalog.  I'm not real sure why this wasn't the case all along ...

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-05-13T16:35:13Z

I wrote:
> Another way to fix that inconsistency is to consider that
> allow_system_table_mods should gate table creations not just drops in
> pg_catalog.  I'm not real sure why this wasn't the case all along ...

Uh, scratch that last comment: actually, allow_system_table_mods *did*
gate that, in every existing release.  I bitched upthread about the fact
that this was changed in 9.3, and did not hear any very satisfactory
defense of the change.

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Robert Haas <robertmhaas@gmail.com> — 2013-05-13T16:59:47Z

On Mon, May 13, 2013 at 12:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Another way to fix that inconsistency is to consider that
>> allow_system_table_mods should gate table creations not just drops in
>> pg_catalog.  I'm not real sure why this wasn't the case all along ...
>
> Uh, scratch that last comment: actually, allow_system_table_mods *did*
> gate that, in every existing release.  I bitched upthread about the fact
> that this was changed in 9.3, and did not hear any very satisfactory
> defense of the change.

It disallowed it only for tables, and not for any other object type.
I found that completely arbitrary.  It's perfectly obvious that people
want to be able to create objects in pg_catalog; shall we adopt a rule
that you can put extension there, as long as those extensions don't
happen to contain tables?  That is certainly confusing and arbitrary.

I suppose we could add a GUC, separate from allow_system_table_mods,
to allow specifically adding and dropping objects in pg_catalog.  It
would be consistent, and there would sure be a place to document it.
And it would make it easy to emit the right error-hint.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: erroneous restore into pg_catalog schema

Andres Freund <andres@2ndquadrant.com> — 2013-05-13T17:03:33Z

On 2013-05-13 12:59:47 -0400, Robert Haas wrote:
> On Mon, May 13, 2013 at 12:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I wrote:
> >> Another way to fix that inconsistency is to consider that
> >> allow_system_table_mods should gate table creations not just drops in
> >> pg_catalog.  I'm not real sure why this wasn't the case all along ...
> >
> > Uh, scratch that last comment: actually, allow_system_table_mods *did*
> > gate that, in every existing release.  I bitched upthread about the fact
> > that this was changed in 9.3, and did not hear any very satisfactory
> > defense of the change.
> 
> It disallowed it only for tables, and not for any other object type.
> I found that completely arbitrary.  It's perfectly obvious that people
> want to be able to create objects in pg_catalog; shall we adopt a rule
> that you can put extension there, as long as those extensions don't
> happen to contain tables?  That is certainly confusing and arbitrary.

Why don't we just prohibit deletion/modification for anything below
FirstNormalObjectId instead of using the schema as a restriction? Then
we can allow creation for tables as well.

Greetings,

Andres Freund

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: erroneous restore into pg_catalog schema

Robert Haas <robertmhaas@gmail.com> — 2013-05-13T17:04:52Z

On Mon, May 13, 2013 at 1:03 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> It disallowed it only for tables, and not for any other object type.
>> I found that completely arbitrary.  It's perfectly obvious that people
>> want to be able to create objects in pg_catalog; shall we adopt a rule
>> that you can put extension there, as long as those extensions don't
>> happen to contain tables?  That is certainly confusing and arbitrary.
>
> Why don't we just prohibit deletion/modification for anything below
> FirstNormalObjectId instead of using the schema as a restriction? Then
> we can allow creation for tables as well.

We currently do, but that led to problems with $SUBJECT.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: erroneous restore into pg_catalog schema

Andres Freund <andres@2ndquadrant.com> — 2013-05-13T17:18:28Z

On 2013-05-13 13:04:52 -0400, Robert Haas wrote:
> On Mon, May 13, 2013 at 1:03 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> It disallowed it only for tables, and not for any other object type.
> >> I found that completely arbitrary.  It's perfectly obvious that people
> >> want to be able to create objects in pg_catalog; shall we adopt a rule
> >> that you can put extension there, as long as those extensions don't
> >> happen to contain tables?  That is certainly confusing and arbitrary.
> >
> > Why don't we just prohibit deletion/modification for anything below
> > FirstNormalObjectId instead of using the schema as a restriction? Then
> > we can allow creation for tables as well.
> 
> We currently do, but that led to problems with $SUBJECT.

But we currently don't allow to drop. Which is confusingly
inconsistent. And allowing object creation withing pg_catalog only from
within extension scripts and not from normal SQL sounds like a *very*
poor workaround giving problems to quite some people upgrading from
earlier releases. Especially from those where we didn't have extensions.

And I don't see why allowing consistent relation creation/removal from
pg_catalog is conflicting with fixing the issue at hand?

Greetings,

Andres Freund

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-05-13T17:27:34Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, May 13, 2013 at 12:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Uh, scratch that last comment: actually, allow_system_table_mods *did*
>> gate that, in every existing release.  I bitched upthread about the fact
>> that this was changed in 9.3, and did not hear any very satisfactory
>> defense of the change.

> It disallowed it only for tables, and not for any other object type.
> I found that completely arbitrary.

No doubt, but nonetheless the name of the GUC is allow_system_TABLE_mods,
not allow_system_object_mods.  And removing just one part of its
longstanding functionality, in a way that creates the type of pg_dump
hazard this thread started with, is as arbitrary as things get.

We don't have time anymore to redesign this for 9.3, so I think just
putting back that one error check might be a reasonable fix for now.

> I suppose we could add a GUC, separate from allow_system_table_mods,
> to allow specifically adding and dropping objects in pg_catalog.

+1 ... for 9.4.  Or maybe the right thing is to replace all these tests
with checks on whether the objects are pinned (which, again, already
happens for the DROP case).  It's not immediately clear to me why we
need any of these restrictions for non-pinned objects.

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Heikki Linnakangas <hlinnakangas@vmware.com> — 2013-05-13T17:32:24Z

On 13.05.2013 19:59, Robert Haas wrote:
> On Mon, May 13, 2013 at 12:35 PM, Tom Lane<tgl@sss.pgh.pa.us>  wrote:
>> I wrote:
>>> Another way to fix that inconsistency is to consider that
>>> allow_system_table_mods should gate table creations not just drops in
>>> pg_catalog.  I'm not real sure why this wasn't the case all along ...
>>
>> Uh, scratch that last comment: actually, allow_system_table_mods *did*
>> gate that, in every existing release.  I bitched upthread about the fact
>> that this was changed in 9.3, and did not hear any very satisfactory
>> defense of the change.
>
> It disallowed it only for tables, and not for any other object type.
> I found that completely arbitrary.  It's perfectly obvious that people
> want to be able to create objects in pg_catalog; shall we adopt a rule
> that you can put extension there, as long as those extensions don't
> happen to contain tables?  That is certainly confusing and arbitrary.

Makes sense to me, actually. It's quite sensible to put functions, 
operators, etc. in pg_catalog. Especially if they're part of an 
extension. But I can't think of a good reason for putting a table in 
pg_catalog. Maybe some sort of control data for an extension, but seems 
like a kludge. Its contents wouldn't be included in pg_dump, for example.

- Heikki

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-05-13T17:40:57Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, May 13, 2013 at 1:03 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> Why don't we just prohibit deletion/modification for anything below
>> FirstNormalObjectId instead of using the schema as a restriction? Then
>> we can allow creation for tables as well.

> We currently do, but that led to problems with $SUBJECT.

AFAIR there are no code restrictions based on OID value.  We've got
restrictions based on things being in pg_catalog or not, and we've got
restrictions based on things being marked pinned in pg_depend.

Looking at the OID range might be a reasonable proxy for pinned-ness,
though, and it would certainly be a lot cheaper than a lookup in
pg_depend.

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Andres Freund <andres@2ndquadrant.com> — 2013-05-13T17:50:32Z

On 2013-05-13 13:40:57 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Mon, May 13, 2013 at 1:03 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> Why don't we just prohibit deletion/modification for anything below
> >> FirstNormalObjectId instead of using the schema as a restriction? Then
> >> we can allow creation for tables as well.
> 
> > We currently do, but that led to problems with $SUBJECT.

> AFAIR there are no code restrictions based on OID value.  We've got
> restrictions based on things being in pg_catalog or not, and we've got
> restrictions based on things being marked pinned in pg_depend.

> Looking at the OID range might be a reasonable proxy for pinned-ness,
> though, and it would certainly be a lot cheaper than a lookup in
> pg_depend.

It might need a slight change in GetNewObjectId() though:
		if (IsPostmasterEnvironment)
		{
			/* wraparound in normal environment */
			ShmemVariableCache->nextOid = FirstNormalObjectId;
			ShmemVariableCache->oidCount = 0;
		}
		else
		{
			/* we may be bootstrapping, so don't enforce the full range */
			if (ShmemVariableCache->nextOid < ((Oid) FirstBootstrapObjectId))
			{
				/* wraparound in standalone environment? */
				ShmemVariableCache->nextOid = FirstBootstrapObjectId;
				ShmemVariableCache->oidCount = 0;
			}
		}

I think we shouldn't check IsPostmasterEnvironment here but instead
IsBootstrapProcessingMode() since we otherwise can generate oids below
FirstNormalObjectId in --single mode. Imo that should be fixed
independently though, given the comment it looks like either an
oversight or the check predating the existance of
IsBootstrapProcessingMode().


Greetings,

Andres Freund

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-05-13T18:35:47Z

Andres Freund <andres@2ndquadrant.com> writes:
> I think we shouldn't check IsPostmasterEnvironment here but instead
> IsBootstrapProcessingMode() since we otherwise can generate oids below
> FirstNormalObjectId in --single mode.

That is, in fact, exactly what we want to do and must do during initdb.
If you change anything about this code you'll break the way the
post-bootstrap initdb steps assign OIDs.

(The comments about "wraparound" are slightly misleading, since those
code blocks also execute during the first OID assignment in normal
mode and the first one in post-bootstrap initdb processing, respectively.)

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Andres Freund <andres@2ndquadrant.com> — 2013-05-13T18:43:55Z

On 2013-05-13 14:35:47 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > I think we shouldn't check IsPostmasterEnvironment here but instead
> > IsBootstrapProcessingMode() since we otherwise can generate oids below
> > FirstNormalObjectId in --single mode.
> 
> That is, in fact, exactly what we want to do and must do during initdb.
> If you change anything about this code you'll break the way the
> post-bootstrap initdb steps assign OIDs.

Well, then we should use some other way to discern from those both
cases. If you currently execute CREATE TABLE or something else in
--single user mode the database cannot safely be pg_upgraded anymore
since the oids might already be used in a freshly initdb'ed cluster in
the new version.
Or am I missing something here?

DROPing and recreating a new index in --single mode isn't that
uncommon...

Greetings,

Andres Freund

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-05-13T18:48:52Z

Andres Freund <andres@2ndquadrant.com> writes:
> On 2013-05-13 14:35:47 -0400, Tom Lane wrote:
>> That is, in fact, exactly what we want to do and must do during initdb.
>> If you change anything about this code you'll break the way the
>> post-bootstrap initdb steps assign OIDs.

> Well, then we should use some other way to discern from those both
> cases. If you currently execute CREATE TABLE or something else in
> --single user mode the database cannot safely be pg_upgraded anymore
> since the oids might already be used in a freshly initdb'ed cluster in
> the new version.

[ shrug... ]  In the list of ways you can break your system in --single
mode, that one has got to be exceedingly far down the list.

> DROPing and recreating a new index in --single mode isn't that
> uncommon...

Surely you'd just REINDEX it instead.  Moreover, if it isn't a system
index already, why are you doing this in --single mode at all?

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Andres Freund <andres@2ndquadrant.com> — 2013-05-13T18:55:01Z

On 2013-05-13 14:48:52 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2013-05-13 14:35:47 -0400, Tom Lane wrote:
> >> That is, in fact, exactly what we want to do and must do during initdb.
> >> If you change anything about this code you'll break the way the
> >> post-bootstrap initdb steps assign OIDs.
> 
> > Well, then we should use some other way to discern from those both
> > cases. If you currently execute CREATE TABLE or something else in
> > --single user mode the database cannot safely be pg_upgraded anymore
> > since the oids might already be used in a freshly initdb'ed cluster in
> > the new version.
> 
> [ shrug... ]  In the list of ways you can break your system in --single
> mode, that one has got to be exceedingly far down the list.

Well, sure there are loads of ways where you can intentionally break
things. But I'd say that it's not exactly obvious that CREATE INDEX
can break things.

> > DROPing and recreating a new index in --single mode isn't that
> > uncommon...
> 
> Surely you'd just REINDEX it instead.  Moreover, if it isn't a system
> index already, why are you doing this in --single mode at all?

The last case I had was that an index was corrupted in a way that
autovacuum got stuck on the corrupt index and wasn't killable. Without
single mode it was hard to be fast enough to drop the index before
autovac grabbed the lock again.

Greetings,

Andres Freund

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: erroneous restore into pg_catalog schema

Marko Kreen <markokr@gmail.com> — 2013-05-13T19:00:04Z

On Sat, May 04, 2013 at 10:57:44PM +0200, Dimitri Fontaine wrote:
> Other than adminpack, I know of PGQ installing their objects in
> pg_catalog. They only began doing that when switching to the CREATE
> EXTENSION facility. And they set relocatable to false.

FYI - PgQ and related modules install no objects into pg_catalog.

I used schema='pg_catalog' because I had trouble getting schema='pgq'
to work.  I wanted 'pgq' schema to live and die with extension,
and that was only way I got it to work on 9.1.

-- 
marko

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-05-13T19:16:17Z

Andres Freund <andres@2ndquadrant.com> writes:
> On 2013-05-13 14:48:52 -0400, Tom Lane wrote:
>> Andres Freund <andres@2ndquadrant.com> writes:
>>> DROPing and recreating a new index in --single mode isn't that
>>> uncommon...

>> Surely you'd just REINDEX it instead.  Moreover, if it isn't a system
>> index already, why are you doing this in --single mode at all?

> The last case I had was that an index was corrupted in a way that
> autovacuum got stuck on the corrupt index and wasn't killable. Without
> single mode it was hard to be fast enough to drop the index before
> autovac grabbed the lock again.

Meh.  Actually, after looking closer at xlog.c, the OID counter starts
out at FirstBootstrapObjectId, which is not what I'd been thinking.
So a value less than that must indicate wraparound, which presumably
never happens during initdb.  We could just change the code to

            if (ShmemVariableCache->nextOid < ((Oid) FirstBootstrapObjectId))
            {
                /* wraparound while in standalone environment */
                ShmemVariableCache->nextOid = FirstNormalObjectId;
                ShmemVariableCache->oidCount = 0;
            }

which is a bit asymmetric-looking but should do the right thing in all
cases.

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Stephen Frost <sfrost@snowman.net> — 2013-05-14T01:04:06Z

* Marko Kreen (markokr@gmail.com) wrote:
> On Sat, May 04, 2013 at 10:57:44PM +0200, Dimitri Fontaine wrote:
> > Other than adminpack, I know of PGQ installing their objects in
> > pg_catalog. They only began doing that when switching to the CREATE
> > EXTENSION facility. And they set relocatable to false.
> 
> FYI - PgQ and related modules install no objects into pg_catalog.
> 
> I used schema='pg_catalog' because I had trouble getting schema='pgq'
> to work.  I wanted 'pgq' schema to live and die with extension,
> and that was only way I got it to work on 9.1.

I've read through this thread and I think you're the only person here
that I actually agree with..  I like the idea of having a schema that
lives & dies with an extension.  imv, putting random objects (of ANY
kind) into pg_catalog is a bad idea.  Sure, it's convenient because it's
always in your search_path, but that, imv, means we should have a way to
say "these schemas are always in the search_path", not that we should
encourage people to dump crap into pg_catalog.

	Thanks,

		Stephen

Re: erroneous restore into pg_catalog schema

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2013-05-14T07:29:38Z

Stephen Frost <sfrost@snowman.net> writes:
> * Marko Kreen (markokr@gmail.com) wrote:
>> On Sat, May 04, 2013 at 10:57:44PM +0200, Dimitri Fontaine wrote:
>> > Other than adminpack, I know of PGQ installing their objects in
>> > pg_catalog. They only began doing that when switching to the CREATE
>> > EXTENSION facility. And they set relocatable to false.
>> 
>> FYI - PgQ and related modules install no objects into pg_catalog.
>> 
>> I used schema='pg_catalog' because I had trouble getting schema='pgq'
>> to work.  I wanted 'pgq' schema to live and die with extension,
>> and that was only way I got it to work on 9.1.

Sorry, I didn't take the time to actually read \dx+ pgq, just remembered
(and checked) that the control file did mention pg_catalog.

There is a documented way to have the schema live and die with the
extension), which is:

  relocatable = false
  schema = 'pgq'

Then CREATE EXTENSION will also create the schema, that will be a member
of the extension, and thus will get DROPed with it.

> I've read through this thread and I think you're the only person here
> that I actually agree with..  I like the idea of having a schema that
> lives & dies with an extension.  imv, putting random objects (of ANY
> kind) into pg_catalog is a bad idea.  Sure, it's convenient because it's
> always in your search_path, but that, imv, means we should have a way to
> say "these schemas are always in the search_path", not that we should
> encourage people to dump crap into pg_catalog.

I'm not sure I agree with that view about pg_catalog. Sometimes we talk
about moving some parts of core in pre-installed extensions instead, and
if we do that we will want those extensions to install themselves into
pg_catalog.

Maybe we need some vocabulary to be able to distinguish "system
extensions" from "user extensions", while keeping in mind that the goal
is for those two beasts to remain the same thing at the SQL level (try
reading the "Inline Extension" or "Extension Templates" threads if
you're not convinced, I got there the hard way).

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

Re: erroneous restore into pg_catalog schema

Marko Kreen <markokr@gmail.com> — 2013-05-14T08:49:48Z

On Tue, May 14, 2013 at 09:29:38AM +0200, Dimitri Fontaine wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Marko Kreen (markokr@gmail.com) wrote:
> >> On Sat, May 04, 2013 at 10:57:44PM +0200, Dimitri Fontaine wrote:
> >> > Other than adminpack, I know of PGQ installing their objects in
> >> > pg_catalog. They only began doing that when switching to the CREATE
> >> > EXTENSION facility. And they set relocatable to false.
> >> 
> >> FYI - PgQ and related modules install no objects into pg_catalog.
> >> 
> >> I used schema='pg_catalog' because I had trouble getting schema='pgq'
> >> to work.  I wanted 'pgq' schema to live and die with extension,
> >> and that was only way I got it to work on 9.1.
> 
> Sorry, I didn't take the time to actually read \dx+ pgq, just remembered
> (and checked) that the control file did mention pg_catalog.
> 
> There is a documented way to have the schema live and die with the
> extension), which is:
> 
>   relocatable = false
>   schema = 'pgq'
> 
> Then CREATE EXTENSION will also create the schema, that will be a member
> of the extension, and thus will get DROPed with it.

That's the problem - it's not dropped with it.

Btw, if I do "ALTER EXTENSION pgq ADD SCHEMA pgq;" during
"CREATE EXTENSION pgq FROM 'unpackaged';" then Postgres
will complain:

  ERROR:  cannot add schema "pgq" to extension "pgq" because the schema
  contains the extension

Seems the code is pretty sure it's invalid concept...

-- 
marko

Re: erroneous restore into pg_catalog schema

Stephen Frost <sfrost@snowman.net> — 2013-05-14T10:59:34Z

* Dimitri Fontaine (dimitri@2ndQuadrant.fr) wrote:
> I'm not sure I agree with that view about pg_catalog. Sometimes we talk
> about moving some parts of core in pre-installed extensions instead, and
> if we do that we will want those extensions to install themselves into
> pg_catalog.

For my part, I'd still prefer to have those go into a different schema
than into pg_catalog.  Perhaps that's overkill but I really do like the
seperation of system tables from extensions which can be added and
removed..

	Thanks,

		Stephen

Re: erroneous restore into pg_catalog schema

Andres Freund <andres@2ndquadrant.com> — 2013-05-14T11:16:55Z

On 2013-05-13 21:04:06 -0400, Stephen Frost wrote:
> * Marko Kreen (markokr@gmail.com) wrote:
> > On Sat, May 04, 2013 at 10:57:44PM +0200, Dimitri Fontaine wrote:
> > > Other than adminpack, I know of PGQ installing their objects in
> > > pg_catalog. They only began doing that when switching to the CREATE
> > > EXTENSION facility. And they set relocatable to false.
> > 
> > FYI - PgQ and related modules install no objects into pg_catalog.
> > 
> > I used schema='pg_catalog' because I had trouble getting schema='pgq'
> > to work.  I wanted 'pgq' schema to live and die with extension,
> > and that was only way I got it to work on 9.1.
> 
> I've read through this thread and I think you're the only person here
> that I actually agree with..  I like the idea of having a schema that
> lives & dies with an extension.  imv, putting random objects (of ANY
> kind) into pg_catalog is a bad idea.  Sure, it's convenient because it's
> always in your search_path, but that, imv, means we should have a way to
> say "these schemas are always in the search_path", not that we should
> encourage people to dump crap into pg_catalog.

I don't disagree, but how is that relevant for fixing the issue at hand?
We still need to fix restores that currently target the wrong schema in
a backward compatible manner?

Greetings,

Andres Freund

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: erroneous restore into pg_catalog schema

Stephen Frost <sfrost@snowman.net> — 2013-05-14T12:35:03Z

* Andres Freund (andres@2ndquadrant.com) wrote:
> I don't disagree, but how is that relevant for fixing the issue at hand?
> We still need to fix restores that currently target the wrong schema in
> a backward compatible manner?

On this, I agree w/ Tom that we should put that check back into place-
it's really too late to do anything else.

	Thanks,

		Stephen

Re: erroneous restore into pg_catalog schema

Heikki Linnakangas <hlinnakangas@vmware.com> — 2013-06-03T07:52:35Z

On 14.05.2013 15:35, Stephen Frost wrote:
> * Andres Freund (andres@2ndquadrant.com) wrote:
>> I don't disagree, but how is that relevant for fixing the issue at hand?
>> We still need to fix restores that currently target the wrong schema in
>> a backward compatible manner?
>
> On this, I agree w/ Tom that we should put that check back into place-
> it's really too late to do anything else.

In the interest of getting the release out, I've reverted commit 
a475c603. We'll probably want to do something more elegant in the 
future, but this will do for now.

- Heikki

Re: erroneous restore into pg_catalog schema

Tom Lane <tgl@sss.pgh.pa.us> — 2013-06-03T14:18:57Z

Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> In the interest of getting the release out, I've reverted commit 
> a475c603. We'll probably want to do something more elegant in the 
> future, but this will do for now.

That may be the best short-term answer, but I see no such revert
in the repo ...

			regards, tom lane

Re: erroneous restore into pg_catalog schema

Heikki Linnakangas <hlinnakangas@vmware.com> — 2013-06-03T14:23:19Z

On 03.06.2013 17:18, Tom Lane wrote:
> Heikki Linnakangas<hlinnakangas@vmware.com>  writes:
>> In the interest of getting the release out, I've reverted commit
>> a475c603. We'll probably want to do something more elegant in the
>> future, but this will do for now.
>
> That may be the best short-term answer, but I see no such revert
> in the repo ...

Oh, forgot to push. It's there now.

- Heikki

Re: erroneous restore into pg_catalog schema

Greg Stark <stark@mit.edu> — 2013-06-05T16:12:47Z

On Tue, May 14, 2013 at 11:59 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Dimitri Fontaine (dimitri@2ndQuadrant.fr) wrote:
>> I'm not sure I agree with that view about pg_catalog. Sometimes we talk
>> about moving some parts of core in pre-installed extensions instead, and
>> if we do that we will want those extensions to install themselves into
>> pg_catalog.
>
> For my part, I'd still prefer to have those go into a different schema
> than into pg_catalog.  Perhaps that's overkill but I really do like the
> seperation of system tables from extensions which can be added and
> removed..

This was discussed previously. It's a bad idea. It's very tempting but
it doesn't scale. Then every user needs to know every schema for every
extension they might want to use.

It's exactly equivalent to the very common pattern of sysadmins
installing things into /usr/local/apache, /usr/local/kde,
/usr/local/gnome, /usr/local/pgsql, etc. Then every user needs a
mile-long PATH, LD_LIBRARY_PATH, JAVACLASSPATH, etc. And every user
has a slightly different ordering and slightly different subset of
directories in their paths resulting in different behaviours and
errors for each user. A correctly integrated package will use standard
locations and then users can simply refer to the standard locations
and find what's been installed. It would be ok to have a schema for
all extensions separately from the core, but it can't be a schema for
each extension or else we might as well not have the extension
mechanism at all. Users would still need to "install" the extension by
editing their config to refer to it.

-- 
greg

Re: erroneous restore into pg_catalog schema

Stephen Frost <sfrost@snowman.net> — 2013-06-10T13:03:05Z

Greg,

* Greg Stark (stark@mit.edu) wrote:
> On Tue, May 14, 2013 at 11:59 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Dimitri Fontaine (dimitri@2ndQuadrant.fr) wrote:
> >> I'm not sure I agree with that view about pg_catalog. Sometimes we talk
> >> about moving some parts of core in pre-installed extensions instead, and
> >> if we do that we will want those extensions to install themselves into
> >> pg_catalog.
> >
> > For my part, I'd still prefer to have those go into a different schema
> > than into pg_catalog.  Perhaps that's overkill but I really do like the
> > seperation of system tables from extensions which can be added and
> > removed..
> 
> This was discussed previously. It's a bad idea. It's very tempting but
> it doesn't scale. Then every user needs to know every schema for every
> extension they might want to use.

Having a schema that isn't pg_catalog doesn't necessairly mean we need a
schema per extension.  Just a 'pg_extensions' schema, which is added to
search_path behind the scenes (just like pg_catalog..) would be better
than having everything go into pg_catalog.  I'd prefer that we let the
admins control both where extensions get installed to and what schemas
are added to the end of the search_path.

> It's exactly equivalent to the very common pattern of sysadmins
> installing things into /usr/local/apache, /usr/local/kde,
> /usr/local/gnome, /usr/local/pgsql, etc. Then every user needs a
> mile-long PATH, LD_LIBRARY_PATH, JAVACLASSPATH, etc. And every user
> has a slightly different ordering and slightly different subset of
> directories in their paths resulting in different behaviours and
> errors for each user. 

This would be because admins can't maintain control over the PATH
variable in every shell, not even to simply add things to it, and so
users end up building up their own PATH by hand over time.  What's more,
even with a distro like Debian, you don't keep all of your system
configuration (eg: /etc) in the same place that all the user-called
binaries (/usr/bin) go, nor do you put the libraries (eg: functions in
extensions which are not intended to be user-facing) in the same place
as binaries.

> A correctly integrated package will use standard
> locations and then users can simply refer to the standard locations
> and find what's been installed. It would be ok to have a schema for
> all extensions separately from the core, but it can't be a schema for
> each extension or else we might as well not have the extension
> mechanism at all. Users would still need to "install" the extension by
> editing their config to refer to it.

... because we don't give the admins (or even the extensions
themselves..) any ability to handle this.

	Thanks,

		Stephen

Re: erroneous restore into pg_catalog schema

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2013-06-10T13:35:27Z

Greg Stark <stark@mit.edu> writes:
> On Tue, May 14, 2013 at 11:59 AM, Stephen Frost <sfrost@snowman.net> wrote:
>> For my part, I'd still prefer to have those go into a different schema
>> than into pg_catalog.  Perhaps that's overkill but I really do like the
>> seperation of system tables from extensions which can be added and
>> removed..
>
> This was discussed previously. It's a bad idea. It's very tempting but
> it doesn't scale. Then every user needs to know every schema for every
> extension they might want to use.

+1

Your description of how bad this idea is is the best I've read I think:

> It's exactly equivalent to the very common pattern of sysadmins
> installing things into /usr/local/apache, /usr/local/kde,
> /usr/local/gnome, /usr/local/pgsql, etc. Then every user needs a
> mile-long PATH, LD_LIBRARY_PATH, JAVACLASSPATH, etc. And every user
> has a slightly different ordering and slightly different subset of
> directories in their paths resulting in different behaviours and
> errors for each user. A correctly integrated package will use standard
> locations and then users can simply refer to the standard locations
> and find what's been installed. It would be ok to have a schema for
> all extensions separately from the core, but it can't be a schema for
> each extension or else we might as well not have the extension
> mechanism at all. Users would still need to "install" the extension by
> editing their config to refer to it.

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

Re: erroneous restore into pg_catalog schema

Greg Stark <stark@mit.edu> — 2013-06-10T14:02:22Z

On Mon, Jun 10, 2013 at 2:03 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Having a schema that isn't pg_catalog doesn't necessairly mean we need a
> schema per extension.  Just a 'pg_extensions' schema, which is added to
> search_path behind the scenes (just like pg_catalog..) would be better
> than having everything go into pg_catalog.

Well no objection here. That's just like having /usr/local/{lib,bin,etc}.

> I'd prefer that we let the
> admins control both where extensions get installed to and what schemas
> are added to the end of the search_path.

This I object to. That's like having /usr/local/{apache,pgsql,kde,gnome}/bin.




-- 
greg

Re: erroneous restore into pg_catalog schema

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2013-06-10T14:45:10Z

Stephen Frost <sfrost@snowman.net> writes:
> Having a schema that isn't pg_catalog doesn't necessairly mean we need a
> schema per extension.  Just a 'pg_extensions' schema, which is added to
> search_path behind the scenes (just like pg_catalog..) would be better
> than having everything go into pg_catalog.  I'd prefer that we let the
> admins control both where extensions get installed to and what schemas
> are added to the end of the search_path.

That was discussed in the scope of the first extension patch and it took
us about 1 year to conclude not to try to solve search_path at the same
time as extensions. I'm not convinced we've had extensions for long
enough to be able to reach a conclusion already, but I'll friendly watch
that conversation happen again.

My opinion is that a pg_extension schema with a proper and well
documented set of search_path properties would be good to have. A way to
rename it per-database doesn't strike me as that useful as long as we
have ALTER EXTENSION … SET SCHEMA …

The current default schema where to install extensions being "public",
almost anything we do on that front will be an improvement.

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

Re: erroneous restore into pg_catalog schema

Stephen Frost <sfrost@snowman.net> — 2013-06-10T14:58:41Z

* Dimitri Fontaine (dimitri@2ndQuadrant.fr) wrote:
> My opinion is that a pg_extension schema with a proper and well
> documented set of search_path properties would be good to have. A way to
> rename it per-database doesn't strike me as that useful as long as we
> have ALTER EXTENSION … SET SCHEMA …

While having one place to put everything sounds great, it doesn't do a
whole lot of good if you consider conflicts- either because you want
multiple versions available or because there just happens to be some
overlap in function names (or similar).  There are also extensions which
have more than just functions in them but also tables, which increases
the chances of a conflict happening.  Having the extension authors end
up having to prefix everything with the name of the extension to avoid
conflicts would certainly be worse than actually using schemas.

Again, in PG, there's a lot more control which the database admin has
and, imv, DBAs are going to be able to manage the extensions if they're
given the right tools.  Saying "dump everything in one place because
that's the only place we can be sure all users will look at" just
doesn't fit.  There also isn't one central authority which deals with
how extension components are named, unlike with package-based systems
where Debian or Red Hat or someone deals with those issues.  Lastly,
afaik, we don't have any 'divert' or 'alternatives' type of system for
dealing with legitimate conflicts when they happen (and they will..).

Basically, there's a lot of infrastructure that goes into making "put
everything in /usr/bin" work and we haven't got any of it while we also
don't have the problem that is individual user shells with unique
.profile/.bashrc/.tcshrc files that set PATH variables.

> The current default schema where to install extensions being "public",
> almost anything we do on that front will be an improvement.

Indeed..  I've never liked that.

	Thanks,

		Stephen

Re: erroneous restore into pg_catalog schema

Stephen Frost <sfrost@snowman.net> — 2013-06-10T15:00:42Z

* Greg Stark (stark@mit.edu) wrote:
> > I'd prefer that we let the
> > admins control both where extensions get installed to and what schemas
> > are added to the end of the search_path.
> 
> This I object to. That's like having /usr/local/{apache,pgsql,kde,gnome}/bin.

... or it's like giving the admins the ability to manage their systems
and deal with conflicts or issues that we don't currently have any way
to handle.

	Thanks,

		Stephen

Re: erroneous restore into pg_catalog schema

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2013-06-10T15:19:31Z

Stephen Frost <sfrost@snowman.net> writes:
> While having one place to put everything sounds great, it doesn't do a
> whole lot of good if you consider conflicts- either because you want
> multiple versions available or because there just happens to be some
> overlap in function names (or similar).  There are also extensions which
> have more than just functions in them but also tables, which increases
> the chances of a conflict happening.  Having the extension authors end
> up having to prefix everything with the name of the extension to avoid
> conflicts would certainly be worse than actually using schemas.

Now you're not talking about *default* settings anymore, or are you?

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

Re: erroneous restore into pg_catalog schema

Stephen Frost <sfrost@snowman.net> — 2013-06-11T05:14:21Z

* Dimitri Fontaine (dimitri@2ndQuadrant.fr) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > While having one place to put everything sounds great, it doesn't do a
> > whole lot of good if you consider conflicts- either because you want
> > multiple versions available or because there just happens to be some
> > overlap in function names (or similar).  There are also extensions which
> > have more than just functions in them but also tables, which increases
> > the chances of a conflict happening.  Having the extension authors end
> > up having to prefix everything with the name of the extension to avoid
> > conflicts would certainly be worse than actually using schemas.
> 
> Now you're not talking about *default* settings anymore, or are you?

What happens with the default settings when you try to install two
extensions that have overlapping function signatures..?  I can't imagine
it 'just works'..  And then what?  Is there a way that an admin can set
up search paths for individual users which provide the 'right' function
and work even when the user decides to change their search_path?

	Thanks,

		Stephen

Re: erroneous restore into pg_catalog schema

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2013-06-11T08:33:29Z

Stephen Frost <sfrost@snowman.net> writes:
> What happens with the default settings when you try to install two
> extensions that have overlapping function signatures..?  I can't imagine
> it 'just works'..  And then what?  Is there a way that an admin can set
> up search paths for individual users which provide the 'right' function
> and work even when the user decides to change their search_path?

That entirely depends on how the extension script is written. Making it
possible to have two versions concurrently installed require a non
trivial amount of efforts, but I don't think the extension facility gets
in the way at all, currently.

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

Re: erroneous restore into pg_catalog schema

Andres Freund <andres@2ndquadrant.com> — 2013-06-11T08:39:01Z

On 2013-06-11 10:33:29 +0200, Dimitri Fontaine wrote:
> That entirely depends on how the extension script is written. Making it
> possible to have two versions concurrently installed require a non
> trivial amount of efforts, but I don't think the extension facility gets
> in the way at all, currently.

It does. We only allow an extension to be installed once, irregardless
of schema...

Greetings,

Andres Freund

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: erroneous restore into pg_catalog schema

Stephen Frost <sfrost@snowman.net> — 2013-06-11T11:26:47Z

* Dimitri Fontaine (dimitri@2ndQuadrant.fr) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > What happens with the default settings when you try to install two
> > extensions that have overlapping function signatures..?  I can't imagine
> > it 'just works'..  And then what?  Is there a way that an admin can set
> > up search paths for individual users which provide the 'right' function
> > and work even when the user decides to change their search_path?
> 
> That entirely depends on how the extension script is written. Making it
> possible to have two versions concurrently installed require a non
> trivial amount of efforts, but I don't think the extension facility gets
> in the way at all, currently.

How would you recommend writing an extension script which deals with
conflicts?

Also, as Andres points out, the current extension system doesn't allow
installing multiple versions.  It'd be kind of nice if it did, but
there's problems in that direction.  Extension authors can manage that
issue by having differently named extensions (where the name includes
some number); similar to libraries.  That isn't the only case where name
conflicts can and will occur between extensions though, which is the
more general issue that I was pointing out.

If there's no knowledge between the extension authors of the other
extension (which is likey the case..) then chances are that such a
conflict will cause either a failure or incorrect behavior.

	Thanks,

		Stephen