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

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.