Tom, all, * Stephen Frost (sfr...@snowman.net) wrote: > This needs more cleanup, testing, and comments explaining why we're > doing this (and then perhaps comments, somewhere.. in the backend ACL > code that explains that the ordering needs to be preserved), but the > basic idea seems sound to me and the case you presented does work with > this patch (for me, at least) whereas what's in master didn't.
Alright, here's an updated patch which cleans things up a bit and adds comments to explain what's going on. I also updated the comments in acl.h to explain that ordering actually does matter. I've tried a bit to break the ordering in the backend a bit but there could probably be more effort put into that, if I'm being honest. Still, this definitely fixes the case which was being complained about and therefore is a step in the right direction. It's a bit late here, so I'll push this in the morning and watch the buildfarm. Thanks! Stephen
From cbca78a387ecfed9570bd079541ec7906c990f0f Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfr...@snowman.net>
Date: Thu, 3 Aug 2017 21:23:37 -0400
Subject: [PATCH] Fix ordering in pg_dump of GRANTs
The order in which GRANTs are output is important as GRANTs which have
been GRANT'd by individuals via WITH GRANT OPTION GRANTs have to come
after the GRANT which included the WITH GRANT OPTION. This happens
naturally in the backend during normal operation as we only change
existing ACLs in-place, only add new ACLs to the end, and when removing
an ACL we remove any which depend on it also.
Also, adjust the comments in acl.h to make this clear.
Unfortunately, the updates to pg_dump to handle initial privileges
involved pulling apart ACLs and then combining them back together and
could end up putting them back together in an invalid order, leading to
dumps which wouldn't restore.
Fix this by adjusting the queries used by pg_dump to ensure that the
ACLs are rebuilt in the same order in which they were originally.
Back-patch to 9.6 where the changes for initial privileges were done.
---
src/bin/pg_dump/dumputils.c | 51 ++++++++++++++++++++++++++++++++-------------
src/include/utils/acl.h | 14 ++++++++++---
2 files changed, 47 insertions(+), 18 deletions(-)
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index dfc6118..67049a6 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -722,21 +722,36 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
* We always perform this delta on all ACLs and expect that by the time
* these are run the initial privileges will be in place, even in a binary
* upgrade situation (see below).
+ *
+ * Finally, the order in which privileges are in the ACL string (the order
+ * they 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.
*/
- printfPQExpBuffer(acl_subquery, "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s)))) as foo)",
+ printfPQExpBuffer(acl_subquery,
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
+ "WITH ORDINALITY AS perm(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+ "AS init(init_acl) WHERE acl = init_acl)) as foo)",
acl_column,
obj_kind,
acl_owner,
obj_kind,
acl_owner);
- printfPQExpBuffer(racl_subquery, "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s)))) as foo)",
+ printfPQExpBuffer(racl_subquery,
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+ "WITH ORDINALITY AS initp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
+ "AS permp(orig_acl) WHERE acl = orig_acl)) as foo)",
obj_kind,
acl_owner,
acl_column,
@@ -761,19 +776,25 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
{
printfPQExpBuffer(init_acl_subquery,
"CASE WHEN privtype = 'e' THEN "
- "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(pip.initprivs) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(pg_catalog.acldefault(%s,%s))) as foo) END",
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM pg_catalog.unnest(pip.initprivs) "
+ "WITH ORDINALITY AS initp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) "
+ "AS privm(orig_acl) WHERE acl = orig_acl)) as foo) END",
obj_kind,
acl_owner);
printfPQExpBuffer(init_racl_subquery,
"CASE WHEN privtype = 'e' THEN "
"(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(pip.initprivs)) as foo) END",
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) "
+ "WITH ORDINALITY AS privp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM pg_catalog.unnest(pip.initprivs) "
+ "AS initp(init_acl) WHERE acl = init_acl)) as foo) END",
obj_kind,
acl_owner);
}
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 43273ea..316eef9 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -12,9 +12,17 @@
* NOTES
* An ACL array is simply an array of AclItems, representing the union
* of the privileges represented by the individual items. A zero-length
- * array represents "no privileges". There are no assumptions about the
- * ordering of the items, but we do expect that there are no two entries
- * in the array with the same grantor and grantee.
+ * array represents "no privileges".
+ *
+ * The order of items in the array is important as client utilities (in
+ * particular, pg_dump, though possibly other clients) expect to be able
+ * to issue GRANTs in the ordering of the items in the array. The reason
+ * this matters is that GRANTs WITH GRANT OPTION must be before any GRANTs
+ * which depend on it. This happens naturally in the backend during
+ * operations as we update ACLs in-place, new items are appended, and
+ * existing entries are only removed if there's no dependency on them (no
+ * GRANT can been based on it, or, if there was, those GRANTs are also
+ * removed).
*
* For backward-compatibility purposes we have to allow null ACL entries
* in system catalogs. A null ACL will be treated as meaning "default
--
2.7.4
signature.asc
Description: Digital signature
