Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces
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
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
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
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
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
Re: [HACKERS] pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces
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
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
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
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
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
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
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
