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

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, 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 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