Re: patch: move dumpUserConfig call in dumpRoles function of pg_dumpall.c

Hello,

The attached patch changes the location of the dumpUserConfig call in
the dumpRoles function of pg_dumpall.

This is related to this thread:
http://archives.postgresql.org/pgsql-hackers/2011-02/msg02359.php

Currently if you use 'ALTER ROLE rolename SET ROLE', pg_dumpall will
dump an 'ALTER ROLE' out right after the 'CREATE ROLE' statement.
Sometimes this will cause a conflict when a dependent role is not yet
created:

--
-- Roles
--

CREATE ROLE a;
ALTER ROLE a WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN
NOREPLICATION;
ALTER ROLE a SET role TO 'b';
CREATE ROLE b;
ALTER ROLE b WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN
NOREPLICATION;
CREATE ROLE postgres;
ALTER ROLE postgres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN
NOREPLICATION;

As you can see, role a is set to role b before role b is created.

This patch moves the call to dumpUserConfig to after the loop where
all the roles are created. This produces output like the this:

--
-- Roles
--

CREATE ROLE a;
ALTER ROLE a WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN
NOREPLICATION;
CREATE ROLE b;
ALTER ROLE b WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN
NOREPLICATION;
CREATE ROLE postgres;
ALTER ROLE postgres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN
NOREPLICATION;
ALTER ROLE a SET role TO 'b';

Now this dump will succeed upon restore.

This passed all regression tests.

Thanks.

Phil Sorber <phil(at)omniti(dot)com> writes:
> Currently if you use 'ALTER ROLE rolename SET ROLE', pg_dumpall will
> dump an 'ALTER ROLE' out right after the 'CREATE ROLE' statement.

I think pg_dumpall is the very least of your problems if you do
something like that. We probably ought to forbid it entirely.

regards, tom lane

On Wed, Jul 27, 2011 at 7:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Phil Sorber <phil(at)omniti(dot)com> writes:
>> Currently if you use 'ALTER ROLE rolename SET ROLE', pg_dumpall will
>> dump an 'ALTER ROLE' out right after the 'CREATE ROLE' statement.
>
> I think pg_dumpall is the very least of your problems if you do
> something like that.  We probably ought to forbid it entirely.

Well, we had a long discussion of that on the thread Phil linked to,
and I don't think there was any consensus that forbidding it was the
right thing to do. Phil appears to be trying to implement the
proposal you made here:

http://archives.postgresql.org/pgsql-hackers/2011-01/msg00452.php

...although I don't think that what he did quite matches what you asked for.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Jul 27, 2011 at 7:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think pg_dumpall is the very least of your problems if you do
>> something like that. We probably ought to forbid it entirely.

> Well, we had a long discussion of that on the thread Phil linked to,
> and I don't think there was any consensus that forbidding it was the
> right thing to do.

You're right, I was half-remembering that thread and thinking that
there are a lot of gotchas in doing an ALTER ROLE SET ROLE. Florian
claimed in the thread that he'd never hit one before, but that doesn't
convince me much.

> Phil appears to be trying to implement the
> proposal you made here:
> http://archives.postgresql.org/pgsql-hackers/2011-01/msg00452.php
> ...although I don't think that what he did quite matches what you asked for.

No, the proposed patch doesn't go nearly far enough to address Florian's
problem. What I was speculating about was moving all the role (and
database) alters to the *end*, so they'd not take effect until after
we'd completed all the restore actions.

regards, tom lane

I have included two patches in this email. The first
(dump_user_config_last_with_set_role.patch) is an extension of my
first patch. In addition to moving the ALTER ROLE statements after the
CREATE ROLE statements it also inserts a SET ROLE after every connect.
It takes the role parameter from the --role command line option. This
fixes the problem of not being able to restore to a database because
of lack of permissions. This is similar to the idea proposed here:
http://archives.postgresql.org/pgsql-hackers/2010-12/msg01046.php

I think this patch is the better one for several reasons. It keeps the
ALTER ROLE and ALTER DATABASE statements in the same sections as their
related CREATE ROLE and CREATE DATABASE statements, thus not
dramatically altering the output of the command. It is a much simpler
non-invasive patch. It's concise and easy to understand. It solves all
the known scenario's that we have discussed previously.

That being said, I understand you have reservations about how it
works. You would prefer to move all of these type's of ALTER
statements to the end of the output. I also would prefer to see the
correct behavior included in the main tree over having my preference
over the changes. To that end I have also included a second patch
(dump_all_configs_last.patch) which handles it the way you prefer.
This creates new sections at the bottom for the ALTER statements.

These two patches are mutually exclusive. I would ask that you at
least consider the first patch before dismissing the idea. However, I
think applying either would would be an acceptable approach.

Thanks for your consideration.

On Thu, Jul 28, 2011 at 10:06 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Jul 27, 2011 at 7:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I think pg_dumpall is the very least of your problems if you do
>>> something like that.  We probably ought to forbid it entirely.
>
>> Well, we had a long discussion of that on the thread Phil linked to,
>> and I don't think there was any consensus that forbidding it was the
>> right thing to do.
>
> You're right, I was half-remembering that thread and thinking that
> there are a lot of gotchas in doing an ALTER ROLE SET ROLE.  Florian
> claimed in the thread that he'd never hit one before, but that doesn't
> convince me much.
>
>> Phil appears to be trying to implement the
>> proposal you made here:
>> http://archives.postgresql.org/pgsql-hackers/2011-01/msg00452.php
>> ...although I don't think that what he did quite matches what you asked for.
>
> No, the proposed patch doesn't go nearly far enough to address Florian's
> problem.  What I was speculating about was moving all the role (and
> database) alters to the *end*, so they'd not take effect until after
> we'd completed all the restore actions.
>
>                        regards, tom lane
>

Phil Sorber <phil(at)omniti(dot)com> writes:
> I have included two patches in this email. The first
> (dump_user_config_last_with_set_role.patch) is an extension of my
> first patch. In addition to moving the ALTER ROLE statements after the
> CREATE ROLE statements it also inserts a SET ROLE after every connect.
> It takes the role parameter from the --role command line option. This
> fixes the problem of not being able to restore to a database because
> of lack of permissions. This is similar to the idea proposed here:
> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01046.php

I don't understand why you think that that will fix anything?

The problem that Florian originally pointed out is that settings
established by ALTER DATABASE/ROLE could interfere with the restoration
script's actions. That seems to be just as much of a risk for the
--role role as the one originally used to connect. I don't see a way
around that other than not applying those settings until we are done
reconnecting to the target database.

Also, given that the --role switch is only defined to select the role
to be used at *dump* time, I'm unconvinced that forcing it to be used
at *restore* time is a good idea. You'd really need to invent a
separate switch if you were to go down this path.

regards, tom lane

On Tue, Aug 2, 2011 at 5:05 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Phil Sorber <phil(at)omniti(dot)com> writes:
>> I have included two patches in this email. The first
>> (dump_user_config_last_with_set_role.patch) is an extension of my
>> first patch. In addition to moving the ALTER ROLE statements after the
>> CREATE ROLE statements it also inserts a SET ROLE after every connect.
>> It takes the role parameter from the --role command line option. This
>> fixes the problem of not being able to restore to a database because
>> of lack of permissions. This is similar to the idea proposed here:
>> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01046.php
>
> I don't understand why you think that that will fix anything?
>
> The problem that Florian originally pointed out is that settings
> established by ALTER DATABASE/ROLE could interfere with the restoration
> script's actions.  That seems to be just as much of a risk for the
> --role role as the one originally used to connect.  I don't see a way
> around that other than not applying those settings until we are done
> reconnecting to the target database.
>
> Also, given that the --role switch is only defined to select the role
> to be used at *dump* time, I'm unconvinced that forcing it to be used
> at *restore* time is a good idea.  You'd really need to invent a
> separate switch if you were to go down this path.
>
>                        regards, tom lane
>

Ok, here is the patch that just moves the ALTER/SET pieces to the end.
Can we get this included in the next commit fest?

On Thu, Aug 4, 2011 at 1:53 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
> Ok, here is the patch that just moves the ALTER/SET pieces to the end.
> Can we get this included in the next commit fest?

Yep, just make yourself an account and add it.

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

On Thu, Aug 4, 2011 at 2:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Aug 4, 2011 at 1:53 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>> Ok, here is the patch that just moves the ALTER/SET pieces to the end.
>> Can we get this included in the next commit fest?
>
> Yep, just make yourself an account and add it.

Unfortunately, it doesn't look like anyone ever replied to this
thread, but Tom posted some thoughts on another thread that seem to me
to be a serious problem for this patch:

http://archives.postgresql.org/message-id/13764.1315094292@sss.pgh.pa.us

I don't see any easy way around that problem, so I'm going to mark
this patch Returned with Feedback for now. It strikes me as craziness
to try to guess which settings we should restore at the beginning and
which at the end, so I think we need a better idea. I don't really
understand why it's not OK to just have pg_dump issue RESET ROLE at
appropriate points in the process; that seems like it would be
sufficient and not particularly ugly.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I don't really
> understand why it's not OK to just have pg_dump issue RESET ROLE at
> appropriate points in the process; that seems like it would be
> sufficient and not particularly ugly.

Well, it was alleged that that would fix this problem:
http://archives.postgresql.org/pgsql-hackers/2010-12/msg00916.php
but if it does fix it, I think that's a bug in itself:
http://archives.postgresql.org/pgsql-hackers/2010-12/msg01031.php

But more to the point, I think the specific case of "ALTER DATABASE SET
ROLE" is just one element of a class of problems, namely that settings
attached to either databases or roles could create issues for restoring
a dump. Issuing RESET ROLE would fix only that one single case.

regards, tom lane

On Mon, Oct 10, 2011 at 12:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I don't really
>> understand why it's not OK to just have pg_dump issue RESET ROLE at
>> appropriate points in the process; that seems like it would be
>> sufficient and not particularly ugly.
>
> Well, it was alleged that that would fix this problem:
> http://archives.postgresql.org/pgsql-hackers/2010-12/msg00916.php
> but if it does fix it, I think that's a bug in itself:
> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01031.php

Hmm.

> But more to the point, I think the specific case of "ALTER DATABASE SET
> ROLE" is just one element of a class of problems, namely that settings
> attached to either databases or roles could create issues for restoring
> a dump.  Issuing RESET ROLE would fix only that one single case.

It's not very clear to me that we're going to find a fix that reaches
across every setting, though. I mean, for something like
maintenance_work_mem, there's no correctness issue regardless of when
the new setting takes effect, but there might very well be a
performance issue, and it's not really all that clear when the "right"
time to put the old setting back is. And that ambiguity about what's
actually correct is, perhaps, the root of the problem.

There are related cases where we have a clear-cut policy. For
example, we are clear that you must use the newer pg_dump against the
older server for best results. That's not always convenient, but at
least it's a line in the sand.

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

On Mon, Oct 10, 2011 at 11:54 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Aug 4, 2011 at 2:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Aug 4, 2011 at 1:53 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>>> Ok, here is the patch that just moves the ALTER/SET pieces to the end.
>>> Can we get this included in the next commit fest?
>>
>> Yep, just make yourself an account and add it.
>
> Unfortunately, it doesn't look like anyone ever replied to this
> thread, but Tom posted some thoughts on another thread that seem to me
> to be a serious problem for this patch:
>
> http://archives.postgresql.org/message-id/13764.1315094292@sss.pgh.pa.us
>
> I don't see any easy way around that problem, so I'm going to mark
> this patch Returned with Feedback for now.  It strikes me as craziness
> to try to guess which settings we should restore at the beginning and
> which at the end, so I think we need a better idea.  I don't really
> understand why it's not OK to just have pg_dump issue RESET ROLE at
> appropriate points in the process; that seems like it would be
> sufficient and not particularly ugly.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

I am going to remove that patch from the commit fest because we all
agree that it does not solve the problem satisfactorily. I would like
to re-iterate a few points, and submit a new patch.

First off, there are two distinct problems here that we have been
lumping into one. There is the issue of the 'ALTER DATABASE SET ROLE'
and then there is the 'ALTER ROLE SET ROLE' case. The former is the
one that has been causing us so many problems, and the latter is the
one that I really care about.

Also, there are more people that are hitting this issue as well:

http://archives.postgresql.org/pgsql-hackers/2011-02/msg02362.php

My proposal would be to table the discussion about the 'ALTER DATABASE
SET ROLE' case because there seems to be a bit of a philosophical
debate behind this that needs to be sorted out first.

If we focus only on the 'ALTER ROLE' case I think there is a trivial
solution. We already separate the CREATE from the ALTER in a single
loop. We also already separate out the user config in a separate
function called from this same loop. The problem is that the user
config can be dependent upon a later CREATE. So all I suggest is to
move the user config dumping into a new loop afterward so that the
user config ALTER's come after all the other CREATE's and ALTER's. It
amounts to a 7 line change and solves our problem rather elegantly.

On Wed, Oct 12, 2011 at 2:27 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
> I am going to remove that patch from the commit fest because we all
> agree that it does not solve the problem satisfactorily. I would like
> to re-iterate a few points, and submit a new patch.
>
> First off, there are two distinct problems here that we have been
> lumping into one. There is the issue of the 'ALTER DATABASE SET ROLE'
> and then there is the 'ALTER ROLE SET ROLE' case. The former is the
> one that has been causing us so many problems, and the latter is the
> one that I really care about.
>
> Also, there are more people that are hitting this issue as well:
>
> http://archives.postgresql.org/pgsql-hackers/2011-02/msg02362.php
>
> My proposal would be to table the discussion about the 'ALTER DATABASE
> SET ROLE' case because there seems to be a bit of a philosophical
> debate behind this that needs to be sorted out first.
>
> If we focus only on the 'ALTER ROLE' case I think there is a trivial
> solution. We already separate the CREATE from the ALTER in a single
> loop. We also already separate out the user config in a separate
> function called from this same loop. The problem is that the user
> config can be dependent upon a later CREATE. So all I suggest is to
> move the user config dumping into a new loop afterward so that the
> user config ALTER's come after all the other CREATE's and ALTER's. It
> amounts to a 7 line change and solves our problem rather elegantly.

I'm not sure I completely follow this explanation of the problem, but
it does seem better to me to set all the role attributes after dumping
all the create role statements, and I don't see how that can break
anything that works now.

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

On Wed, Oct 12, 2011 at 3:48 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Oct 12, 2011 at 2:27 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>> I am going to remove that patch from the commit fest because we all
>> agree that it does not solve the problem satisfactorily. I would like
>> to re-iterate a few points, and submit a new patch.
>>
>> First off, there are two distinct problems here that we have been
>> lumping into one. There is the issue of the 'ALTER DATABASE SET ROLE'
>> and then there is the 'ALTER ROLE SET ROLE' case. The former is the
>> one that has been causing us so many problems, and the latter is the
>> one that I really care about.
>>
>> Also, there are more people that are hitting this issue as well:
>>
>> http://archives.postgresql.org/pgsql-hackers/2011-02/msg02362.php
>>
>> My proposal would be to table the discussion about the 'ALTER DATABASE
>> SET ROLE' case because there seems to be a bit of a philosophical
>> debate behind this that needs to be sorted out first.
>>
>> If we focus only on the 'ALTER ROLE' case I think there is a trivial
>> solution. We already separate the CREATE from the ALTER in a single
>> loop. We also already separate out the user config in a separate
>> function called from this same loop. The problem is that the user
>> config can be dependent upon a later CREATE. So all I suggest is to
>> move the user config dumping into a new loop afterward so that the
>> user config ALTER's come after all the other CREATE's and ALTER's. It
>> amounts to a 7 line change and solves our problem rather elegantly.
>
> I'm not sure I completely follow this explanation of the problem, but
> it does seem better to me to set all the role attributes after dumping
> all the create role statements, and I don't see how that can break
> anything that works now.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Just to be clear, this doesn't move all the ALTER's. It will still do
CREATE/ALTER pair's for attributes that could go in the CREATE.

Example:

CREATE ROLE bob;
ALTER ROLE bob WITH LOGIN PASSWORD 'blah';

You could turn those in to one statement, but for various reasons you
do not want to. None of these have any dependencies other than the
actual creation of the role.

Then there is a separate section of code that is called as a separate
function 'dumpUserConfig()' that does other role attributes like
'ALTER ROLE bob SET role TO charlie'. These are the ALTER's that can
have dependencies on other roles.

I agree this is a little confusing, and if you prefer to see all the
ALTER's in one section together directly after the CREATE statements I
can make the patch do that, but it isn't necessary to solve the
problem.

Phil Sorber <phil(at)omniti(dot)com> writes:
> Then there is a separate section of code that is called as a separate
> function 'dumpUserConfig()' that does other role attributes like
> 'ALTER ROLE bob SET role TO charlie'. These are the ALTER's that can
> have dependencies on other roles.

Right. Phrased that way, this is an obvious improvement, and I concur
that it doesn't look like it could break anything that works now.
The more general problem remains to be solved, but we might as well
apply this.

regards, tom lane

On Wed, Oct 12, 2011 at 02:27:51PM -0400, Phil Sorber wrote:
> My proposal would be to table the discussion about the 'ALTER DATABASE
> SET ROLE' case because there seems to be a bit of a philosophical
> debate behind this that needs to be sorted out first.

Note: "to table the discussion" means opposite things in American and
British english. I think the rest of the sentence makes it clear what
you mean though :).

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> He who writes carelessly confesses thereby at the very outset that he does
> not attach much importance to his own thoughts.
-- Arthur Schopenhauer

On Wed, Oct 12, 2011 at 7:07 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Phil Sorber <phil(at)omniti(dot)com> writes:
>> Then there is a separate section of code that is called as a separate
>> function 'dumpUserConfig()' that does other role attributes like
>> 'ALTER ROLE bob SET role TO charlie'. These are the ALTER's that can
>> have dependencies on other roles.
>
> Right.  Phrased that way, this is an obvious improvement, and I concur
> that it doesn't look like it could break anything that works now.
> The more general problem remains to be solved, but we might as well
> apply this.

OK, done.

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