Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-11-09 Thread Bruce Momjian
Florian Pflug wrote:
 On Oct27, 2011, at 23:02 , Bruce Momjian wrote:
  Florian Pflug wrote:
  On Oct21, 2011, at 16:42 , Phil Sorber wrote:
  If you did want to make them immutable, I also like Florian's idea of
  a dependency graph. This would make the dumps less readable though.
  
  Hm, I kinda reversed my opinion on that, though - i.e., I no longer think
  that the dependency graph idea has much merit. For two reasons
  
  First, dependencies work on OIDs, not on names. Thus, for the dependency
  machinery to work for GUCs, they'd also need to store OIDs instead of
  names of referenced schema objects. (Otherwise you get into trouble if
  objects are renamed)
  
  Which of course doesn't work, at least for roles, because roles are
  shared objects, but referenced objects might be database-local.
  (search_path, for example).
  
  Is this a TODO?
 
 The idea quoted above, no. But
 
  Downgrade non-immutable (i.e., dependent on database state) checks during
  ALTER ROLE/DATABASE SET to WARNINGs to avoid breakage during restore
 
 makes for a fine TODO, I'd say.

Well, psql currently ignored restore errors too, so I am not sure what
the value of this is, except that pg_upgrade will not error exit, but I
am not sure that is a good idea here either.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

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


Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-10-27 Thread Bruce Momjian
Florian Pflug wrote:
 On Oct21, 2011, at 16:42 , Phil Sorber wrote:
  If you did want to make them immutable, I also like Florian's idea of
  a dependency graph. This would make the dumps less readable though.
 
 Hm, I kinda reversed my opinion on that, though - i.e., I no longer think
 that the dependency graph idea has much merit. For two reasons
 
 First, dependencies work on OIDs, not on names. Thus, for the dependency
 machinery to work for GUCs, they'd also need to store OIDs instead of
 names of referenced schema objects. (Otherwise you get into trouble if
 objects are renamed)
 
 Which of course doesn't work, at least for roles, because roles are
 shared objects, but referenced objects might be database-local.
 (search_path, for example).

Is this a TODO?

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

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


Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-10-27 Thread Florian Pflug
On Oct27, 2011, at 23:02 , Bruce Momjian wrote:
 Florian Pflug wrote:
 On Oct21, 2011, at 16:42 , Phil Sorber wrote:
 If you did want to make them immutable, I also like Florian's idea of
 a dependency graph. This would make the dumps less readable though.
 
 Hm, I kinda reversed my opinion on that, though - i.e., I no longer think
 that the dependency graph idea has much merit. For two reasons
 
 First, dependencies work on OIDs, not on names. Thus, for the dependency
 machinery to work for GUCs, they'd also need to store OIDs instead of
 names of referenced schema objects. (Otherwise you get into trouble if
 objects are renamed)
 
 Which of course doesn't work, at least for roles, because roles are
 shared objects, but referenced objects might be database-local.
 (search_path, for example).
 
 Is this a TODO?

The idea quoted above, no. But

 Downgrade non-immutable (i.e., dependent on database state) checks during
 ALTER ROLE/DATABASE SET to WARNINGs to avoid breakage during restore

makes for a fine TODO, I'd say.

best regards,
Florian Pflug


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


Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-10-21 Thread Phil Sorber
On Wed, Oct 19, 2011 at 7:46 PM, Florian Pflug f...@phlo.org wrote:
 On Oct20, 2011, at 01:19 , Tom Lane wrote:
 Florian Pflug f...@phlo.org writes:
 Taking this even further, why do we bother with non-immutable (i.e.,
 depending on the database's contents) checks during ALTER ROLE/DATABASET SET
 at all?

 Yeah, I was wondering about that one too.  It would not solve all the
 problems here, but skipping validity checks would fix some of them.
 The trouble of course is what happens if the value is found to be bad
 when you try to use it ...

 Presumably we'd detect that during logon, because the GUC assignment
 hook will quite probably complain. I'd vote for emitting a warning in
 that case. This is also what we due currently if we fail to set the
 GUC to the desired value due to permission issues

 postgres=# create role r1 login;
 CREATE ROLE
 postgres=# create role r2;
 CREATE ROLE
 postgres=# alter role r1 set role = r2;
 ALTER ROLE
 postgres=# \connect - r1
 WARNING:  permission denied to set role r2
 WARNING:  permission denied to set role r2
 You are now connected to database postgres as user r1.

 (Dunno why that WARNING appears twice)

 Since an ALTER DATABASE/ROLE SET doesn't prevent the user from overriding
 the value, ignoring invalid settings shouldn't be a security risk.

I didn't realize these dependencies weren't immutable. If that is the
desired behavior, then I agree a warning should be sufficient to catch
typo's and oversights.

If you did want to make them immutable, I also like Florian's idea of
a dependency graph. This would make the dumps less readable though.


 best regards,
 Florian Pflug


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


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


Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-10-21 Thread Florian Pflug
On Oct21, 2011, at 16:42 , Phil Sorber wrote:
 If you did want to make them immutable, I also like Florian's idea of
 a dependency graph. This would make the dumps less readable though.

Hm, I kinda reversed my opinion on that, though - i.e., I no longer think
that the dependency graph idea has much merit. For two reasons

First, dependencies work on OIDs, not on names. Thus, for the dependency
machinery to work for GUCs, they'd also need to store OIDs instead of
names of referenced schema objects. (Otherwise you get into trouble if
objects are renamed)

Which of course doesn't work, at least for roles, because roles are
shared objects, but referenced objects might be database-local.
(search_path, for example).

best regards,
Florian Pflug


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


[HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-10-19 Thread David E. Wheeler
Hackers,

We've just found an issue with pg_dumpall in 9.1.1 where a dump starts with 
lines like these:

ALTER ROLE dude WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB LOGIN 
PASSWORD 'md5bdd7f8e73a214981b1519212b02a5530' VALID UNTIL 'infinity';
ALTER ROLE dude SET default_tablespace TO 'users';

And later in the file has lines like this:

CREATE TABLESPACE users OWNER postgres LOCATION 
'/data/postgres/pg_tblspc/users';

Unsurprisingly, perhaps, this results in errors such as:

ERROR:  invalid value for parameter default_tablespace: users

Seems to me that default_tablespace should only be set after tablespaces are 
created, no?

This is wreaking havoc with our ability to run pg_upgrade, FWIW.

Best,

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


Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-10-19 Thread Tom Lane
David E. Wheeler da...@kineticode.com writes:
 We've just found an issue with pg_dumpall in 9.1.1 where a dump starts with 
 lines like these:

 ALTER ROLE dude WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB LOGIN 
 PASSWORD 'md5bdd7f8e73a214981b1519212b02a5530' VALID UNTIL 'infinity';
 ALTER ROLE dude SET default_tablespace TO 'users';

I'm beginning to think that the correct solution to these problems is to
greatly restrict what you can set in ALTER ROLE/DATABASE SET.  Or at
least to document that if you use it, you get to keep both pieces after
you break pg_dump.

regards, tom lane

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


Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-10-19 Thread David E. Wheeler
On Oct 19, 2011, at 2:13 PM, Tom Lane wrote:

ALTER ROLE dude WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB LOGIN 
 PASSWORD 'md5bdd7f8e73a214981b1519212b02a5530' VALID UNTIL 'infinity';
ALTER ROLE dude SET default_tablespace TO 'users';
 
 I'm beginning to think that the correct solution to these problems is to
 greatly restrict what you can set in ALTER ROLE/DATABASE SET.  Or at
 least to document that if you use it, you get to keep both pieces after
 you break pg_dump.

Sorry, get to keep what two pieces?

Best,

David


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


Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-10-19 Thread Robert Haas
On Wed, Oct 19, 2011 at 5:13 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 David E. Wheeler da...@kineticode.com writes:
 We've just found an issue with pg_dumpall in 9.1.1 where a dump starts with 
 lines like these:

     ALTER ROLE dude WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB LOGIN 
 PASSWORD 'md5bdd7f8e73a214981b1519212b02a5530' VALID UNTIL 'infinity';
     ALTER ROLE dude SET default_tablespace TO 'users';

 I'm beginning to think that the correct solution to these problems is to
 greatly restrict what you can set in ALTER ROLE/DATABASE SET.  Or at
 least to document that if you use it, you get to keep both pieces after
 you break pg_dump.

This is another instance of the general principle that we need to
create all the objects first, and then set their properties.  I
believe you came up with one counterexample where we needed to set the
GUC first in order to be able to create the object, but ISTM most of
them are going the other way.

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

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


Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-10-19 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Wed, Oct 19, 2011 at 5:13 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 I'm beginning to think that the correct solution to these problems is to
 greatly restrict what you can set in ALTER ROLE/DATABASE SET.  Or at
 least to document that if you use it, you get to keep both pieces after
 you break pg_dump.

 This is another instance of the general principle that we need to
 create all the objects first, and then set their properties.  I
 believe you came up with one counterexample where we needed to set the
 GUC first in order to be able to create the object, but ISTM most of
 them are going the other way.

Well, a general principle for which we already know one counterexample
isn't general enough for me.  The problem that I'm having here is that
it's not clear that there is any general solution, short of pg_dumpall
having variable-by-variable knowledge of which GUCs to set when, and
maybe even that wouldn't be good enough.

regards, tom lane

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


Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-10-19 Thread Florian Pflug
On Oct20, 2011, at 00:09 , Tom Lane wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Wed, Oct 19, 2011 at 5:13 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 I'm beginning to think that the correct solution to these problems is to
 greatly restrict what you can set in ALTER ROLE/DATABASE SET.  Or at
 least to document that if you use it, you get to keep both pieces after
 you break pg_dump.
 
 This is another instance of the general principle that we need to
 create all the objects first, and then set their properties.  I
 believe you came up with one counterexample where we needed to set the
 GUC first in order to be able to create the object, but ISTM most of
 them are going the other way.
 
 Well, a general principle for which we already know one counterexample
 isn't general enough for me.  The problem that I'm having here is that
 it's not clear that there is any general solution, short of pg_dumpall
 having variable-by-variable knowledge of which GUCs to set when, and
 maybe even that wouldn't be good enough.

This whole issue reminds me of the situation we had before pg_dump
had the smarts to traverse the object dependency graph and emit schema
objects in the correct order. (pg_dump gained that ability somewhere
around 7.3 or 7.4 if memory serves correctly)

So here's a wild idea. Could we somehow make use of the dependency
machinery to solve this once and for all? Maybe we could record the
dependency between per role and/or database GUC settings and the
referenced objects.

Or we could add a flag FORCE to ALTER ROLE/DATABASE SET for pg_dump's
benefit which would skip all validity checks on the value (except it being
of the correct type, maybe).

Taking this even further, why do we bother with non-immutable (i.e.,
depending on the database's contents) checks during ALTER ROLE/DATABASET SET
at all? If we don't record a dependency on referenced schema objects,
nothing prevents that object from being dropped *after* the ALTER ROLE/DATABASE
SET occurred...

If we're trying to protect against typos in settings such as default_tablespace,
a WARNING ought to be sufficient.

best regards,
Florian Pflug


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


Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-10-19 Thread Tom Lane
Florian Pflug f...@phlo.org writes:
 Taking this even further, why do we bother with non-immutable (i.e.,
 depending on the database's contents) checks during ALTER ROLE/DATABASET SET
 at all?

Yeah, I was wondering about that one too.  It would not solve all the
problems here, but skipping validity checks would fix some of them.
The trouble of course is what happens if the value is found to be bad
when you try to use it ...

regards, tom lane

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


Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

2011-10-19 Thread Florian Pflug
On Oct20, 2011, at 01:19 , Tom Lane wrote:
 Florian Pflug f...@phlo.org writes:
 Taking this even further, why do we bother with non-immutable (i.e.,
 depending on the database's contents) checks during ALTER ROLE/DATABASET SET
 at all?
 
 Yeah, I was wondering about that one too.  It would not solve all the
 problems here, but skipping validity checks would fix some of them.
 The trouble of course is what happens if the value is found to be bad
 when you try to use it ...

Presumably we'd detect that during logon, because the GUC assignment
hook will quite probably complain. I'd vote for emitting a warning in
that case. This is also what we due currently if we fail to set the
GUC to the desired value due to permission issues

postgres=# create role r1 login;
CREATE ROLE
postgres=# create role r2;
CREATE ROLE
postgres=# alter role r1 set role = r2;
ALTER ROLE
postgres=# \connect - r1
WARNING:  permission denied to set role r2
WARNING:  permission denied to set role r2
You are now connected to database postgres as user r1.

(Dunno why that WARNING appears twice)

Since an ALTER DATABASE/ROLE SET doesn't prevent the user from overriding
the value, ignoring invalid settings shouldn't be a security risk.

best regards,
Florian Pflug


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