Thread: "Re: Question about grant create on database and pg_dump/pg_dumpall

<div dir="ltr"><span style="font-size:12.8px">On Tue, Jul 5, 2016 at 06:39 AM, Haribabu Kommi</span><br
style="font-size:12.8px"/><span style="font-size:12.8px">kommi(dot)haribabu(at)gmail(</span><span
style="font-size:12.8px">do<wbr/>t)com wrote:</span><br style="font-size:12.8px" /><br style="font-size:12.8px"
/><blockquotestyle="font-size:12.8px;margin:0px 0px 0px 40px;border:none;padding:0px">Still i feel the GRANT statements
shouldbe present, as the create<br />database statement<br />is generated only with -C option. So attached patch
producesthe GRANT<br />statements based<br />on the -x option.</blockquote><br style="font-size:12.8px" /><span
style="font-size:12.8px">Theattached patch does the job fine. However, I am a little skeptical about this addition,
since,it is clearly mentioned in the documentation of pg_dump that it would not restore global objects, then why
expectingthis. This addditon makes pg_dump -C somewhat special as now it is restoring these grant statements. Only if
weconsider the popular method of dump-restore mentioned in the thread (</span><a
href="https://www.postgresql.org/message-id/E1VYMqi-0001P4-P4%40wrigleys.postgresql.org"style="font-size:12.8px"
target="_blank">https://www.postgresql.org/me<wbr/>ssage-id/E1VYMqi-0001P4-P4%40w<wbr />rigleys.postgresql.org</a><span
style="font-size:12.8px">)with pg_dumpall -g and then individual pg_dump, then it would be helpful to have this
patch.</span><brstyle="font-size:12.8px" /><div style="font-size:12.8px"><br /></div><div
style="font-size:12.8px"><spanstyle="font-size:12.8px">Regards,</span></div><div style="font-size:12.8px"><span
style="font-size:12.8px">RafiaSabih</span></div><div style="font-size:12.8px"><span
style="font-size:12.8px">EnterpriseDB:</span><spanstyle="color:rgb(80,0,80);font-size:12.8px">: </span><a
href="http://www.enterprisedb.com/"rel="noreferrer" style="font-size:12.8px" target="_blank">http://www.ente<wbr
/>rprisedb.com</a></div></div>

On Thu, Sep 29, 2016 at 4:04 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> I am also not sure whether pg_dumpall -g and then individual pg_dump
> is the more widely used approach or not?

That's the approach I normally recommend.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Sep 29, 2016 at 4:04 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> I am also not sure whether pg_dumpall -g and then individual pg_dump
>> is the more widely used approach or not?

> That's the approach I normally recommend.

The fundamental thing we have to do in order to move forward on this is
to rethink what's the division of labor between pg_dump and pg_dumpall.
I find the patch as presented quite unacceptable because it's made no
effort to do that (or even to touch the documentation).

What do people think of this sketch:

1. pg_dump without --create continues to do what it does today, ie it just
dumps objects within the database, assuming that database-level properties
will already be set correctly for the target database.

2. pg_dump with --create creates the target database and also sets all
database-level properties (ownership, ACLs, ALTER DATABASE SET, etc etc).

3. pg_dumpall loses all code relating to individual-database creation
and property setting and instead relies on pg_dump --create to do that.
This would leave only the code relating to "pg_dumpall -g" (ie, dump roles
and tablespaces) within pg_dumpall itself.

One thing that would still be messy is that presumably "pg_dumpall -g"
would issue ALTER ROLE SET commands, but it's unclear what to do with
ALTER ROLE IN DATABASE SET commands.  Should those become part of
"pg_dump --create"'s charter?  It seems like not, but I'm not certain.

Another thing that requires some thought is that pg_dumpall is currently
willing to dump ACLs and other properties for template1/template0, though
it does not invoke pg_dump on them.  If we wanted to preserve that
behavior while still moving the code that does those things to pg_dump,
pg_dump would have to grow an option that would let it do that.  But
I'm not sure how much of that behavior is actually sensible.

This would probably take a pg_dump archive version bump, since I think
we don't currently record enough information for --create to do this
(and we can't just cram the extra commands into the DATABASE entry,
since we don't know whether --create will be specified to pg_restore).
But we've done those before.

Thoughts?  Is there a better way to look at this?
        regards, tom lane

On Fri, Sep 30, 2016 at 12:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The fundamental thing we have to do in order to move forward on this is
> to rethink what's the division of labor between pg_dump and pg_dumpall.
> I find the patch as presented quite unacceptable because it's made no
> effort to do that (or even to touch the documentation).

Patch marked as returned with feedback for now.

> 1. pg_dump without --create continues to do what it does today, ie it just
> dumps objects within the database, assuming that database-level properties
> will already be set correctly for the target database.
>
> 2. pg_dump with --create creates the target database and also sets all
> database-level properties (ownership, ACLs, ALTER DATABASE SET, etc etc).
>
> 3. pg_dumpall loses all code relating to individual-database creation
> and property setting and instead relies on pg_dump --create to do that.
> This would leave only the code relating to "pg_dumpall -g" (ie, dump roles
> and tablespaces) within pg_dumpall itself.

I saw a couple of people willing to have that on those mailling lists
over the years... So +1 for the idea.

> One thing that would still be messy is that presumably "pg_dumpall -g"
> would issue ALTER ROLE SET commands, but it's unclear what to do with
> ALTER ROLE IN DATABASE SET commands.  Should those become part of
> "pg_dump --create"'s charter?  It seems like not, but I'm not certain.

I guess that we need to think about simplifying the restore flow if we
have the occasion:
1) First restore the result of pg_dumpall -g
2) Restore the state of each database's dump --create
pg_dump --create assigns the created database to its rightful owner,
so it assumes that the role has been already created. If we do not
include those commands in pg_dump --create we make the whole restore
scenario more complicated.

> Another thing that requires some thought is that pg_dumpall is currently
> willing to dump ACLs and other properties for template1/template0, though
> it does not invoke pg_dump on them.  If we wanted to preserve that
> behavior while still moving the code that does those things to pg_dump,
> pg_dump would have to grow an option that would let it do that.  But
> I'm not sure how much of that behavior is actually sensible.

An extra option looks like the way to go. There are people that
willingly set up ACLs and we could just document that something like
pg_dump --template needs to be run to generate the template's dump
values.

> This would probably take a pg_dump archive version bump, since I think
> we don't currently record enough information for --create to do this
> (and we can't just cram the extra commands into the DATABASE entry,
> since we don't know whether --create will be specified to pg_restore).
> But we've done those before.

Yep.

(By the way, last time I saw dump/restore integration into an internal
system we have finished by using pg_dumpall -g and then pg_dump on a
given DB for simplicity)
-- 
Michael

On Thu, Sep 29, 2016 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The fundamental thing we have to do in order to move forward on this is
> to rethink what's the division of labor between pg_dump and pg_dumpall.
> I find the patch as presented quite unacceptable because it's made no
> effort to do that (or even to touch the documentation).
>
> What do people think of this sketch:
>
> 1. pg_dump without --create continues to do what it does today, ie it just
> dumps objects within the database, assuming that database-level properties
> will already be set correctly for the target database.
>
> 2. pg_dump with --create creates the target database and also sets all
> database-level properties (ownership, ACLs, ALTER DATABASE SET, etc etc).
>
> 3. pg_dumpall loses all code relating to individual-database creation
> and property setting and instead relies on pg_dump --create to do that.
> This would leave only the code relating to "pg_dumpall -g" (ie, dump roles
> and tablespaces) within pg_dumpall itself.

Seems like a good sketch.

> One thing that would still be messy is that presumably "pg_dumpall -g"
> would issue ALTER ROLE SET commands, but it's unclear what to do with
> ALTER ROLE IN DATABASE SET commands.  Should those become part of
> "pg_dump --create"'s charter?  It seems like not, but I'm not certain.

I could go either way on this.

> Another thing that requires some thought is that pg_dumpall is currently
> willing to dump ACLs and other properties for template1/template0, though
> it does not invoke pg_dump on them.  If we wanted to preserve that
> behavior while still moving the code that does those things to pg_dump,
> pg_dump would have to grow an option that would let it do that.  But
> I'm not sure how much of that behavior is actually sensible.

I'm not sure, either, but it's usually bad when dump-and-restore
doesn't dump-and-restore things which a user might reasonably have
changed.  That tends to lead to bug reports and/or pg_upgrade
failures.

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

On Thu, Sep 29, 2016 at 11:29:09AM -0400, Tom Lane wrote:
> The fundamental thing we have to do in order to move forward on this is
> to rethink what's the division of labor between pg_dump and pg_dumpall.
> I find the patch as presented quite unacceptable because it's made no
> effort to do that (or even to touch the documentation).
> 
> What do people think of this sketch:

This looks good so far.  It covers most of the problems from TODO item
"Refactor handling of database attributes between pg_dump and pg_dumpall".

> 1. pg_dump without --create continues to do what it does today, ie it just
> dumps objects within the database, assuming that database-level properties
> will already be set correctly for the target database.

dumpDatabase() isn't fully committed to that doctrine today; it skips most
database properties, but it does dump COMMENT ON DATABASE and any
pg_shseclabel entry for the database.  Would you make it stop doing that?

> One thing that would still be messy is that presumably "pg_dumpall -g"
> would issue ALTER ROLE SET commands, but it's unclear what to do with
> ALTER ROLE IN DATABASE SET commands.  Should those become part of
> "pg_dump --create"'s charter?  It seems like not, but I'm not certain.

"pg_dump --create" should emit ALTER ROLE IN DATABASE SET.  Regeardless,
pg_dump needs the roles in place for object ownership and ALTER DEFAULT
PRIVILEGES.  The latter is conceptually similar to ALTER ROLE IN DATABASE SET.

> Another thing that requires some thought is that pg_dumpall is currently
> willing to dump ACLs and other properties for template1/template0, though
> it does not invoke pg_dump on them.  If we wanted to preserve that
> behavior while still moving the code that does those things to pg_dump,
> pg_dump would have to grow an option that would let it do that.  But
> I'm not sure how much of that behavior is actually sensible.

It's appropriate to restore the template1/template0 attributes one can cover
without connecting to the database (pg_database, pg_db_role_setting,
pg_shseclabel, pg_shdescription).  template0 modifications that require a
connection are essentially unsupported, so ignore those.  I think, ideally, a
full-cluster dump should include template1 contents.  For template1, then, one
would want pg_dump to emit everything except the CREATE DATABASE statement.
That's a regrettable wart.

Thanks,
nm