Re: [HACKERS] dumping database privileges broken in 9.6

[Stephen Frost]

* Noah Misch (n...@leadboat.com) wrote:
> On Fri, Jul 15, 2016 at 03:46:17PM -0400, Stephen Frost wrote:
> > * Noah Misch (n...@leadboat.com) wrote:
> > > On Sat, Jul 09, 2016 at 12:55:33AM -0400, Stephen Frost wrote:
> > > > * Noah Misch (n...@leadboat.com) wrote:
> > > > > This PostgreSQL 9.6 open item is past due for your status update.  
> > > > > Kindly send
> > > > > a status update within 24 hours, and include a date for your 
> > > > > subsequent status
> > > > > update.  Refer to the policy on open item ownership:
> > > > > http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com
> > > > 
> > > > Unfortunately, not going to make any further progress on this tonight or
> > > > over the weekend as I'm going to be out of town.  I believe I've
> > > > convinced myself that adding a template1 entry to pg_init_privs will be
> > > > both sufficient and produce the correct results, along with adjusting
> > > > the query in pg_dumpall to join through it.  Will provide an update on
> > > > Monday.
> > > 
> > > This PostgreSQL 9.6 open item is long past due for your status update.  
> > > Kindly
> > > send a status update within 24 hours, and include a date for your 
> > > subsequent
> > > status update.  (Your Tuesday posting lacked a date.)
> > 
> > Yeah, I realized that tablespaces have the same issue and have updated
> > the patch to address them as well, in the same way.
> > 
> > Going through and doing testing now.  Unfortunately, it doesn't look
> > like adding in testing of tablespaces into the TAP tests would be very
> > easy (the only TAP test that deals with tablespaces that I found was
> > pg_basebackup and that looks rather grotty..), so I'm not planning to do
> > that, at least not at this time.
> > 
> > As such, I'm planning to commit the patch with the fix+regression test
> > for database ACLs and the fix for tablespace ACLs either later today
> > or tomorrow.
> 
> Does commit 47f5bb9 fully fix this open item?

Yes, I've updated the open items wiki.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] dumping database privileges broken in 9.6

[Stephen Frost]

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Sat, Jul 16, 2016 at 4:46 AM, Stephen Frost  wrote:
> > Going through and doing testing now.  Unfortunately, it doesn't look
> > like adding in testing of tablespaces into the TAP tests would be very
> > easy (the only TAP test that deals with tablespaces that I found was
> > pg_basebackup and that looks rather grotty..), so I'm not planning to do
> > that, at least not at this time.
> 
> The cleanest way to handle that in PostgresNode would be to have a
> dedicated routine calling psql -c 'create tablespace' with tablespaces
> located in a folder $basedir/tbspace. And on top of that there should
> be the tablespace metadata saved in the context of the test, with a
> pair of (tbspc name, location). But I think that we'd need first
> stronger arguments (take more use cases) to introduce such an
> extension of the test APIs.

The way pg_basebackup handles this is to use TestLib::tempdir_short and
create a symlink with it, then to call psql to create the tablespace.  I
don't have any problem using $basedir/tbspace instead though.

What I was thinking is that we'd add a 'create_tablespace' or similar
routine to PostgresNode and then make the pg_basebackup and pg_dump
tests use it.  Otherwise, I suspect the next person who ends up writing
a 'create tablespace' into the TAP tests will use yet another location
and/or method.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] dumping database privileges broken in 9.6

[Stephen Frost]

Noah,

On Monday, July 18, 2016, Noah Misch  wrote:

> On Fri, Jul 15, 2016 at 03:46:17PM -0400, Stephen Frost wrote:
> > * Noah Misch (n...@leadboat.com ) wrote:
> > > On Sat, Jul 09, 2016 at 12:55:33AM -0400, Stephen Frost wrote:
> > > > * Noah Misch (n...@leadboat.com ) wrote:
> > > > > This PostgreSQL 9.6 open item is past due for your status update.
> Kindly send
> > > > > a status update within 24 hours, and include a date for your
> subsequent status
> > > > > update.  Refer to the policy on open item ownership:
> > > > >
> http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com
> > > >
> > > > Unfortunately, not going to make any further progress on this
> tonight or
> > > > over the weekend as I'm going to be out of town.  I believe I've
> > > > convinced myself that adding a template1 entry to pg_init_privs will
> be
> > > > both sufficient and produce the correct results, along with adjusting
> > > > the query in pg_dumpall to join through it.  Will provide an update
> on
> > > > Monday.
> > >
> > > This PostgreSQL 9.6 open item is long past due for your status
> update.  Kindly
> > > send a status update within 24 hours, and include a date for your
> subsequent
> > > status update.  (Your Tuesday posting lacked a date.)
> >
> > Yeah, I realized that tablespaces have the same issue and have updated
> > the patch to address them as well, in the same way.
> >
> > Going through and doing testing now.  Unfortunately, it doesn't look
> > like adding in testing of tablespaces into the TAP tests would be very
> > easy (the only TAP test that deals with tablespaces that I found was
> > pg_basebackup and that looks rather grotty..), so I'm not planning to do
> > that, at least not at this time.
> >
> > As such, I'm planning to commit the patch with the fix+regression test
> > for database ACLs and the fix for tablespace ACLs either later today
> > or tomorrow.
>
> Does commit 47f5bb9 fully fix this open item?
>

Yes, it does. Apologies for not closing it.

Thanks!

Stephen

Re: [HACKERS] dumping database privileges broken in 9.6

[Michael Paquier]

On Sat, Jul 16, 2016 at 4:46 AM, Stephen Frost  wrote:
> Going through and doing testing now.  Unfortunately, it doesn't look
> like adding in testing of tablespaces into the TAP tests would be very
> easy (the only TAP test that deals with tablespaces that I found was
> pg_basebackup and that looks rather grotty..), so I'm not planning to do
> that, at least not at this time.

The cleanest way to handle that in PostgresNode would be to have a
dedicated routine calling psql -c 'create tablespace' with tablespaces
located in a folder $basedir/tbspace. And on top of that there should
be the tablespace metadata saved in the context of the test, with a
pair of (tbspc name, location). But I think that we'd need first
stronger arguments (take more use cases) to introduce such an
extension of the test APIs.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dumping database privileges broken in 9.6

[Noah Misch]

On Fri, Jul 15, 2016 at 03:46:17PM -0400, Stephen Frost wrote:
> * Noah Misch (n...@leadboat.com) wrote:
> > On Sat, Jul 09, 2016 at 12:55:33AM -0400, Stephen Frost wrote:
> > > * Noah Misch (n...@leadboat.com) wrote:
> > > > This PostgreSQL 9.6 open item is past due for your status update.  
> > > > Kindly send
> > > > a status update within 24 hours, and include a date for your subsequent 
> > > > status
> > > > update.  Refer to the policy on open item ownership:
> > > > http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com
> > > 
> > > Unfortunately, not going to make any further progress on this tonight or
> > > over the weekend as I'm going to be out of town.  I believe I've
> > > convinced myself that adding a template1 entry to pg_init_privs will be
> > > both sufficient and produce the correct results, along with adjusting
> > > the query in pg_dumpall to join through it.  Will provide an update on
> > > Monday.
> > 
> > This PostgreSQL 9.6 open item is long past due for your status update.  
> > Kindly
> > send a status update within 24 hours, and include a date for your subsequent
> > status update.  (Your Tuesday posting lacked a date.)
> 
> Yeah, I realized that tablespaces have the same issue and have updated
> the patch to address them as well, in the same way.
> 
> Going through and doing testing now.  Unfortunately, it doesn't look
> like adding in testing of tablespaces into the TAP tests would be very
> easy (the only TAP test that deals with tablespaces that I found was
> pg_basebackup and that looks rather grotty..), so I'm not planning to do
> that, at least not at this time.
> 
> As such, I'm planning to commit the patch with the fix+regression test
> for database ACLs and the fix for tablespace ACLs either later today
> or tomorrow.

Does commit 47f5bb9 fully fix this open item?


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dumping database privileges broken in 9.6

[Stephen Frost]

* Noah Misch (n...@leadboat.com) wrote:
> On Sat, Jul 09, 2016 at 12:55:33AM -0400, Stephen Frost wrote:
> > * Noah Misch (n...@leadboat.com) wrote:
> > > This PostgreSQL 9.6 open item is past due for your status update.  Kindly 
> > > send
> > > a status update within 24 hours, and include a date for your subsequent 
> > > status
> > > update.  Refer to the policy on open item ownership:
> > > http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com
> > 
> > Unfortunately, not going to make any further progress on this tonight or
> > over the weekend as I'm going to be out of town.  I believe I've
> > convinced myself that adding a template1 entry to pg_init_privs will be
> > both sufficient and produce the correct results, along with adjusting
> > the query in pg_dumpall to join through it.  Will provide an update on
> > Monday.
> 
> This PostgreSQL 9.6 open item is long past due for your status update.  Kindly
> send a status update within 24 hours, and include a date for your subsequent
> status update.  (Your Tuesday posting lacked a date.)

Yeah, I realized that tablespaces have the same issue and have updated
the patch to address them as well, in the same way.

Going through and doing testing now.  Unfortunately, it doesn't look
like adding in testing of tablespaces into the TAP tests would be very
easy (the only TAP test that deals with tablespaces that I found was
pg_basebackup and that looks rather grotty..), so I'm not planning to do
that, at least not at this time.

As such, I'm planning to commit the patch with the fix+regression test
for database ACLs and the fix for tablespace ACLs either later today
or tomorrow.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] dumping database privileges broken in 9.6

[Noah Misch]

On Sat, Jul 09, 2016 at 12:55:33AM -0400, Stephen Frost wrote:
> * Noah Misch (n...@leadboat.com) wrote:
> > This PostgreSQL 9.6 open item is past due for your status update.  Kindly 
> > send
> > a status update within 24 hours, and include a date for your subsequent 
> > status
> > update.  Refer to the policy on open item ownership:
> > http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com
> 
> Unfortunately, not going to make any further progress on this tonight or
> over the weekend as I'm going to be out of town.  I believe I've
> convinced myself that adding a template1 entry to pg_init_privs will be
> both sufficient and produce the correct results, along with adjusting
> the query in pg_dumpall to join through it.  Will provide an update on
> Monday.

This PostgreSQL 9.6 open item is long past due for your status update.  Kindly
send a status update within 24 hours, and include a date for your subsequent
status update.  (Your Tuesday posting lacked a date.)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dumping database privileges broken in 9.6

[Michael Paquier]

On Wed, Jul 13, 2016 at 5:18 AM, Stephen Frost  wrote:
> Attached is a patch to address this.
>
> After much consideration and deliberation, I went with the simpler
> solution to simply dump out the database privileges based on what a new
> creation of those privileges would yield, resulting in output similar to
> pre-9.6.  We document that template1 is allowed to be dropped/recreated,
> which greatly complicates using pg_init_privs to record and produce a
> delta against the initdb-time values, as we lose the connection between
> pg_init_privs and the "template1" database as soon as it is dropped
> (something which can't be done with objects in that catalog).

+"(SELECT pg_catalog.array_agg(acl) FROM (SELECT
pg_catalog.unnest(coalesce(datacl,pg_catalog.acldefault('d',datdba)))
AS acl "
+"EXCEPT SELECT
pg_catalog.unnest(pg_catalog.acldefault('d',datdba))) as foo)"
+"AS datacl,"
+"(SELECT pg_catalog.array_agg(acl) FROM (SELECT
pg_catalog.unnest(pg_catalog.acldefault('d',datdba)) AS acl "
+"EXCEPT SELECT
pg_catalog.unnest(coalesce(datacl,pg_catalog.acldefault('d',datdba
as foo)"
+"AS rdatacl,"
It took me some time to understand that those are the GRANT and REVOKE
ACLs separated into two columns to get advantage of buildACLCommands..
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dumping database privileges broken in 9.6

[Stephen Frost]

All,

* Noah Misch (n...@leadboat.com) wrote:
> On Wed, Jun 29, 2016 at 11:50:17AM -0400, Stephen Frost wrote:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> > > Do this:
> > > 
> > > CREATE DATABASE test1;
> > > REVOKE CONNECT ON DATABASE test1 FROM PUBLIC;
> > > 
> > > Run pg_dumpall.
> > > 
> > > In 9.5, this produces
> > > 
> > > CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> > > REVOKE ALL ON DATABASE test1 FROM PUBLIC;
> > > REVOKE ALL ON DATABASE test1 FROM peter;
> > > GRANT ALL ON DATABASE test1 TO peter;
> > > GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> > > 
> > > In 9.6, this produces only
> > > 
> > > CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> > > GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> > > GRANT ALL ON DATABASE test1 TO peter;
> > > 
> > > Note that the REVOKE statements are missing.  This does not
> > > correctly recreate the original state.
> > 
> > I see what happened here, the query in dumpCreateDB() needs to be
> > adjusted to pull the default information to then pass to
> > buildACLComments(), similar to how the objects handled by pg_dump work.
> > The oversight was in thinking that databases didn't have any default
> > rights granted, which clearly isn't correct.
> > 
> > I'll take care of that in the next day or so and add an appropriate
> > regression test.
> 
> This PostgreSQL 9.6 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com

Attached is a patch to address this.

After much consideration and deliberation, I went with the simpler
solution to simply dump out the database privileges based on what a new
creation of those privileges would yield, resulting in output similar to
pre-9.6.  We document that template1 is allowed to be dropped/recreated,
which greatly complicates using pg_init_privs to record and produce a
delta against the initdb-time values, as we lose the connection between
pg_init_privs and the "template1" database as soon as it is dropped
(something which can't be done with objects in that catalog).

Comments welcome.

Thanks!

Stephen
From fc87c0a07f37d7b4bbcf067d22d31467a511e865 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Tue, 12 Jul 2016 15:37:35 -0400
Subject: [PATCH] Correctly dump database ACLs

---
 src/bin/pg_dump/pg_dumpall.c |  54 +++---
 src/bin/pg_dump/t/002_pg_dump.pl | 153 +++
 2 files changed, 195 insertions(+), 12 deletions(-)

diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index d4fb03e..99c0f90 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -1284,14 +1284,43 @@ dumpCreateDB(PGconn *conn)
 
 	PQclear(res);
 
-	/* Now collect all the information about databases to dump */
-	if (server_version >= 90300)
+
+	/*
+	 * Now collect all the information about databases to dump.
+	 *
+	 * For the database ACLs, as of 9.6, we extract both the positive (as
+	 * datacl) and negative (as rdatacl) ACLs, relative to the default ACL for
+	 * databases, which are then passed to buildACLCommands() below.
+	 *
+	 * See buildACLQueries() and buildACLCommands().
+	 *
+	 * Note that we do not support initial privileges (pg_init_privs) on
+	 * databases.
+	 */
+	if (server_version >= 90600)
+		res = executeQuery(conn,
+		   "SELECT datname, "
+		   "coalesce(rolname, (select rolname from pg_authid where oid=(select datdba from pg_database where datname='template0'))), "
+		   "pg_encoding_to_char(d.encoding), "
+		   "datcollate, datctype, datfrozenxid, datminmxid, "
+		   "datistemplate, "
+		   "(SELECT pg_catalog.array_agg(acl) FROM (SELECT pg_catalog.unnest(coalesce(datacl,pg_catalog.acldefault('d',datdba))) AS acl "
+		   "EXCEPT SELECT pg_catalog.unnest(pg_catalog.acldefault('d',datdba))) as foo)"
+		   "AS datacl,"
+		   "(SELECT pg_catalog.array_agg(acl) FROM (SELECT pg_catalog.unnest(pg_catalog.acldefault('d',datdba)) AS acl "
+		   "EXCEPT SELECT pg_catalog.unnest(coalesce(datacl,pg_catalog.acldefault('d',datdba as foo)"
+		   "AS rdatacl,"
+		   "datconnlimit, "
+		   "(SELECT spcname FROM pg_tablespace t WHERE t.oid = d.dattablespace) AS dattablespace "
+			  "FROM pg_database d LEFT JOIN pg_authid u ON (datdba = u.oid) "
+		   "WHERE datallowconn ORDER BY 1");
+	else if (server_version >= 90300)
 		res = executeQuery(conn,
 		   "SELECT datname, "
 		   "coalesce(rolname, (select rolname from pg_authid where oid=(select datdba from pg_database where datname='template0'))), "
 		   "pg_encoding_to_char(d.encoding), "
 		   "datcollate, datctype, datfrozenxid, datminmxid, "
-		   "datistemplate, datacl, datconnlimit, "
+		   "datistemplate, datacl, '' as 

Re: [HACKERS] dumping database privileges broken in 9.6

[Stephen Frost]

* Noah Misch (n...@leadboat.com) wrote:
> This PostgreSQL 9.6 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com

Unfortunately, not going to make any further progress on this tonight or
over the weekend as I'm going to be out of town.  I believe I've
convinced myself that adding a template1 entry to pg_init_privs will be
both sufficient and produce the correct results, along with adjusting
the query in pg_dumpall to join through it.  Will provide an update on
Monday.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] dumping database privileges broken in 9.6

[Noah Misch]

On Wed, Jul 06, 2016 at 07:03:33PM -0400, Stephen Frost wrote:
> * Noah Misch (n...@leadboat.com) wrote:
> > On Wed, Jun 29, 2016 at 11:50:17AM -0400, Stephen Frost wrote:
> > > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> > > > Do this:
> > > > 
> > > > CREATE DATABASE test1;
> > > > REVOKE CONNECT ON DATABASE test1 FROM PUBLIC;
> > > > 
> > > > Run pg_dumpall.
> > > > 
> > > > In 9.5, this produces
> > > > 
> > > > CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> > > > REVOKE ALL ON DATABASE test1 FROM PUBLIC;
> > > > REVOKE ALL ON DATABASE test1 FROM peter;
> > > > GRANT ALL ON DATABASE test1 TO peter;
> > > > GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> > > > 
> > > > In 9.6, this produces only
> > > > 
> > > > CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> > > > GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> > > > GRANT ALL ON DATABASE test1 TO peter;
> > > > 
> > > > Note that the REVOKE statements are missing.  This does not
> > > > correctly recreate the original state.
> > > 
> > > I see what happened here, the query in dumpCreateDB() needs to be
> > > adjusted to pull the default information to then pass to
> > > buildACLComments(), similar to how the objects handled by pg_dump work.
> > > The oversight was in thinking that databases didn't have any default
> > > rights granted, which clearly isn't correct.
> > > 
> > > I'll take care of that in the next day or so and add an appropriate
> > > regression test.
> > 
> > This PostgreSQL 9.6 open item is past due for your status update.  Kindly 
> > send
> > a status update within 24 hours, and include a date for your subsequent 
> > status
> > update.  Refer to the policy on open item ownership:
> > http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com
> 
> I've not forgotten about this and have an initial patch, but I'm
> considering if I like the way template0/template1 are handled.
> Specifically, we don't currently record their initdb-set privileges into
> pg_init_privs (unlike all other objects with initial privileges).  This
> is complicated by the idea that template1 could be dropped/recreated
> (ending up with a different OID in the process).
> 
> More to come tomorrow.

This PostgreSQL 9.6 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dumping database privileges broken in 9.6

[Stephen Frost]

All,

* Noah Misch (n...@leadboat.com) wrote:
> On Wed, Jun 29, 2016 at 11:50:17AM -0400, Stephen Frost wrote:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> > > Do this:
> > > 
> > > CREATE DATABASE test1;
> > > REVOKE CONNECT ON DATABASE test1 FROM PUBLIC;
> > > 
> > > Run pg_dumpall.
> > > 
> > > In 9.5, this produces
> > > 
> > > CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> > > REVOKE ALL ON DATABASE test1 FROM PUBLIC;
> > > REVOKE ALL ON DATABASE test1 FROM peter;
> > > GRANT ALL ON DATABASE test1 TO peter;
> > > GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> > > 
> > > In 9.6, this produces only
> > > 
> > > CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> > > GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> > > GRANT ALL ON DATABASE test1 TO peter;
> > > 
> > > Note that the REVOKE statements are missing.  This does not
> > > correctly recreate the original state.
> > 
> > I see what happened here, the query in dumpCreateDB() needs to be
> > adjusted to pull the default information to then pass to
> > buildACLComments(), similar to how the objects handled by pg_dump work.
> > The oversight was in thinking that databases didn't have any default
> > rights granted, which clearly isn't correct.
> > 
> > I'll take care of that in the next day or so and add an appropriate
> > regression test.
> 
> This PostgreSQL 9.6 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com

I've not forgotten about this and have an initial patch, but I'm
considering if I like the way template0/template1 are handled.
Specifically, we don't currently record their initdb-set privileges into
pg_init_privs (unlike all other objects with initial privileges).  This
is complicated by the idea that template1 could be dropped/recreated
(ending up with a different OID in the process).

More to come tomorrow.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] dumping database privileges broken in 9.6

[Stephen Frost]

Noah, all,

On Saturday, July 2, 2016, Noah Misch  wrote:

> On Wed, Jun 29, 2016 at 11:50:17AM -0400, Stephen Frost wrote:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com )
> wrote:
> > > Do this:
> > >
> > > CREATE DATABASE test1;
> > > REVOKE CONNECT ON DATABASE test1 FROM PUBLIC;
> > >
> > > Run pg_dumpall.
> > >
> > > In 9.5, this produces
> > >
> > > CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> > > REVOKE ALL ON DATABASE test1 FROM PUBLIC;
> > > REVOKE ALL ON DATABASE test1 FROM peter;
> > > GRANT ALL ON DATABASE test1 TO peter;
> > > GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> > >
> > > In 9.6, this produces only
> > >
> > > CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> > > GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> > > GRANT ALL ON DATABASE test1 TO peter;
> > >
> > > Note that the REVOKE statements are missing.  This does not
> > > correctly recreate the original state.
> >
> > I see what happened here, the query in dumpCreateDB() needs to be
> > adjusted to pull the default information to then pass to
> > buildACLComments(), similar to how the objects handled by pg_dump work.
> > The oversight was in thinking that databases didn't have any default
> > rights granted, which clearly isn't correct.
> >
> > I'll take care of that in the next day or so and add an appropriate
> > regression test.
>
> This PostgreSQL 9.6 open item is past due for your status update.  Kindly
> send
> a status update within 24 hours, and include a date for your subsequent
> status
> update.  Refer to the policy on open item ownership:
>
> http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com
>

Will work on it tomorrow but for a deadline for next status update, I'll
say Tuesday (which I expect is when I'll commit the fix) as it's a holiday
weekend in the US.

Thanks!

Stephen

Re: [HACKERS] dumping database privileges broken in 9.6

[Noah Misch]

On Wed, Jun 29, 2016 at 11:50:17AM -0400, Stephen Frost wrote:
> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> > Do this:
> > 
> > CREATE DATABASE test1;
> > REVOKE CONNECT ON DATABASE test1 FROM PUBLIC;
> > 
> > Run pg_dumpall.
> > 
> > In 9.5, this produces
> > 
> > CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> > REVOKE ALL ON DATABASE test1 FROM PUBLIC;
> > REVOKE ALL ON DATABASE test1 FROM peter;
> > GRANT ALL ON DATABASE test1 TO peter;
> > GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> > 
> > In 9.6, this produces only
> > 
> > CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> > GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> > GRANT ALL ON DATABASE test1 TO peter;
> > 
> > Note that the REVOKE statements are missing.  This does not
> > correctly recreate the original state.
> 
> I see what happened here, the query in dumpCreateDB() needs to be
> adjusted to pull the default information to then pass to
> buildACLComments(), similar to how the objects handled by pg_dump work.
> The oversight was in thinking that databases didn't have any default
> rights granted, which clearly isn't correct.
> 
> I'll take care of that in the next day or so and add an appropriate
> regression test.

This PostgreSQL 9.6 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dumping database privileges broken in 9.6

[Stephen Frost]

Peter,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> Do this:
> 
> CREATE DATABASE test1;
> REVOKE CONNECT ON DATABASE test1 FROM PUBLIC;
> 
> Run pg_dumpall.
> 
> In 9.5, this produces
> 
> CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> REVOKE ALL ON DATABASE test1 FROM PUBLIC;
> REVOKE ALL ON DATABASE test1 FROM peter;
> GRANT ALL ON DATABASE test1 TO peter;
> GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> 
> In 9.6, this produces only
> 
> CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> GRANT ALL ON DATABASE test1 TO peter;
> 
> Note that the REVOKE statements are missing.  This does not
> correctly recreate the original state.

I see what happened here, the query in dumpCreateDB() needs to be
adjusted to pull the default information to then pass to
buildACLComments(), similar to how the objects handled by pg_dump work.
The oversight was in thinking that databases didn't have any default
rights granted, which clearly isn't correct.

I'll take care of that in the next day or so and add an appropriate
regression test.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] dumping database privileges broken in 9.6

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Jun 28, 2016 at 11:12 PM, Peter Eisentraut
>  wrote:
> > Do this:
> >
> > CREATE DATABASE test1;
> > REVOKE CONNECT ON DATABASE test1 FROM PUBLIC;
> >
> > Run pg_dumpall.
> >
> > In 9.5, this produces
> >
> > CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> > REVOKE ALL ON DATABASE test1 FROM PUBLIC;
> > REVOKE ALL ON DATABASE test1 FROM peter;
> > GRANT ALL ON DATABASE test1 TO peter;
> > GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> >
> > In 9.6, this produces only
> >
> > CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> > GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> > GRANT ALL ON DATABASE test1 TO peter;
> >
> > Note that the REVOKE statements are missing.  This does not correctly
> > recreate the original state.
> 
> If I were a betting man, I'd bet that one of Stephen Frost's pg_dump
> commits broke this.  But we'd have to bisect to be sure.

Wouldn't be too surprising.  I'm planning to look into this a bit later
today.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] dumping database privileges broken in 9.6

[Robert Haas]

On Tue, Jun 28, 2016 at 11:12 PM, Peter Eisentraut
 wrote:
> Do this:
>
> CREATE DATABASE test1;
> REVOKE CONNECT ON DATABASE test1 FROM PUBLIC;
>
> Run pg_dumpall.
>
> In 9.5, this produces
>
> CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> REVOKE ALL ON DATABASE test1 FROM PUBLIC;
> REVOKE ALL ON DATABASE test1 FROM peter;
> GRANT ALL ON DATABASE test1 TO peter;
> GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
>
> In 9.6, this produces only
>
> CREATE DATABASE test1 WITH TEMPLATE = template0 OWNER = peter;
> GRANT TEMPORARY ON DATABASE test1 TO PUBLIC;
> GRANT ALL ON DATABASE test1 TO peter;
>
> Note that the REVOKE statements are missing.  This does not correctly
> recreate the original state.

If I were a betting man, I'd bet that one of Stephen Frost's pg_dump
commits broke this.  But we'd have to bisect to be sure.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers