Re: pg_dumpall Sets Roll default_tablespace Before Creating Tablespaces

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

"David E. Wheeler" <david(at)kineticode(dot)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

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

On Wed, Oct 19, 2011 at 5:13 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "David E. Wheeler" <david(at)kineticode(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Oct 19, 2011 at 5:13 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Oct20, 2011, at 00:09 , Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Oct 19, 2011 at 5:13 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Florian Pflug <fgp(at)phlo(dot)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

On Oct20, 2011, at 01:19 , Tom Lane wrote:
> Florian Pflug <fgp(at)phlo(dot)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

On Wed, Oct 19, 2011 at 7:46 PM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
> On Oct20, 2011, at 01:19 , Tom Lane wrote:
>> Florian Pflug <fgp(at)phlo(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

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

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 <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

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

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 <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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