Thread: Segmentation fault in pg_dumpall from master down to 9.1 and other bug introduced by RLS

Hi,

There's a segfault when trying to dump global object from a running
7.4.27 with a pg_dumpall of version 9.3.5 but also 9.2.9.

$ pg_dumpall  -g -h localhost -p 5474

    column number -1 is out of range 0..12
    Segmentation fault (core dumped)

The problem comes from the first columns of the query in function
dumpRoles(PGconn *conn) that has no alias name. Fixing it with

    SELECT 0 **as oid**, ...;

Fix the issue. This bug affect all versions of PostgreSQL from master
down to 9.1, I mean 9.1 is working.

In the same query there is an other bug introduced by commit 491c029
that add Row-Level Security Policies.  Current master code has a broken
pg_dumpall when trying to dump from a backend lower than 8.1. Here is
the error:

    ERROR:  each UNION query must have the same number of columns

The query sent to the database is the following:

    SELECT 0, usename as rolname, usesuper as rolsuper, true as
rolinherit, usesuper as rolcreaterole, usecreatedb as rolcreatedb, true
as rolcanlogin, -1 as rolconnlimit, passwd as rolpassword, valuntil as
rolvaliduntil, false as rolreplication, null as rolcomment, usename =
current_user AS is_current_user FROM pg_shadow UNION ALL SELECT 0,
groname as rolname, false as rolsuper, true as rolinherit, false as
rolcreaterole, false as rolcreatedb, false as rolcanlogin, -1 as
rolconnlimit, null::text as rolpassword, null::abstime as rolvaliduntil,
false as rolreplication, false as rolbypassrls, null as rolcomment,
false FROM pg_group WHERE NOT EXISTS (SELECT 1 FROM pg_shadow  WHERE
usename = groname) ORDER BY 2;

The column rolbypassrls is missing in the first UNION query. As this is
the same query as previous issue the first column of the query need the
same alias: oid.

I've attached a patch against master that fix the two issues but for
older branch, only alias to the first column of the query might be
applied. Let me know if it need other work.

Best regards,

--
Gilles Darold
http://dalibo.com - http://dalibo.org

Gilles Darold <gilles.darold@dalibo.com> writes:
> There's a segfault when trying to dump global object from a running
> 7.4.27 with a pg_dumpall of version 9.3.5 but also 9.2.9.

Hm ... I make a practice of checking pg_dump's backwards compatibility
from time to time, but I confess I've not tested pg_dumpall lately.
Will take care of this.  Thanks for the report and patch!
        regards, tom lane

Gilles Darold <gilles.darold@dalibo.com> writes:
> In the same query there is another bug introduced by commit 491c029
> that add Row-Level Security Policies.  Current master code has a broken
> pg_dumpall when trying to dump from a backend lower than 8.1.

Actually, I think that code is not just under-tested but poorly thought
out.  It will dump ALL roles from a pre-9.5 database with NOBYPASSRLS;
even superusers.  I would think that existing superusers ought to be
assumed to have the BYPASSRLS property, no?  (This assumes that the
property means anything at all for superusers, which maybe it doesn't;
but if so we probably ought to be forcing it true for superusers to
avoid confusion.)
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Gilles Darold <gilles.darold@dalibo.com> writes:
> > In the same query there is another bug introduced by commit 491c029
> > that add Row-Level Security Policies.  Current master code has a broken
> > pg_dumpall when trying to dump from a backend lower than 8.1.
>
> Actually, I think that code is not just under-tested but poorly thought
> out.  It will dump ALL roles from a pre-9.5 database with NOBYPASSRLS;
> even superusers.  I would think that existing superusers ought to be
> assumed to have the BYPASSRLS property, no?  (This assumes that the
> property means anything at all for superusers, which maybe it doesn't;
> but if so we probably ought to be forcing it true for superusers to
> avoid confusion.)

Superusers are always considered to have it, regardless of if the option
is set for them and so, no, it isn't relevant to superusers (that's true
for nearly all of the role attribute options, as I recall..).  It can be
reworked to set it for superusers when it's dumped, but I'm not sure
that really helps.  Consider that creating a new superuser role doesn't
go and set CREATEROLE or any of the other attributes, yet a superuser is
considered to have those rights regardless.

If we want to really force it to 'true' for superusers then we should
change all the role attributes to act in the same way and to be set
whenever superuser is set.  It might get annoying though for users who
grant superuser and then revoke it- what do we do then?  The only sane
answer seems to be "leave those other attributes in place", but that
could certainly be confusing for users.

Still, I don't feel strongly either way about it- but whatever we do
here, we should remember to do the same for the other new role
attributes under discussion to be added.

I'm happy to fix it either way (and fix it for 8.1, and back to.. what?
Postgres95?)
Thanks!
    Stephen

* Stephen Frost (sfrost@snowman.net) wrote:
> I'm happy to fix it either way (and fix it for 8.1, and back to.. what?
> Postgres95?)

Err.  That might have come off poorly- I didn't mean that sarcastically
but was really wondering how far back you test (or how far back we
feel pg_dumpall needs to work against..).  I was originally going to say
7.4, but then figured we might try to go back farther than even that
and, well, Postgres95 is the oldest we have "easy" access to..
Thanks!
    Stephen

On 2014-11-13 18:00:45 -0500, Stephen Frost wrote:
> * Stephen Frost (sfrost@snowman.net) wrote:
> > I'm happy to fix it either way (and fix it for 8.1, and back to.. what?
> > Postgres95?)
> 
> Err.  That might have come off poorly- I didn't mean that sarcastically
> but was really wondering how far back you test (or how far back we
> feel pg_dumpall needs to work against..).  I was originally going to say
> 7.4, but then figured we might try to go back farther than even that
> and, well, Postgres95 is the oldest we have "easy" access to..

pg_dump currently errors out before 7.0:/* * We allow the server to be back to 7.0, and up to any minor release of *
ourown major version.  (See also version check in pg_dumpall.c.) */fout->minRemoteVersion =
70000;fout->maxRemoteVersion= (PG_VERSION_NUM / 100) * 100 + 99;
 


I do think it might be justifyable to bump that a bit.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-11-13 18:00:45 -0500, Stephen Frost wrote:
>> Err.  That might have come off poorly- I didn't mean that sarcastically
>> but was really wondering how far back you test (or how far back we
>> feel pg_dumpall needs to work against..).

> pg_dump currently errors out before 7.0:
> I do think it might be justifyable to bump that a bit.

Yeah, we've discussed that before, and I wouldn't mind explicitly ripping
out support for pre-7.3 or pre-7.4 servers; we could get rid of a pretty
fair amount of cruft in pg_dump with such a policy change.  But right now
the standard is 7.0 and I don't want that goalpost moving silently.

(And yeah, I did just test it back to 7.0.  But that depends on my HPPA
box which is looking increasingly frail.  I'm not sure that we can get
versions that old to compile on more modern platforms, at least not
without a lot more work than it's probably worth.)
        regards, tom lane

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Actually, I think that code is not just under-tested but poorly thought
>> out.  It will dump ALL roles from a pre-9.5 database with NOBYPASSRLS;
>> even superusers.

> Superusers are always considered to have it, regardless of if the option
> is set for them and so, no, it isn't relevant to superusers (that's true
> for nearly all of the role attribute options, as I recall..).

OK, good.

> It can be
> reworked to set it for superusers when it's dumped, but I'm not sure
> that really helps.  Consider that creating a new superuser role doesn't
> go and set CREATEROLE or any of the other attributes, yet a superuser is
> considered to have those rights regardless.

What's bothering me is that I see this in pg_dumpall output from a 9.4
or earlier database:

ALTER ROLE postgres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN REPLICATION NOBYPASSRLS;

That means that if you do a pg_upgrade from a 9.4 database, your built-in
superuser will now not have rolbypassrls set, though it does in a database
built in any other way.  Even if that doesn't have any functional effect,
it's a recipe for confusion IMO.  So I think that the code ought to be
"usesuper as rolbypassrls" rather than "false as rolbypassrls" for
back branches.

The only other similar case is rolreplication, which perhaps also ought
to read as usesuper for old branches.
        regards, tom lane

On 2014-11-13 18:24:53 -0500, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2014-11-13 18:00:45 -0500, Stephen Frost wrote:
> >> Err.  That might have come off poorly- I didn't mean that sarcastically
> >> but was really wondering how far back you test (or how far back we
> >> feel pg_dumpall needs to work against..).
> 
> > pg_dump currently errors out before 7.0:
> > I do think it might be justifyable to bump that a bit.
> 
> Yeah, we've discussed that before, and I wouldn't mind explicitly ripping
> out support for pre-7.3 or pre-7.4 servers; we could get rid of a pretty
> fair amount of cruft in pg_dump with such a policy change.  But right now
> the standard is 7.0 and I don't want that goalpost moving silently.

Oh, absolutely agreed. It'd be a master only change anyway.

> (And yeah, I did just test it back to 7.0.  But that depends on my HPPA
> box which is looking increasingly frail.  I'm not sure that we can get
> versions that old to compile on more modern platforms, at least not
> without a lot more work than it's probably worth.)

Right. I just tested, and the first one that compiles out of the box on
my quite recent debian sid workstation is 7.4. That also passes make
check.

7.3 fails in compiling the bison output; 7.2 doesn't detect flex; 7.1,
7.0 fail in configure.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> What's bothering me is that I see this in pg_dumpall output from a 9.4
> or earlier database:
>
> ALTER ROLE postgres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN REPLICATION NOBYPASSRLS;

Ah, yeah, good point.

> That means that if you do a pg_upgrade from a 9.4 database, your built-in
> superuser will now not have rolbypassrls set, though it does in a database
> built in any other way.  Even if that doesn't have any functional effect,
> it's a recipe for confusion IMO.  So I think that the code ought to be
> "usesuper as rolbypassrls" rather than "false as rolbypassrls" for
> back branches.
>
> The only other similar case is rolreplication, which perhaps also ought
> to read as usesuper for old branches.

Agreed.  I'll take care of both and we'll make sure the new role
attributes being added will do the same for upgrades also.
Thanks!
    Stephen

On Thu, Nov 13, 2014 at 08:24:36PM -0500, Stephen Frost wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > What's bothering me is that I see this in pg_dumpall output from a 9.4
> > or earlier database:
> > 
> > ALTER ROLE postgres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN REPLICATION NOBYPASSRLS;
> 
> Ah, yeah, good point.
> 
> > That means that if you do a pg_upgrade from a 9.4 database, your built-in
> > superuser will now not have rolbypassrls set, though it does in a database
> > built in any other way.  Even if that doesn't have any functional effect,
> > it's a recipe for confusion IMO.  So I think that the code ought to be
> > "usesuper as rolbypassrls" rather than "false as rolbypassrls" for
> > back branches.
> > 
> > The only other similar case is rolreplication, which perhaps also ought
> > to read as usesuper for old branches.
> 
> Agreed.  I'll take care of both and we'll make sure the new role
> attributes being added will do the same for upgrades also.

That would make pg_dumpall less faithful for every role other than the
bootstrap superuser, a net loss.  It would be defensible to do this for
BOOTSTRAP_SUPERUSERID only.  Even there I prefer the current behavior; this is
just another of many fine details that pg_upgrade reproduces more precisely
than other pg_dumpall/pg_dump invocations.

Noah Misch <noah@leadboat.com> writes:
> On Thu, Nov 13, 2014 at 08:24:36PM -0500, Stephen Frost wrote:
>> Agreed.  I'll take care of both and we'll make sure the new role
>> attributes being added will do the same for upgrades also.

> That would make pg_dumpall less faithful for every role other than the
> bootstrap superuser, a net loss.  It would be defensible to do this for
> BOOTSTRAP_SUPERUSERID only.

Huh?  It seems difficult to argue that it's "less faithful" to do this
when the previous version didn't have the concept at all.

> Even there I prefer the current behavior; this is
> just another of many fine details that pg_upgrade reproduces more precisely
> than other pg_dumpall/pg_dump invocations.

So far as catalog contents are concerned, pg_upgrade *is* pg_dumpall.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Thu, Nov 13, 2014 at 08:24:36PM -0500, Stephen Frost wrote:
> >> Agreed.  I'll take care of both and we'll make sure the new role
> >> attributes being added will do the same for upgrades also.
>
> > That would make pg_dumpall less faithful for every role other than the
> > bootstrap superuser, a net loss.  It would be defensible to do this for
> > BOOTSTRAP_SUPERUSERID only.
>
> Huh?  It seems difficult to argue that it's "less faithful" to do this
> when the previous version didn't have the concept at all.

I believe what Noah is pointing out is that we'll end up adding
attributes which weren't there already for superusers created by users.

You're correct that we currently enable all attributes for the bootstrap
superuser and therefore a dump/restore upgrade looks different from an
initdb, unless the dump includes all new attributes for the bootstrap
superuser.

There's a couple ways to address this-

Stop enabling all the role attribute bits for the bootstrap superuser,
in which case it'd look a lot more like other superusers that a user
might create (at least, in my experience, no one bothers to set the role
attributes beyond superuser in real environments).

or

Reflect actual capability in what is viewed through the catalog.  This
might actually dovetail nicely with the role attribute representation
change which is also being discussed, were we to make pg_authid a view
which called 'has_rolX_privilege' to get the value for each attribute.
What's actually in the bitmask might end up being different, but at
least what's seen in pg_authid (and hopefully for all client tools)
would make sense.  Of course, this also has the downside that if the
superuser bit is removed later, we'd revert to whatever is actually in
the catalog for the user and that'd potentially be different for the
bootstrap superuser vs. user-created superusers.

Personally, I'm leaning towards the first as it's less clutter in the
output of psql.  If we add the role attributes proposed and continue to
enable all of them explicitly for the bootstrap superuser, the
'Attributes' column is going to get mighty wide.  I don't really see the
explicit list of attributes as helping de-mystify what is going on for
users as it's akin to root anyway- doing an 'ls' as root doesn't show
all the file permissions based on what root can do.
Thanks!
    Stephen

On Fri, Nov 14, 2014 at 08:35:25AM -0500, Stephen Frost wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > Noah Misch <noah@leadboat.com> writes:
> > > On Thu, Nov 13, 2014 at 08:24:36PM -0500, Stephen Frost wrote:
> > >> Agreed.  I'll take care of both and we'll make sure the new role
> > >> attributes being added will do the same for upgrades also.
> > 
> > > That would make pg_dumpall less faithful for every role other than the
> > > bootstrap superuser, a net loss.  It would be defensible to do this for
> > > BOOTSTRAP_SUPERUSERID only.
> > 
> > Huh?  It seems difficult to argue that it's "less faithful" to do this
> > when the previous version didn't have the concept at all.
> 
> I believe what Noah is pointing out is that we'll end up adding
> attributes which weren't there already for superusers created by users.

Yes.

> There's a couple ways to address this-
> 
> Stop enabling all the role attribute bits for the bootstrap superuser,
> in which case it'd look a lot more like other superusers that a user
> might create (at least, in my experience, no one bothers to set the role
> attributes beyond superuser in real environments).
> 
> or [...]

> Personally, I'm leaning towards the first as it's less clutter in the
> output of psql.

I'd agree for a new design, but I see too little to gain from changing it now.
Today's behavior is fine.

* Noah Misch (noah@leadboat.com) wrote:
> On Fri, Nov 14, 2014 at 08:35:25AM -0500, Stephen Frost wrote:
> > Personally, I'm leaning towards the first as it's less clutter in the
> > output of psql.
>
> I'd agree for a new design, but I see too little to gain from changing it now.
> Today's behavior is fine.

To clarify- you mean with the changes described- using usesuper for
rolreplication and rolbypassrls instead of 'false' when dumping from
older versions, correct?

Note for all- rolreplication goes back to 9.1.  Are we thinking that
change should be backpatched?
Thanks,
    Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Noah Misch (noah@leadboat.com) wrote:
>> I'd agree for a new design, but I see too little to gain from changing it now.
>> Today's behavior is fine.

> To clarify- you mean with the changes described- using usesuper for
> rolreplication and rolbypassrls instead of 'false' when dumping from
> older versions, correct?

I think Noah is arguing for leaving the pg_dumpall queries as they
stand.  I disagree, but he's entitled to his opinion.

> Note for all- rolreplication goes back to 9.1.  Are we thinking that
> change should be backpatched?

I would say it's not really worth back-patching.
        regards, tom lane

On Fri, Nov 14, 2014 at 11:24:20AM -0500, Tom Lane wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Noah Misch (noah@leadboat.com) wrote:
> >> I'd agree for a new design, but I see too little to gain from changing it now.
> >> Today's behavior is fine.
> 
> > To clarify- you mean with the changes described- using usesuper for
> > rolreplication and rolbypassrls instead of 'false' when dumping from
> > older versions, correct?
> 
> I think Noah is arguing for leaving the pg_dumpall queries as they
> stand.  I disagree, but he's entitled to his opinion.

Yes, that.  (Adopt Gilles Darold's fix, of course.)

* Noah Misch (noah@leadboat.com) wrote:
> On Fri, Nov 14, 2014 at 11:24:20AM -0500, Tom Lane wrote:
> > Stephen Frost <sfrost@snowman.net> writes:
> > > * Noah Misch (noah@leadboat.com) wrote:
> > >> I'd agree for a new design, but I see too little to gain from changing it now.
> > >> Today's behavior is fine.
> >
> > > To clarify- you mean with the changes described- using usesuper for
> > > rolreplication and rolbypassrls instead of 'false' when dumping from
> > > older versions, correct?
> >
> > I think Noah is arguing for leaving the pg_dumpall queries as they
> > stand.  I disagree, but he's entitled to his opinion.
>
> Yes, that.

Ah, ok.  I'm impartial, but I do note that we're currently inconsistent
and so I'd prefer to go one way or the other.

rolcreaterole uses usesuper, while rolreplication and rolbypassrls do
not.  Noah- would you argue that we should change rolcreaterole, which
has this behavior in all released branches (though, of course, it's only
relevant when upgrading from a pre-8.1 server where we didn't have
rolcreaterole)?  What are your thoughts on the additional role
attributes which are being discussed?

> (Adopt Gilles Darold's fix, of course.)

That's been done already.
Thanks!
    Stephen

On Thu, Nov 13, 2014 at 6:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> What's bothering me is that I see this in pg_dumpall output from a 9.4
> or earlier database:
>
> ALTER ROLE postgres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN REPLICATION NOBYPASSRLS;

What about leaving out NOBYPASSRLS and letting it go to whatever the default is?

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

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Thu, Nov 13, 2014 at 6:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > What's bothering me is that I see this in pg_dumpall output from a 9.4
> > or earlier database:
> >
> > ALTER ROLE postgres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN REPLICATION NOBYPASSRLS;
>
> What about leaving out NOBYPASSRLS and letting it go to whatever the default is?

I'd be fine with that- but would we want to do it for the other role
attributes also?  Specifically rolcreaterole and rolreplication for
older server versions.  I'm still of the opinion that we should just
drop the explicit "true" for all the role attributes for the bootstrap
superuser and then go with this suggestion to let it go to the default
for upgrades from older versions.
Thanks,
    Stephen

On Fri, Nov 14, 2014 at 2:51 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> On Thu, Nov 13, 2014 at 6:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > What's bothering me is that I see this in pg_dumpall output from a 9.4
>> > or earlier database:
>> >
>> > ALTER ROLE postgres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN REPLICATION NOBYPASSRLS;
>>
>> What about leaving out NOBYPASSRLS and letting it go to whatever the default is?
>
> I'd be fine with that- but would we want to do it for the other role
> attributes also?  Specifically rolcreaterole and rolreplication for
> older server versions.  I'm still of the opinion that we should just
> drop the explicit "true" for all the role attributes for the bootstrap
> superuser and then go with this suggestion to let it go to the default
> for upgrades from older versions.

Not sure; I was just brainstorming.

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

On Fri, Nov 14, 2014 at 11:47:49AM -0500, Stephen Frost wrote:
> > On Fri, Nov 14, 2014 at 11:24:20AM -0500, Tom Lane wrote:
> > > I think Noah is arguing for leaving the pg_dumpall queries as they
> > > stand.  I disagree, but he's entitled to his opinion.

> Ah, ok.  I'm impartial, but I do note that we're currently inconsistent
> and so I'd prefer to go one way or the other.
> 
> rolcreaterole uses usesuper, while rolreplication and rolbypassrls do
> not.  Noah- would you argue that we should change rolcreaterole, which
> has this behavior in all released branches (though, of course, it's only
> relevant when upgrading from a pre-8.1 server where we didn't have
> rolcreaterole)?

Setting aside that I wouldn't have argued for any change here, yes.  I agree
that there's no good reason to handle rolcreaterole unlike rolreplication.
The choice between their respective techniques has behavior consequences only
if you later use ALTER ROLE x NOSUPERUSER, which I have not seen done apart
from explicit ALTER ROLE testing.  Having said that, I prefer setting these
attributes to false when dumping from a version that did not have them, for
these reasons:

1) It's fail-safe.  The hypothetical ALTER ROLE x NOSUPERUSER may leave the  role with fewer privileges than expected,
neverwith more privileges than  expected.
 

2) It's more consistent with how folks create superuser accounts.  I've not  seen "CREATE USER x SUPERUSER CREATEROLE"
used. Where SUPERUSER preempts a  given role attribute, the norm among sites I've seen is to omit the  attribute.  (The
bootstrapsuperuser does turn this point on its head.)
 

3) It's cleaner in \du output.

I can't pinpoint a technical argument against your proposal to cease adding
excess attributes to the bootstrap superuser at initdb time.  It feels like a
case of the tail wagging the dog, changing antediluvian initdb behavior to
make pg_dumpall slightly more transparent.

So, if you desire to make this consistent, I recommend using rolreplication's
treatment as the gold standard.  That is to say, when dumping from an older
version, set to false any of these role attributes not existing in that
version.  Robert's proposal to emit neither NOBYPASSRLS nor BYPASSRLS is a
reasonable alternative, though less so for "pg_dumpall --clean".  It would be
defensible to send NOBYPASSRLS under --clean and omit the option otherwise;
consider that my second choice.

> What are your thoughts on the additional role
> attributes which are being discussed?

All three of rolcreaterole, rolreplication and rolbypassrls deserve the same
dumpRoles() treatment.

nm

* Noah Misch (noah@leadboat.com) wrote:
> On Fri, Nov 14, 2014 at 11:47:49AM -0500, Stephen Frost wrote:
> > rolcreaterole uses usesuper, while rolreplication and rolbypassrls do
> > not.  Noah- would you argue that we should change rolcreaterole, which
> > has this behavior in all released branches (though, of course, it's only
> > relevant when upgrading from a pre-8.1 server where we didn't have
> > rolcreaterole)?
>
> Setting aside that I wouldn't have argued for any change here, yes.  I agree
> that there's no good reason to handle rolcreaterole unlike rolreplication.
> The choice between their respective techniques has behavior consequences only
> if you later use ALTER ROLE x NOSUPERUSER, which I have not seen done apart
> from explicit ALTER ROLE testing.  Having said that, I prefer setting these
> attributes to false when dumping from a version that did not have them, for
> these reasons:
>
> 1) It's fail-safe.  The hypothetical ALTER ROLE x NOSUPERUSER may leave the
>    role with fewer privileges than expected, never with more privileges than
>    expected.
>
> 2) It's more consistent with how folks create superuser accounts.  I've not
>    seen "CREATE USER x SUPERUSER CREATEROLE" used.  Where SUPERUSER preempts a
>    given role attribute, the norm among sites I've seen is to omit the
>    attribute.  (The bootstrap superuser does turn this point on its head.)
>
> 3) It's cleaner in \du output.

I agree with these points and my experience matches yours.  The
bootstrap user is the one anomaly at those sites and makes it stand out
rather than look like the majority of the superuser accounts which
exist (where there exists more than one or two anyway).

> I can't pinpoint a technical argument against your proposal to cease adding
> excess attributes to the bootstrap superuser at initdb time.  It feels like a
> case of the tail wagging the dog, changing antediluvian initdb behavior to
> make pg_dumpall slightly more transparent.

For my 2c, it feels like it was simply an attempt to reflect actual
capabilities without consideration for what happened later rather than
any particularly technical reason, and I don't feel that original
reasoning (if that's what it was, anyway) was sound.

> So, if you desire to make this consistent, I recommend using rolreplication's
> treatment as the gold standard.  That is to say, when dumping from an older
> version, set to false any of these role attributes not existing in that
> version.  Robert's proposal to emit neither NOBYPASSRLS nor BYPASSRLS is a
> reasonable alternative, though less so for "pg_dumpall --clean".  It would be
> defensible to send NOBYPASSRLS under --clean and omit the option otherwise;
> consider that my second choice.

I don't see the point in including them for --clean..?  --clean states
that DROP commands would be added, not that existing roles would be
adjusted in some way.

As for using 'always false'- I tend to think Robert actually has it
better by using the default for users.  Consider rolinherit- that
defaults to 'true' and while it would technically be more 'safe' to set
it to false, it wouldn't have matched what we provided under the user /
group system prior to roles.  Doing this would also reduce clutter in
pg_dumpall output.

> > What are your thoughts on the additional role
> > attributes which are being discussed?
>
> All three of rolcreaterole, rolreplication and rolbypassrls deserve the same
> dumpRoles() treatment.

Agreed.
Thanks!
    Stephen

On Fri, Nov 14, 2014 at 08:39:28PM -0500, Stephen Frost wrote:
> * Noah Misch (noah@leadboat.com) wrote:
> > So, if you desire to make this consistent, I recommend using rolreplication's
> > treatment as the gold standard.  That is to say, when dumping from an older
> > version, set to false any of these role attributes not existing in that
> > version.  Robert's proposal to emit neither NOBYPASSRLS nor BYPASSRLS is a
> > reasonable alternative, though less so for "pg_dumpall --clean".  It would be
> > defensible to send NOBYPASSRLS under --clean and omit the option otherwise;
> > consider that my second choice.
> 
> I don't see the point in including them for --clean..?  --clean states
> that DROP commands would be added, not that existing roles would be
> adjusted in some way.

It does state that, but note this comment in dumpRoles():
    /*     * We dump CREATE ROLE followed by ALTER ROLE to ensure that the role     * will acquire the right properties
evenif it already exists (ie, it     * won't hurt for the CREATE to fail).  This is particularly important     * for
therole we are connected as, since even with --clean we will     * have failed to drop it.  binary_upgrade cannot
generateany errors,     * so we assume the current role is already created.     */
 

Under --clean, "the right properties" are those the role would have had if the
DROP ROLE had succeeded.  Those are necessarily independent of the pre-DROP
version of the role.  (Otherwise, you potentially get different outcomes
depending on which superuser restored the --clean dump.)

> As for using 'always false'- I tend to think Robert actually has it
> better by using the default for users.  Consider rolinherit- that
> defaults to 'true' and while it would technically be more 'safe' to set
> it to false, it wouldn't have matched what we provided under the user /
> group system prior to roles.  Doing this would also reduce clutter in
> pg_dumpall output.

My arguments and conclusion apply only to the permission-like attributes that
are subsets of SUPERUSER.  rolinherit is indeed not in that category.

* Noah Misch (noah@leadboat.com) wrote:
> On Fri, Nov 14, 2014 at 08:39:28PM -0500, Stephen Frost wrote:
> > I don't see the point in including them for --clean..?  --clean states
> > that DROP commands would be added, not that existing roles would be
> > adjusted in some way.
>
> It does state that, but note this comment in dumpRoles():
>
>         /*
>          * We dump CREATE ROLE followed by ALTER ROLE to ensure that the role
>          * will acquire the right properties even if it already exists (ie, it
>          * won't hurt for the CREATE to fail).  This is particularly important
>          * for the role we are connected as, since even with --clean we will
>          * have failed to drop it.  binary_upgrade cannot generate any errors,
>          * so we assume the current role is already created.
>          */

Ah, yes, of course.

> Under --clean, "the right properties" are those the role would have had if the
> DROP ROLE had succeeded.  Those are necessarily independent of the pre-DROP
> version of the role.  (Otherwise, you potentially get different outcomes
> depending on which superuser restored the --clean dump.)

Agreed, and in this case we'd need to set any attributes not set back to
the default, which would include having NOBYPASSRLS.

> > As for using 'always false'- I tend to think Robert actually has it
> > better by using the default for users.  Consider rolinherit- that
> > defaults to 'true' and while it would technically be more 'safe' to set
> > it to false, it wouldn't have matched what we provided under the user /
> > group system prior to roles.  Doing this would also reduce clutter in
> > pg_dumpall output.
>
> My arguments and conclusion apply only to the permission-like attributes that
> are subsets of SUPERUSER.  rolinherit is indeed not in that category.

Understood.
Thanks!
    Stephen