Date: Mon, 20 May 2019 17:27:05 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Bossart, Nathan" <bossartn@amazon.com>
Cc: Haribabu Kommi <kommi.haribabu@gmail.com>,
	"pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #15788: 'pg_dump --create' orders database GRANTs incorrectly
Message-ID: <20190520082705.GA1921@paquier.xyz>
References: <15788-4e18847520ebcc75@postgresql.org>
 <9C9271CF-9A7A-45AE-B034-1246A93788C9@amazon.com>
 <CAJrrPGcsS_=bTuKvPOg6TEgkrc9PaEJVCFmhm=z_7VKpLnjxOg@mail.gmail.com>
 <4D066CB2-B177-40B4-8510-9318B9A184CE@amazon.com>
 <20190504123221.GA2127@paquier.xyz>
 <0AF6E4BC-EE9A-4158-85BE-3EAE4308D228@amazon.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="0eh6TmSyL6TZE2Uz"
Content-Disposition: inline
In-Reply-To: <0AF6E4BC-EE9A-4158-85BE-3EAE4308D228@amazon.com>
Precedence: bulk


--0eh6TmSyL6TZE2Uz
Content-Type: multipart/mixed; boundary="fdj2RfSjLxBAspz7"
Content-Disposition: inline


--fdj2RfSjLxBAspz7
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, May 06, 2019 at 04:08:47PM +0000, Bossart, Nathan wrote:
> Added here: https://commitfest.postgresql.org/23/2111/

Thanks for adding the patch to the CF.

With your patch attached the difference in the dump is plain:
 REVOKE CONNECT,TEMPORARY ON DATABASE mydb FROM PUBLIC;
+GRANT TEMPORARY ON DATABASE mydb TO c_user WITH GRANT OPTION;
 SET SESSION AUTHORIZATION c_user;
 GRANT TEMPORARY ON DATABASE mydb TO a_user;
 RESET SESSION AUTHORIZATION;
-GRANT TEMPORARY ON DATABASE mydb TO c_user WITH GRANT OPTION;

So what happens is that the GRANT command to a_user fails when
switching to the session context to c_user as this user does not have
yet the authorization to perform this command.  If the GRANT
permissions assigned to c_user are moved prior its actual actions then
the restore is able to work.  I have been looking at it, and wondered
first if we could have just used buildACLQueries(), until I noticed
that we don't support initial privileges on databases, so the patch
you have sent looks fine to me.

I had first a hard time parsing the subqueries added, so I have
tweaked your patch with more indentation, and a comment block with
more details about why we need to preserve the ACL ordering (you will
note that I don't have a lot of imagination here).

v12 beta1 is going to ship soon, so let's wait for the version to be
tagged before committing it.
--
Michael

--fdj2RfSjLxBAspz7
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="database-acls-v3.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a8f1ad7333..8931fc4014 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -2664,20 +2664,41 @@ dumpDatabase(Archive *fout)
=20
 	pg_log_info("saving database definition");
=20
-	/* Fetch the database-level properties for this database */
+	/*
+	 * Fetch the database-level properties for this database.
+	 *
+	 * The order in which privileges are in the ACL string (the order they
+	 * have been GRANT'd in, which the backend maintains) must be preserved to
+	 * ensure that GRANTs WITH GRANT OPTION and subsequent GRANTs based on
+	 * those are dumped in the correct order.  Note that initial privileges
+	 * (pg_init_privs) are not supported on databases, so this logic cannot
+	 * make use of buildACLQueries().
+	 */
 	if (fout->remoteVersion >=3D 90600)
 	{
 		appendPQExpBuffer(dbQry, "SELECT tableoid, oid, datname, "
 						  "(%s datdba) AS dba, "
 						  "pg_encoding_to_char(encoding) AS encoding, "
 						  "datcollate, datctype, datfrozenxid, datminmxid, "
-						  "(SELECT array_agg(acl ORDER BY acl::text COLLATE \"C\") FROM ( "
-						  "  SELECT unnest(coalesce(datacl,acldefault('d',datdba))) AS acl "
-						  "  EXCEPT SELECT unnest(acldefault('d',datdba))) as datacls)"
+						  "(SELECT array_agg(acl ORDER BY row_n) FROM "
+						  "  (SELECT acl, row_n FROM "
+						  "     unnest(coalesce(datacl,acldefault('d',datdba))) "
+						  "     WITH ORDINALITY AS perm(acl,row_n) "
+						  "   WHERE NOT EXISTS ( "
+						  "     SELECT 1 "
+						  "     FROM unnest(acldefault('d',datdba)) "
+						  "       AS init(init_acl) "
+						  "     WHERE acl =3D init_acl)) as datacls) "
 						  " AS datacl, "
-						  "(SELECT array_agg(acl ORDER BY acl::text COLLATE \"C\") FROM ( "
-						  "  SELECT unnest(acldefault('d',datdba)) AS acl "
-						  "  EXCEPT SELECT unnest(coalesce(datacl,acldefault('d',datdba)))) =
as rdatacls)"
+						  "(SELECT array_agg(acl ORDER BY row_n) FROM "
+						  "  (SELECT acl, row_n FROM "
+						  "     unnest(acldefault('d',datdba)) "
+						  "     WITH ORDINALITY AS initp(acl,row_n) "
+						  "   WHERE NOT EXISTS ( "
+						  "     SELECT 1 "
+						  "     FROM unnest(coalesce(datacl,acldefault('d',datdba))) "
+						  "       AS permp(orig_acl) "
+						  "     WHERE acl =3D orig_acl)) AS rdatacls)"
 						  " AS rdatacl, "
 						  "datistemplate, datconnlimit, "
 						  "(SELECT spcname FROM pg_tablespace t WHERE t.oid =3D dattablespac=
e) AS tablespace, "

--fdj2RfSjLxBAspz7--

--0eh6TmSyL6TZE2Uz
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlziZNgACgkQnvQgOdby
QH08bBAAleehAYyxvBACl28hVznRKdlk0NdoliYRtQ2ZVjsrAu5iJBEfkB9Ljb4v
CuXhvaJjeTWwZnGo4ZoFo7z+GvH0LRaOKRHvb3htzTL6VwQERqxZe1qhWh19dopS
om4JEqXIFw68FLvMlApODg/oB+YqLPSC3r8pxZmS4CYS+kKzPPcQzVQtuyUrMaz8
t2sAzCwuGlFfA4ZcxzE4EWF/N5m4NDwdkHfrg5GKRtcd4aJlWogC8vQ3E1K/wKrC
VI0y3uWCyAvrYA5WUDENdYLoSS8CyfAWVogZfwPqrg0Ha5ah6Azc2o005iteMOhl
snZsajg3WQpdAQkQnAbrABlzX8a4p27PT9zpuwxGVvqCFG3mzUE4GABHnXn62yHs
JN7o2dSfolx6KvFBAo0FXspp5maj7fBKgOHmttDq/Xr4wtK+xy27e7OldAZ2t9fP
xHr6STMCi0Dctcz3MX3RJvFNB2UHREoxZIbbt8l4AM0e6UKzOFPEBW6WqhuBtZEv
UP9+moGFFQ0pAG0/nx0+4aJ0W+0QuZ/OFzlZhMx4Bbh8r0OrUkZOFvCHqaRwrpbB
WbiKHtXsrq6d72kH4q+EpGnr559wArDrecS/Tzu1Ow6WBr972nI5bYmIp1NrrGYT
cRCFPs/sp+t1ep1EyuPRGdKfCnPzVfmkM5QdAY0D4vd9eMG8noQ=
=haGR
-----END PGP SIGNATURE-----

--0eh6TmSyL6TZE2Uz--