public inbox for [email protected]help / color / mirror / Atom feed
PROPERTY GRAPH pg_dump ACL minimization 11+ messages / 4 participants [nested] [flat]
* PROPERTY GRAPH pg_dump ACL minimization @ 2026-06-30 02:33 Noah Misch <[email protected]> 0 siblings, 2 replies; 11+ messages in thread From: Noah Misch @ 2026-06-30 02:33 UTC (permalink / raw) To: pgsql-hackers pg_dump doesn't do its normal ACL minimization for the new PROPERTY GRAPH feature. Patch attached. See log message for details. Most of the patch bulk (modest as it is) exists to keep support for dumping from beta1. I'm not sure whether it was worth bothering. Breaking dump from a beta is without precedent known to me, so I just erred on the side of not breaking it. If we were to decide pg_dump could drop support for betas, I'd be fine with that. This entails a catversion bump on the v19 branch. If the master branch already has a post-branch catversion bump by then, the two catversions should remain distinct. I'll use yyyymmdd1 for v19 and yyyymmdd2 for master. That feels cleanest to me, since it uses the *2 value where it will be shortest-lived. There's precedent in 20b6847 (master) / e256312 (v15). From: Noah Misch <[email protected]> Fix pg_dump ACL minimization for PROPERTY GRAPH. Adding a GRANT caused pg_dump to emit a useless REVOKE + GRANT of owner privileges, as seen in a dump of the regression database: REVOKE ALL ON PROPERTY GRAPH graph_rls_schema.cabinet FROM nm; GRANT ALL ON PROPERTY GRAPH graph_rls_schema.cabinet TO nm; GRANT ALL ON PROPERTY GRAPH graph_rls_schema.cabinet TO PUBLIC; For normal dumps, this has no functional consequences. For --no-owner restores, the extra statements may fail or locate unrelated users of the destination cluster. The problem was pg_dump assuming NULL relacl implies acldefault('r'), the default for TABLE. Fix by teaching acldefault() to retrieve the PROPERTY GRAPH default ACL. So pg_dump can still dump from 19beta1, use acldefault('g') for v20+ only. For v19, use a hard-coded snapshot of the v19 default. information_schema.pg_property_graph_privileges also misused acldefault('r'), but its "c.prtype IN ('SELECT')" predicate compensated for it. Switch to the new acldefault('g') for clarity. Bump catversion since a new view won't work with old binaries. Back-patch to v19, which introduced PROPERTY GRAPH. Reviewed-by: FIXME Discussion: https://postgr.es/m/FIXME Backpatch-through: 19 diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql index 4f0e249..624d538 100644 --- a/src/backend/catalog/information_schema.sql +++ b/src/backend/catalog/information_schema.sql @@ -3328,7 +3328,7 @@ CREATE VIEW pg_property_graph_privileges AS THEN 'YES' ELSE 'NO' END AS yes_or_no) AS is_grantable FROM ( - SELECT oid, relname, relnamespace, relkind, relowner, (aclexplode(coalesce(relacl, acldefault('r', relowner)))).* FROM pg_class + SELECT oid, relname, relnamespace, relkind, relowner, (aclexplode(coalesce(relacl, acldefault('g', relowner)))).* FROM pg_class ) AS c (oid, relname, relnamespace, relkind, relowner, grantor, grantee, prtype, grantable), pg_namespace nc, pg_authid u_grantor, diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index 01caa12..e2547d7 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -956,6 +956,9 @@ acldefault_sql(PG_FUNCTION_ARGS) case 'c': objtype = OBJECT_COLUMN; break; + case 'g': + objtype = OBJECT_PROPGRAPH; + break; case 'r': objtype = OBJECT_TABLE; break; diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index c56437d..41b9531 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -7358,8 +7358,17 @@ getTables(Archive *fout, int *numTables) "c.relhastriggers, c.relpersistence, " "c.reloftype, " "c.relacl, " - "acldefault(CASE WHEN c.relkind = " CppAsString2(RELKIND_SEQUENCE) - " THEN 's'::\"char\" ELSE 'r'::\"char\" END, c.relowner) AS acldefault, " + "acldefault(CASE" + " WHEN c.relkind = " CppAsString2(RELKIND_PROPGRAPH)); + /* 19beta1 didn't support acldefault('g'), so we'll fix that below */ + appendPQExpBufferStr(query, + fout->remoteVersion >= 200000 ? + " THEN 'g'::\"char\"" : + " THEN NULL"); + appendPQExpBufferStr(query, + " WHEN c.relkind = " CppAsString2(RELKIND_SEQUENCE) + " THEN 's'::\"char\"" + " ELSE 'r'::\"char\" END, c.relowner) AS acldefault, " "CASE WHEN c.relkind = " CppAsString2(RELKIND_FOREIGN_TABLE) " THEN " "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) " "ELSE 0 END AS foreignserver, " @@ -7579,7 +7588,7 @@ getTables(Archive *fout, int *numTables) tblinfo[i].dobj.namespace = findNamespace(atooid(PQgetvalue(res, i, i_relnamespace))); tblinfo[i].dacl.acl = pg_strdup(PQgetvalue(res, i, i_relacl)); - tblinfo[i].dacl.acldefault = pg_strdup(PQgetvalue(res, i, i_acldefault)); + /* acldefault computed below */ tblinfo[i].dacl.privtype = 0; tblinfo[i].dacl.initprivs = NULL; tblinfo[i].relkind = *(PQgetvalue(res, i, i_relkind)); @@ -7631,6 +7640,28 @@ getTables(Archive *fout, int *numTables) tblinfo[i].is_identity_sequence = (strcmp(PQgetvalue(res, i, i_is_identity_sequence), "t") == 0); tblinfo[i].ispartition = (strcmp(PQgetvalue(res, i, i_ispartition), "t") == 0); + if (tblinfo[i].relkind == RELKIND_PROPGRAPH && + !(fout->remoteVersion >= 200000)) + { + PQExpBuffer aclarray = createPQExpBuffer(); + PQExpBuffer aclitem = createPQExpBuffer(); + + /* Standard ACL as of v19 is {owner=r/owner} */ + appendPQExpBufferChar(aclarray, '{'); + quoteAclUserName(aclitem, tblinfo[i].rolname); + appendPQExpBufferStr(aclitem, "=r/"); + quoteAclUserName(aclitem, tblinfo[i].rolname); + appendPGArray(aclarray, aclitem->data); + appendPQExpBufferChar(aclarray, '}'); + + tblinfo[i].dacl.acldefault = pstrdup(aclarray->data); + + destroyPQExpBuffer(aclarray); + destroyPQExpBuffer(aclitem); + } + else + tblinfo[i].dacl.acldefault = pg_strdup(PQgetvalue(res, i, i_acldefault)); + /* other fields were zeroed above */ /* Attachments: [text/plain] acldefault-propgraph-v1.patch (5.0K, ../../[email protected]/2-acldefault-propgraph-v1.patch) download | inline diff: From: Noah Misch <[email protected]> Fix pg_dump ACL minimization for PROPERTY GRAPH. Adding a GRANT caused pg_dump to emit a useless REVOKE + GRANT of owner privileges, as seen in a dump of the regression database: REVOKE ALL ON PROPERTY GRAPH graph_rls_schema.cabinet FROM nm; GRANT ALL ON PROPERTY GRAPH graph_rls_schema.cabinet TO nm; GRANT ALL ON PROPERTY GRAPH graph_rls_schema.cabinet TO PUBLIC; For normal dumps, this has no functional consequences. For --no-owner restores, the extra statements may fail or locate unrelated users of the destination cluster. The problem was pg_dump assuming NULL relacl implies acldefault('r'), the default for TABLE. Fix by teaching acldefault() to retrieve the PROPERTY GRAPH default ACL. So pg_dump can still dump from 19beta1, use acldefault('g') for v20+ only. For v19, use a hard-coded snapshot of the v19 default. information_schema.pg_property_graph_privileges also misused acldefault('r'), but its "c.prtype IN ('SELECT')" predicate compensated for it. Switch to the new acldefault('g') for clarity. Bump catversion since a new view won't work with old binaries. Back-patch to v19, which introduced PROPERTY GRAPH. Reviewed-by: FIXME Discussion: https://postgr.es/m/FIXME Backpatch-through: 19 diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql index 4f0e249..624d538 100644 --- a/src/backend/catalog/information_schema.sql +++ b/src/backend/catalog/information_schema.sql @@ -3328,7 +3328,7 @@ CREATE VIEW pg_property_graph_privileges AS THEN 'YES' ELSE 'NO' END AS yes_or_no) AS is_grantable FROM ( - SELECT oid, relname, relnamespace, relkind, relowner, (aclexplode(coalesce(relacl, acldefault('r', relowner)))).* FROM pg_class + SELECT oid, relname, relnamespace, relkind, relowner, (aclexplode(coalesce(relacl, acldefault('g', relowner)))).* FROM pg_class ) AS c (oid, relname, relnamespace, relkind, relowner, grantor, grantee, prtype, grantable), pg_namespace nc, pg_authid u_grantor, diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index 01caa12..e2547d7 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -956,6 +956,9 @@ acldefault_sql(PG_FUNCTION_ARGS) case 'c': objtype = OBJECT_COLUMN; break; + case 'g': + objtype = OBJECT_PROPGRAPH; + break; case 'r': objtype = OBJECT_TABLE; break; diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index c56437d..41b9531 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -7358,8 +7358,17 @@ getTables(Archive *fout, int *numTables) "c.relhastriggers, c.relpersistence, " "c.reloftype, " "c.relacl, " - "acldefault(CASE WHEN c.relkind = " CppAsString2(RELKIND_SEQUENCE) - " THEN 's'::\"char\" ELSE 'r'::\"char\" END, c.relowner) AS acldefault, " + "acldefault(CASE" + " WHEN c.relkind = " CppAsString2(RELKIND_PROPGRAPH)); + /* 19beta1 didn't support acldefault('g'), so we'll fix that below */ + appendPQExpBufferStr(query, + fout->remoteVersion >= 200000 ? + " THEN 'g'::\"char\"" : + " THEN NULL"); + appendPQExpBufferStr(query, + " WHEN c.relkind = " CppAsString2(RELKIND_SEQUENCE) + " THEN 's'::\"char\"" + " ELSE 'r'::\"char\" END, c.relowner) AS acldefault, " "CASE WHEN c.relkind = " CppAsString2(RELKIND_FOREIGN_TABLE) " THEN " "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) " "ELSE 0 END AS foreignserver, " @@ -7579,7 +7588,7 @@ getTables(Archive *fout, int *numTables) tblinfo[i].dobj.namespace = findNamespace(atooid(PQgetvalue(res, i, i_relnamespace))); tblinfo[i].dacl.acl = pg_strdup(PQgetvalue(res, i, i_relacl)); - tblinfo[i].dacl.acldefault = pg_strdup(PQgetvalue(res, i, i_acldefault)); + /* acldefault computed below */ tblinfo[i].dacl.privtype = 0; tblinfo[i].dacl.initprivs = NULL; tblinfo[i].relkind = *(PQgetvalue(res, i, i_relkind)); @@ -7631,6 +7640,28 @@ getTables(Archive *fout, int *numTables) tblinfo[i].is_identity_sequence = (strcmp(PQgetvalue(res, i, i_is_identity_sequence), "t") == 0); tblinfo[i].ispartition = (strcmp(PQgetvalue(res, i, i_ispartition), "t") == 0); + if (tblinfo[i].relkind == RELKIND_PROPGRAPH && + !(fout->remoteVersion >= 200000)) + { + PQExpBuffer aclarray = createPQExpBuffer(); + PQExpBuffer aclitem = createPQExpBuffer(); + + /* Standard ACL as of v19 is {owner=r/owner} */ + appendPQExpBufferChar(aclarray, '{'); + quoteAclUserName(aclitem, tblinfo[i].rolname); + appendPQExpBufferStr(aclitem, "=r/"); + quoteAclUserName(aclitem, tblinfo[i].rolname); + appendPGArray(aclarray, aclitem->data); + appendPQExpBufferChar(aclarray, '}'); + + tblinfo[i].dacl.acldefault = pstrdup(aclarray->data); + + destroyPQExpBuffer(aclarray); + destroyPQExpBuffer(aclitem); + } + else + tblinfo[i].dacl.acldefault = pg_strdup(PQgetvalue(res, i, i_acldefault)); + /* other fields were zeroed above */ /* ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: PROPERTY GRAPH pg_dump ACL minimization @ 2026-06-30 03:11 Tom Lane <[email protected]> parent: Noah Misch <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Tom Lane @ 2026-06-30 03:11 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: pgsql-hackers Noah Misch <[email protected]> writes: > Most of the patch bulk (modest as it is) exists to keep support for dumping > from beta1. I'm not sure whether it was worth bothering. Breaking dump from > a beta is without precedent known to me, so I just erred on the side of not > breaking it. If we were to decide pg_dump could drop support for betas, I'd > be fine with that. > This entails a catversion bump on the v19 branch. Those points are not unrelated. If you bump catversion then beta testers must use pg_upgrade to get from beta1 to beta2, so you should not drop support for dumping from beta1. I could agree with dropping that support after beta2, though. That'd imply having to update via beta2 to beta3 or later, but I doubt those hardy enough to test beta1 would have a problem with that. > If the master branch > already has a post-branch catversion bump by then, the two catversions should > remain distinct. I'll use yyyymmdd1 for v19 and yyyymmdd2 for master. Check. (I didn't read the patch, just responded to your commentary.) regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: PROPERTY GRAPH pg_dump ACL minimization @ 2026-07-01 16:21 Ashutosh Bapat <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Ashutosh Bapat @ 2026-07-01 16:21 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; pgsql-hackers Thanks Noah for the patch and the report. On Tue, Jun 30, 2026 at 8:41 AM Tom Lane <[email protected]> wrote: > > Noah Misch <[email protected]> writes: > > Most of the patch bulk (modest as it is) exists to keep support for dumping > > from beta1. I'm not sure whether it was worth bothering. Breaking dump from > > a beta is without precedent known to me, so I just erred on the side of not > > breaking it. If we were to decide pg_dump could drop support for betas, I'd > > be fine with that. > > > This entails a catversion bump on the v19 branch. > > Those points are not unrelated. If you bump catversion then beta > testers must use pg_upgrade to get from beta1 to beta2, so you should > not drop support for dumping from beta1. > > I could agree with dropping that support after beta2, though. That'd > imply having to update via beta2 to beta3 or later, but I doubt those > hardy enough to test beta1 would have a problem with that. > Should there be two commits one which will be reverted post beta2 and one which will stay after beta2? > > If the master branch > > already has a post-branch catversion bump by then, the two catversions should > > remain distinct. I'll use yyyymmdd1 for v19 and yyyymmdd2 for master. > > Check. > > (I didn't read the patch, just responded to your commentary.) I reviewed the patch. - tblinfo[i].dacl.acldefault = pg_strdup(PQgetvalue(res, i, i_acldefault)); + /* acldefault computed below */ Rather than spacially separating the acldefault computation, can we just write a function to compute the acldefault for a given relkind and owner, and call that function here? Did you consider writing a test for the same? Other than that the patch looks good. I wondered whether we are missing special handling for PROPGRAPH at other places. I looked at other places where we handle OBJECT_SEQUENCE separately in acl related files. I discovered following missing cases 1. ExecGrant_Relation: I think we should clip the extra privileges with a warning when GRANT ... TABLE syntax is used to grant privileges on a property graph, just like sequences. To me it looks like we should prohibit GRANT ... TABLE on property graph altogether. But haven't done so to keep it in sync with sequences. The backward compatibility comment, "For backward compatibility, just ... " should not be applicable in case of property graph since we can introduce whatever behaviour we expect from GRANT ... TABLE right from the first release which introduced property graph. But I am not sure if that's the only backward compatibility we are talking about here. Those commits go more than a few decades back and commit message itself doesn't help me much. Maybe someone with a better historical perspective may help. I have also added a test scenario for a non-property graph privilege to be added using GRANT ... TABLE syntax. The second change in this function seems necessary but without it, I couldn't find a visible bug. Mostly it's masked because the privileges available on a table are a superset of privileges available on a property graph. Now that the function handles property graph separately, its prologue needs to change. I think we should mention property graph along with sequences as done in the patch OR just mention "all types of objects in pg_class." 2. pg_class_aclmask_ext(): this seems to be another omission. Probably innocuous since we will test only SELECT privileges on a property graph and ignore other default table privileges. 3. pg_default_acl: doesn't need any update since property graph is not an object listed in the types of objects for which the user is allowed to specify default permissions through pg_default_acl. All other places which handle OBJECT_SEQUENCE also handle OBJECT_PROPGRAPH. I also notice that information_schema.pg_propgraph_privileges shows only privileges of type "SELECT" so we wouldn't be able to notice a privilege type other than SELECT being granted on a property graph through information_schema. But a similar filtering exists in the view information_schema.table_privileges. So it looks intentional and I didn't touch it. -- Best Wishes, Ashutosh Bapat Attachments: [text/x-patch] missing_propgraph_acl.patch (4.0K, ../../CAExHW5tz+ufmneS+gYTNrYLbi=bF=D2Xp5rvr12cuLYoNn80kg@mail.gmail.com/2-missing_propgraph_acl.patch) download | inline diff: commit 17e1f6bbb289d1c4d89a8f65462032bf37d34061 Author: Ashutosh Bapat <[email protected]> Date: Wed Jul 1 21:34:48 2026 +0530 Using GRANT ... TABLE command on a property graph Property graph can only have SELECT privilege. The privileges can be granted using GRANT ... PROPERTY GRAPH command or GRANT ... TABLE command. When privileges are granted using GRANT ... TABLE command, similar to the case of sequences, ignore the privileges other than SELECT privilege with a warning. While at it also handle missing RELKIND_PROPGRAPH in pg_class_aclmask_ext() and ExecGrant_Relation() functions. Author: Ashutosh Bapat <[email protected]> diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 007ede997c5..05c8f440626 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -1784,7 +1784,8 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid, const char *relname, } /* - * This processes both sequences and non-sequences. + * This processes sequences, property graphs and other relations types in + * pg_class catalog. */ static void ExecGrant_Relation(InternalGrant *istmt) @@ -1891,6 +1892,27 @@ ExecGrant_Relation(InternalGrant *istmt) this_privileges &= (AclMode) ACL_ALL_RIGHTS_SEQUENCE; } } + else if (pg_class_tuple->relkind == RELKIND_PROPGRAPH) + { + /* + * For backward compatibility, just throw a warning for + * invalid property graph permissions when using the non property graph + * GRANT syntax. + */ + if (this_privileges & ~((AclMode) ACL_ALL_RIGHTS_PROPGRAPH)) + { + /* + * Mention the object name because the user needs to know + * which operations succeeded. This is required because + * WARNING allows the command to continue. + */ + ereport(WARNING, + (errcode(ERRCODE_INVALID_GRANT_OPERATION), + errmsg("property graph \"%s\" only supports SELECT privileges", + NameStr(pg_class_tuple->relname)))); + this_privileges &= (AclMode) ACL_ALL_RIGHTS_PROPGRAPH; + } + } else { if (this_privileges & ~((AclMode) ACL_ALL_RIGHTS_RELATION)) @@ -1996,6 +2018,9 @@ ExecGrant_Relation(InternalGrant *istmt) case RELKIND_SEQUENCE: objtype = OBJECT_SEQUENCE; break; + case RELKIND_PROPGRAPH: + objtype = OBJECT_PROPGRAPH; + break; default: objtype = OBJECT_TABLE; break; @@ -3375,6 +3400,9 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask, case RELKIND_SEQUENCE: acl = acldefault(OBJECT_SEQUENCE, ownerId); break; + case RELKIND_PROPGRAPH: + acl = acldefault(OBJECT_PROPGRAPH, ownerId); + break; default: acl = acldefault(OBJECT_TABLE, ownerId); break; diff --git a/src/test/regress/expected/create_property_graph.out b/src/test/regress/expected/create_property_graph.out index 4eb7f487770..0376c82e10c 100644 --- a/src/test/regress/expected/create_property_graph.out +++ b/src/test/regress/expected/create_property_graph.out @@ -237,6 +237,8 @@ SET ROLE regress_graph_user1; GRANT SELECT ON PROPERTY GRAPH g1 TO regress_graph_user2; GRANT UPDATE ON PROPERTY GRAPH g1 TO regress_graph_user2; -- fail ERROR: invalid privilege type UPDATE for property graph +GRANT UPDATE ON TABLE g1 TO regress_graph_user2; -- warning +WARNING: property graph "g1" only supports SELECT privileges RESET ROLE; -- collation CREATE TABLE tc1 (a int, b text); diff --git a/src/test/regress/sql/create_property_graph.sql b/src/test/regress/sql/create_property_graph.sql index 8a9baa674dc..5f5a9cfdad3 100644 --- a/src/test/regress/sql/create_property_graph.sql +++ b/src/test/regress/sql/create_property_graph.sql @@ -185,6 +185,7 @@ ALTER PROPERTY GRAPH g1 OWNER TO regress_graph_user1; SET ROLE regress_graph_user1; GRANT SELECT ON PROPERTY GRAPH g1 TO regress_graph_user2; GRANT UPDATE ON PROPERTY GRAPH g1 TO regress_graph_user2; -- fail +GRANT UPDATE ON TABLE g1 TO regress_graph_user2; -- warning RESET ROLE; -- collation ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: PROPERTY GRAPH pg_dump ACL minimization @ 2026-07-03 10:39 Ashutosh Bapat <[email protected]> parent: Ashutosh Bapat <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Ashutosh Bapat @ 2026-07-03 10:39 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; pgsql-hackers On Wed, Jul 1, 2026 at 9:51 PM Ashutosh Bapat <[email protected]> wrote: > > I wondered whether we are missing special handling for PROPGRAPH at > other places. I looked at other places where we handle OBJECT_SEQUENCE > separately in acl related files. I discovered following missing cases > > 1. ExecGrant_Relation: I think we should clip the extra privileges > with a warning when GRANT ... TABLE syntax is used to grant privileges > on a property graph, just like sequences. To me it looks like we > should prohibit GRANT ... TABLE on property graph altogether. But > haven't done so to keep it in sync with sequences. The backward > compatibility comment, "For backward compatibility, just ... " should > not be applicable in case of property graph since we can introduce > whatever behaviour we expect from GRANT ... TABLE right from the first > release which introduced property graph. But I am not sure if that's > the only backward compatibility we are talking about here. Those > commits go more than a few decades back and commit message itself > doesn't help me much. Maybe someone with a better historical > perspective may help. I have also added a test scenario for a > non-property graph privilege to be added using GRANT ... TABLE syntax. > Since property graphs share the namespace with regular tables, I think GRANT ... TABLE should be supported on property graphs, but restrict it to only the privileges applicable to property graphs. Done that way in the attached patch. > The second change in this function seems necessary but without it, I > couldn't find a visible bug. Mostly it's masked because the privileges > available on a table are a superset of privileges available on a > property graph. > This change is needed so that we can provide a correct error message. Here's a revised patch set. 0010 is your patch without any changes 0011 is my changes described above. -- Best Wishes, Ashutosh Bapat Attachments: [text/x-patch] v20260703-0010-Fix-pg_dump-ACL-minimization-for-PROPERTY-.patch (5.4K, ../../CAExHW5va66cEd6uXvj16F0dGRfikWXf7jzRULrqV3OQ99EPsXg@mail.gmail.com/2-v20260703-0010-Fix-pg_dump-ACL-minimization-for-PROPERTY-.patch) download | inline diff: From 43a27e7a278d7b8a23bb7967c60aec5a226a3ebb Mon Sep 17 00:00:00 2001 From: Noah Misch <[email protected]> Date: Wed, 1 Jul 2026 11:59:33 +0530 Subject: [PATCH v20260703 10/13] Fix pg_dump ACL minimization for PROPERTY GRAPH. Adding a GRANT caused pg_dump to emit a useless REVOKE + GRANT of owner privileges, as seen in a dump of the regression database: REVOKE ALL ON PROPERTY GRAPH graph_rls_schema.cabinet FROM nm; GRANT ALL ON PROPERTY GRAPH graph_rls_schema.cabinet TO nm; GRANT ALL ON PROPERTY GRAPH graph_rls_schema.cabinet TO PUBLIC; For normal dumps, this has no functional consequences. For --no-owner restores, the extra statements may fail or locate unrelated users of the destination cluster. The problem was pg_dump assuming NULL relacl implies acldefault('r'), the default for TABLE. Fix by teaching acldefault() to retrieve the PROPERTY GRAPH default ACL. So pg_dump can still dump from 19beta1, use acldefault('g') for v20+ only. For v19, use a hard-coded snapshot of the v19 default. information_schema.pg_property_graph_privileges also misused acldefault('r'), but its "c.prtype IN ('SELECT')" predicate compensated for it. Switch to the new acldefault('g') for clarity. Bump catversion since a new view won't work with old binaries. Back-patch to v19, which introduced PROPERTY GRAPH. Reviewed-by: FIXME Discussion: https://postgr.es/m/FIXME Backpatch-through: 19 --- src/backend/catalog/information_schema.sql | 2 +- src/backend/utils/adt/acl.c | 3 ++ src/bin/pg_dump/pg_dump.c | 37 ++++++++++++++++++++-- 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql index 4f0e2492937..624d538a5c0 100644 --- a/src/backend/catalog/information_schema.sql +++ b/src/backend/catalog/information_schema.sql @@ -3328,7 +3328,7 @@ CREATE VIEW pg_property_graph_privileges AS THEN 'YES' ELSE 'NO' END AS yes_or_no) AS is_grantable FROM ( - SELECT oid, relname, relnamespace, relkind, relowner, (aclexplode(coalesce(relacl, acldefault('r', relowner)))).* FROM pg_class + SELECT oid, relname, relnamespace, relkind, relowner, (aclexplode(coalesce(relacl, acldefault('g', relowner)))).* FROM pg_class ) AS c (oid, relname, relnamespace, relkind, relowner, grantor, grantee, prtype, grantable), pg_namespace nc, pg_authid u_grantor, diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index 01caa12eca7..e2547d719ed 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -956,6 +956,9 @@ acldefault_sql(PG_FUNCTION_ARGS) case 'c': objtype = OBJECT_COLUMN; break; + case 'g': + objtype = OBJECT_PROPGRAPH; + break; case 'r': objtype = OBJECT_TABLE; break; diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index f67daf85911..4d660d14b4c 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -7231,8 +7231,17 @@ getTables(Archive *fout, int *numTables) "c.relhastriggers, c.relpersistence, " "c.reloftype, " "c.relacl, " - "acldefault(CASE WHEN c.relkind = " CppAsString2(RELKIND_SEQUENCE) - " THEN 's'::\"char\" ELSE 'r'::\"char\" END, c.relowner) AS acldefault, " + "acldefault(CASE" + " WHEN c.relkind = " CppAsString2(RELKIND_PROPGRAPH)); + /* 19beta1 didn't support acldefault('g'), so we'll fix that below */ + appendPQExpBufferStr(query, + fout->remoteVersion >= 200000 ? + " THEN 'g'::\"char\"" : + " THEN NULL"); + appendPQExpBufferStr(query, + " WHEN c.relkind = " CppAsString2(RELKIND_SEQUENCE) + " THEN 's'::\"char\"" + " ELSE 'r'::\"char\" END, c.relowner) AS acldefault, " "CASE WHEN c.relkind = " CppAsString2(RELKIND_FOREIGN_TABLE) " THEN " "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) " "ELSE 0 END AS foreignserver, " @@ -7418,7 +7427,7 @@ getTables(Archive *fout, int *numTables) tblinfo[i].dobj.namespace = findNamespace(atooid(PQgetvalue(res, i, i_relnamespace))); tblinfo[i].dacl.acl = pg_strdup(PQgetvalue(res, i, i_relacl)); - tblinfo[i].dacl.acldefault = pg_strdup(PQgetvalue(res, i, i_acldefault)); + /* acldefault computed below */ tblinfo[i].dacl.privtype = 0; tblinfo[i].dacl.initprivs = NULL; tblinfo[i].relkind = *(PQgetvalue(res, i, i_relkind)); @@ -7470,6 +7479,28 @@ getTables(Archive *fout, int *numTables) tblinfo[i].is_identity_sequence = (strcmp(PQgetvalue(res, i, i_is_identity_sequence), "t") == 0); tblinfo[i].ispartition = (strcmp(PQgetvalue(res, i, i_ispartition), "t") == 0); + if (tblinfo[i].relkind == RELKIND_PROPGRAPH && + !(fout->remoteVersion >= 200000)) + { + PQExpBuffer aclarray = createPQExpBuffer(); + PQExpBuffer aclitem = createPQExpBuffer(); + + /* Standard ACL as of v19 is {owner=r/owner} */ + appendPQExpBufferChar(aclarray, '{'); + quoteAclUserName(aclitem, tblinfo[i].rolname); + appendPQExpBufferStr(aclitem, "=r/"); + quoteAclUserName(aclitem, tblinfo[i].rolname); + appendPGArray(aclarray, aclitem->data); + appendPQExpBufferChar(aclarray, '}'); + + tblinfo[i].dacl.acldefault = pstrdup(aclarray->data); + + destroyPQExpBuffer(aclarray); + destroyPQExpBuffer(aclitem); + } + else + tblinfo[i].dacl.acldefault = pg_strdup(PQgetvalue(res, i, i_acldefault)); + /* other fields were zeroed above */ /* -- 2.34.1 [text/x-patch] v20260703-0011-Fix-GRANT-.-TABLE-on-a-property-graph.patch (4.5K, ../../CAExHW5va66cEd6uXvj16F0dGRfikWXf7jzRULrqV3OQ99EPsXg@mail.gmail.com/3-v20260703-0011-Fix-GRANT-.-TABLE-on-a-property-graph.patch) download | inline diff: From b21ed425500c8bd492ed7acd615715c9d32ae3a7 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <[email protected]> Date: Wed, 1 Jul 2026 21:34:48 +0530 Subject: [PATCH v20260703 11/13] Fix GRANT ... TABLE on a property graph A property graph only supports the SELECT privileges. Privileges can be granted using either GRANT ... PROPERTY GRAPH or GRANT ... TABLE. When the GRANT ... TABLE form is used, ignore any privileges other than SELECT and emit a warning, as we already do for sequences. While here, add the missing RELKIND_PROPGRAPH cases in pg_class_aclmask_ext() and in the object-type switch in ExecGrant_Relation() so that the default ACL and the objtype passed to restrict_and_check_grant() are correct. Author: Ashutosh Bapat <[email protected]> Discussion: https://www.postgresql.org/message-id/[email protected] --- src/backend/catalog/aclchk.c | 31 ++++++++++++++++++- .../expected/create_property_graph.out | 2 ++ .../regress/sql/create_property_graph.sql | 1 + 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 140cd1302f5..54b59892c43 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -1784,7 +1784,7 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid, const char *relname, } /* - * This processes both sequences and non-sequences. + * This processes all pg_class entries including sequences and property graphs. */ static void ExecGrant_Relation(InternalGrant *istmt) @@ -1891,6 +1891,29 @@ ExecGrant_Relation(InternalGrant *istmt) this_privileges &= (AclMode) ACL_ALL_RIGHTS_SEQUENCE; } } + else if (pg_class_tuple->relkind == RELKIND_PROPGRAPH) + { + /* + * Per the SQL standard, property graphs share the namespace + * with tables, so GRANT ... ON TABLE also works on a property + * graph. A property graph only supports SELECT, so mask off + * any other requested privileges and emit a warning, as we do + * for sequences. + */ + if (this_privileges & ~((AclMode) ACL_ALL_RIGHTS_PROPGRAPH)) + { + /* + * Mention the object name because the user needs to know + * which operations succeeded. This is required because + * WARNING allows the command to continue. + */ + ereport(WARNING, + (errcode(ERRCODE_INVALID_GRANT_OPERATION), + errmsg("property graph \"%s\" only supports SELECT privileges", + NameStr(pg_class_tuple->relname)))); + this_privileges &= (AclMode) ACL_ALL_RIGHTS_PROPGRAPH; + } + } else { if (this_privileges & ~((AclMode) ACL_ALL_RIGHTS_RELATION)) @@ -1996,6 +2019,9 @@ ExecGrant_Relation(InternalGrant *istmt) case RELKIND_SEQUENCE: objtype = OBJECT_SEQUENCE; break; + case RELKIND_PROPGRAPH: + objtype = OBJECT_PROPGRAPH; + break; default: objtype = OBJECT_TABLE; break; @@ -3389,6 +3415,9 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask, case RELKIND_SEQUENCE: acl = acldefault(OBJECT_SEQUENCE, ownerId); break; + case RELKIND_PROPGRAPH: + acl = acldefault(OBJECT_PROPGRAPH, ownerId); + break; default: acl = acldefault(OBJECT_TABLE, ownerId); break; diff --git a/src/test/regress/expected/create_property_graph.out b/src/test/regress/expected/create_property_graph.out index e9f91f77ee9..713966e8a9e 100644 --- a/src/test/regress/expected/create_property_graph.out +++ b/src/test/regress/expected/create_property_graph.out @@ -251,6 +251,8 @@ SET ROLE regress_graph_user1; GRANT SELECT ON PROPERTY GRAPH g1 TO regress_graph_user2; GRANT UPDATE ON PROPERTY GRAPH g1 TO regress_graph_user2; -- fail ERROR: invalid privilege type UPDATE for property graph +GRANT UPDATE ON TABLE g1 TO regress_graph_user2; -- warning +WARNING: property graph "g1" only supports SELECT privileges RESET ROLE; -- collation CREATE TABLE tc1 (a int, b text); diff --git a/src/test/regress/sql/create_property_graph.sql b/src/test/regress/sql/create_property_graph.sql index 08341e13a50..5325d22206e 100644 --- a/src/test/regress/sql/create_property_graph.sql +++ b/src/test/regress/sql/create_property_graph.sql @@ -197,6 +197,7 @@ ALTER PROPERTY GRAPH g1 OWNER TO regress_graph_user1; SET ROLE regress_graph_user1; GRANT SELECT ON PROPERTY GRAPH g1 TO regress_graph_user2; GRANT UPDATE ON PROPERTY GRAPH g1 TO regress_graph_user2; -- fail +GRANT UPDATE ON TABLE g1 TO regress_graph_user2; -- warning RESET ROLE; -- collation -- 2.34.1 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: PROPERTY GRAPH pg_dump ACL minimization @ 2026-07-04 14:46 Noah Misch <[email protected]> parent: Ashutosh Bapat <[email protected]> 0 siblings, 3 replies; 11+ messages in thread From: Noah Misch @ 2026-07-04 14:46 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers On Wed, Jul 01, 2026 at 09:51:13PM +0530, Ashutosh Bapat wrote: > On Tue, Jun 30, 2026 at 8:41 AM Tom Lane <[email protected]> wrote: > > Noah Misch <[email protected]> writes: > > > Most of the patch bulk (modest as it is) exists to keep support for dumping > > > from beta1. I'm not sure whether it was worth bothering. Breaking dump from > > > a beta is without precedent known to me, so I just erred on the side of not > > > breaking it. If we were to decide pg_dump could drop support for betas, I'd > > > be fine with that. > > > > > This entails a catversion bump on the v19 branch. > > > > Those points are not unrelated. If you bump catversion then beta > > testers must use pg_upgrade to get from beta1 to beta2, so you should > > not drop support for dumping from beta1. > > > > I could agree with dropping that support after beta2, though. That'd > > imply having to update via beta2 to beta3 or later, but I doubt those > > hardy enough to test beta1 would have a problem with that. I agree dropping 19beta1 dump support after beta2 would be reasonable, and I hadn't thought of doing so. However, ... > Should there be two commits one which will be reverted post beta2 and > one which will stay after beta2? ... since the sole benefit would be removing ~25 lines, I don't think a followup patch is worth it. I wouldn't object if someone wants to remove that code later. > > > If the master branch > > > already has a post-branch catversion bump by then, the two catversions should > > > remain distinct. I'll use yyyymmdd1 for v19 and yyyymmdd2 for master. > > > > Check. > > > > (I didn't read the patch, just responded to your commentary.) > > I reviewed the patch. > > - tblinfo[i].dacl.acldefault = pg_strdup(PQgetvalue(res, i, i_acldefault)); > + /* acldefault computed below */ > > Rather than spacially separating the acldefault computation, can we > just write a function to compute the acldefault for a given relkind > and owner, and call that function here? Such a function would have just one caller, and that would even further spatially separate the code lines defining the computation. Either way is fine, but I'm not inclined to change to that. > Did you consider writing a test for the same? Somewhat. Long-term, I think a more general test would have a place. > Other than that the patch looks good. Thanks for reviewing. > I wondered whether we are missing special handling for PROPGRAPH at > other places. I looked at other places where we handle OBJECT_SEQUENCE > separately in acl related files. I discovered following missing cases This probably calls for its own thread; feel free to fork the thread for any followup on that. On Fri, Jul 03, 2026 at 04:09:20PM +0530, Ashutosh Bapat wrote: > On Wed, Jul 1, 2026 at 9:51 PM Ashutosh Bapat > <[email protected]> wrote: > > > > I wondered whether we are missing special handling for PROPGRAPH at > > other places. I looked at other places where we handle OBJECT_SEQUENCE > > separately in acl related files. I discovered following missing cases > > > > 1. ExecGrant_Relation: I think we should clip the extra privileges > > with a warning when GRANT ... TABLE syntax is used to grant privileges > > on a property graph, just like sequences. To me it looks like we > > should prohibit GRANT ... TABLE on property graph altogether. But > > haven't done so to keep it in sync with sequences. The backward > > compatibility comment, "For backward compatibility, just ... " should > > not be applicable in case of property graph since we can introduce > > whatever behaviour we expect from GRANT ... TABLE right from the first > > release which introduced property graph. But I am not sure if that's > > the only backward compatibility we are talking about here. Those > > commits go more than a few decades back and commit message itself > > doesn't help me much. Maybe someone with a better historical > > perspective may help. I have also added a test scenario for a > > non-property graph privilege to be added using GRANT ... TABLE syntax. > > Since property graphs share the namespace with regular tables, I think > GRANT ... TABLE should be supported on property graphs, but restrict > it to only the privileges applicable to property graphs. I don't have a strong opinion on that, but I likely would have chosen to block GRANT TABLE on a propgraph. The backward compatibility argument written for SEQUENCE likely means that someone noticed GRANT TABLE worked on sequences and decided both that it was a mistake and that reversing the mistake would be a cure worse than the disease. There was a rebuttable presumption that when we add a new grantable object with its own GRANT subtype, older GRANT subtypes should reject that object. > > 2. pg_class_aclmask_ext(): this seems to be another omission. Probably > > innocuous since we will test only SELECT privileges on a property > > graph and ignore other default table privileges. Makes sense. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: PROPERTY GRAPH pg_dump ACL minimization @ 2026-07-04 15:51 Tom Lane <[email protected]> parent: Noah Misch <[email protected]> 2 siblings, 1 reply; 11+ messages in thread From: Tom Lane @ 2026-07-04 15:51 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; pgsql-hackers Noah Misch <[email protected]> writes: > On Wed, Jul 01, 2026 at 09:51:13PM +0530, Ashutosh Bapat wrote: >> Since property graphs share the namespace with regular tables, I think >> GRANT ... TABLE should be supported on property graphs, but restrict >> it to only the privileges applicable to property graphs. > I don't have a strong opinion on that, but I likely would have chosen to block > GRANT TABLE on a propgraph. The backward compatibility argument written for > SEQUENCE likely means that someone noticed GRANT TABLE worked on sequences and > decided both that it was a mistake and that reversing the mistake would be a > cure worse than the disease. My recollection is that there was an intentional policy change. Originally the idea was "why make people be careful about which kind of relation they're granting on?". The arguments made against that included: * It's exposing an implementation detail, namely that sequences and tables live in the same catalog. Admittedly that detail is also exposed by the fact that they can't share a name. * It doesn't comport very well with the fact that the sets of possible privileges are different. * It doesn't obey the SQL standard (I think, maybe someone will correct me). But you are entirely right that we felt that disallowing what used to work was worse than leaving it alone. We need not duplicate that mistake for a new kind of relation, and should not. regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: PROPERTY GRAPH pg_dump ACL minimization @ 2026-07-06 11:54 Ashutosh Bapat <[email protected]> parent: Noah Misch <[email protected]> 2 siblings, 0 replies; 11+ messages in thread From: Ashutosh Bapat @ 2026-07-06 11:54 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers On Sat, Jul 4, 2026 at 8:16 PM Noah Misch <[email protected]> wrote: > > On Wed, Jul 01, 2026 at 09:51:13PM +0530, Ashutosh Bapat wrote: > > On Tue, Jun 30, 2026 at 8:41 AM Tom Lane <[email protected]> wrote: > > > Noah Misch <[email protected]> writes: > > > > Most of the patch bulk (modest as it is) exists to keep support for dumping > > > > from beta1. I'm not sure whether it was worth bothering. Breaking dump from > > > > a beta is without precedent known to me, so I just erred on the side of not > > > > breaking it. If we were to decide pg_dump could drop support for betas, I'd > > > > be fine with that. > > > > > > > This entails a catversion bump on the v19 branch. > > > > > > Those points are not unrelated. If you bump catversion then beta > > > testers must use pg_upgrade to get from beta1 to beta2, so you should > > > not drop support for dumping from beta1. > > > > > > I could agree with dropping that support after beta2, though. That'd > > > imply having to update via beta2 to beta3 or later, but I doubt those > > > hardy enough to test beta1 would have a problem with that. > > I agree dropping 19beta1 dump support after beta2 would be reasonable, and I > hadn't thought of doing so. However, ... > > > Should there be two commits one which will be reverted post beta2 and > > one which will stay after beta2? > > ... since the sole benefit would be removing ~25 lines, I don't think a > followup patch is worth it. I wouldn't object if someone wants to remove that > code later. > As long as the code that needs to be removed after beta2 is clear, I am fine. If somebody other than the ones involved here want to remove the code, we should make it easy for them to do so. > > > I wondered whether we are missing special handling for PROPGRAPH at > > other places. I looked at other places where we handle OBJECT_SEQUENCE > > separately in acl related files. I discovered following missing cases > > This probably calls for its own thread; feel free to fork the thread for any > followup on that. > Will do that soon. -- Best Wishes, Ashutosh Bapat ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: PROPERTY GRAPH pg_dump ACL minimization @ 2026-07-06 11:55 Ashutosh Bapat <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Ashutosh Bapat @ 2026-07-06 11:55 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; pgsql-hackers On Sat, Jul 4, 2026 at 9:21 PM Tom Lane <[email protected]> wrote: > > Noah Misch <[email protected]> writes: > > On Wed, Jul 01, 2026 at 09:51:13PM +0530, Ashutosh Bapat wrote: > >> Since property graphs share the namespace with regular tables, I think > >> GRANT ... TABLE should be supported on property graphs, but restrict > >> it to only the privileges applicable to property graphs. > > > I don't have a strong opinion on that, but I likely would have chosen to block > > GRANT TABLE on a propgraph. The backward compatibility argument written for > > SEQUENCE likely means that someone noticed GRANT TABLE worked on sequences and > > decided both that it was a mistake and that reversing the mistake would be a > > cure worse than the disease. > > My recollection is that there was an intentional policy change. > Originally the idea was "why make people be careful about which > kind of relation they're granting on?". The arguments made > against that included: > > * It's exposing an implementation detail, namely that sequences > and tables live in the same catalog. Admittedly that detail is > also exposed by the fact that they can't share a name. > > * It doesn't comport very well with the fact that the sets of > possible privileges are different. > > * It doesn't obey the SQL standard (I think, maybe someone will > correct me). > > But you are entirely right that we felt that disallowing what used > to work was worse than leaving it alone. We need not duplicate that > mistake for a new kind of relation, and should not. Thanks for the clarification. I will change the patch to prohibit using GRANT ... TABLE on a property graph. -- Best Wishes, Ashutosh Bapat ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: PROPERTY GRAPH pg_dump ACL minimization @ 2026-07-06 16:40 Robert Haas <[email protected]> parent: Noah Misch <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Robert Haas @ 2026-07-06 16:40 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: pgsql-hackers On Mon, Jun 29, 2026 at 10:33 PM Noah Misch <[email protected]> wrote: > pg_dump doesn't do its normal ACL minimization for the new PROPERTY GRAPH > feature. Patch attached. See log message for details. > > Most of the patch bulk (modest as it is) exists to keep support for dumping > from beta1. I'm not sure whether it was worth bothering. Breaking dump from > a beta is without precedent known to me, so I just erred on the side of not > breaking it. If we were to decide pg_dump could drop support for betas, I'd > be fine with that. Thanks for catching this, Noah. While reviewing this patch, I wondered whether quoteAclUserName() exactly matches what the server would do. It has this dire warning: /* This test had better match what putid() does */ It doesn't any more, quite, because quoteAclUserName() uses this test: !isalnum((unsigned char) *src) && *src != '_' And putid uses this test: !is_safe_acl_char(*src, false) The difference between the two is that is_safe_acl_char(c) will return false for any bytes where IS_HIGHBIT_SET(c) returns true. So consider: CREATE ROLE álvaro; CREATE PROPERTY GRAPH herrera; ALTER PROPERTY GRAPH herrera OWNER TO álvaro; GRANT SELECT ON PROPERTY GRAPH herrera TO PUBLIC; In a UTF8 database, everything is fine. But with encoding = LATIN1 and lc_ctype = en_US.ISO8859-1, pg_class.relacl is display with quotes around álvaro, and the string pg_dump synthesizes lacks them. This doesn't seem to cause a functional problem, because the ACLs are not directly compared -- they get parsed first, and that undoes the quoting. It seems a tad fragile, maybe, but perhaps not worth worrying about. It's also not really the fault of this patch anyway, but it was the only thing I found while looking through this, so I figured I would mention it. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: PROPERTY GRAPH pg_dump ACL minimization @ 2026-07-06 20:44 Noah Misch <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Noah Misch @ 2026-07-06 20:44 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On Mon, Jul 06, 2026 at 12:40:05PM -0400, Robert Haas wrote: > On Mon, Jun 29, 2026 at 10:33 PM Noah Misch <[email protected]> wrote: > > pg_dump doesn't do its normal ACL minimization for the new PROPERTY GRAPH > > feature. Patch attached. See log message for details. > While reviewing this patch, I wondered whether quoteAclUserName() > exactly matches what the server would do. It has this dire warning: > > /* This test had better match what putid() does */ > > It doesn't any more, quite, because quoteAclUserName() uses this test: > > !isalnum((unsigned char) *src) && *src != '_' > > And putid uses this test: > > !is_safe_acl_char(*src, false) > > The difference between the two is that is_safe_acl_char(c) will return > false for any bytes where IS_HIGHBIT_SET(c) returns true. So consider: > > CREATE ROLE álvaro; > CREATE PROPERTY GRAPH herrera; > ALTER PROPERTY GRAPH herrera OWNER TO álvaro; > GRANT SELECT ON PROPERTY GRAPH herrera TO PUBLIC; > > In a UTF8 database, everything is fine. But with encoding = LATIN1 and > lc_ctype = en_US.ISO8859-1, pg_class.relacl is display with quotes > around álvaro, and the string pg_dump synthesizes lacks them. This > doesn't seem to cause a functional problem, because the ACLs are not > directly compared -- they get parsed first, and that undoes the > quoting. It seems a tad fragile, maybe, but perhaps not worth worrying > about. It's also not really the fault of this patch anyway, but it was > the only thing I found while looking through this, so I figured I > would mention it. Thanks for reviewing and for identifying that. I think this boils down to the comment being too dire given current use cases. It's important for both quoteAclUserName() and putid() to quote metacharacters, but it's okay if one of them quotes non-metacharacters that the other doesn't quote. I guess the behavior mismatch you've identified also makes this comment wrong: /* * Test whether an identifier char can be left unquoted in ACLs. * * Formerly, we used isalnum() even on non-ASCII characters, resulting in * unportable behavior. To ensure dump compatibility with old versions, * we now treat high-bit-set characters as always requiring quoting during * putid(), but getid() will always accept them without quotes. */ static inline bool is_safe_acl_char(unsigned char c, bool is_getid) It's not just "compatibility with old versions" as long as pg_dump calls to quoteAclUserName() are still churning out affected values. Best if we make quoteAclUserName() match putid(), so the comments can just be correct and nobody needs to wonder about the mismatch. ^ permalink raw reply [nested|flat] 11+ messages in thread
* GRANT ... TABLE for property graph @ 2026-07-08 11:37 Ashutosh Bapat <[email protected]> parent: Noah Misch <[email protected]> 2 siblings, 0 replies; 11+ messages in thread From: Ashutosh Bapat @ 2026-07-08 11:37 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers Starting a new thread from [1] as suggested. > > On Fri, Jul 03, 2026 at 04:09:20PM +0530, Ashutosh Bapat wrote: > > On Wed, Jul 1, 2026 at 9:51 PM Ashutosh Bapat > > <[email protected]> wrote: > > > > > > I wondered whether we are missing special handling for PROPGRAPH at > > > other places. I looked at other places where we handle OBJECT_SEQUENCE > > > separately in acl related files. I discovered following missing cases > > > > > > 1. ExecGrant_Relation: I think we should clip the extra privileges > > > with a warning when GRANT ... TABLE syntax is used to grant privileges > > > on a property graph, just like sequences. To me it looks like we > > > should prohibit GRANT ... TABLE on property graph altogether. But > > > haven't done so to keep it in sync with sequences. The backward > > > compatibility comment, "For backward compatibility, just ... " should > > > not be applicable in case of property graph since we can introduce > > > whatever behaviour we expect from GRANT ... TABLE right from the first > > > release which introduced property graph. But I am not sure if that's > > > the only backward compatibility we are talking about here. Those > > > commits go more than a few decades back and commit message itself > > > doesn't help me much. Maybe someone with a better historical > > > perspective may help. I have also added a test scenario for a > > > non-property graph privilege to be added using GRANT ... TABLE syntax. > > > > Since property graphs share the namespace with regular tables, I think > > GRANT ... TABLE should be supported on property graphs, but restrict > > it to only the privileges applicable to property graphs. > > I don't have a strong opinion on that, but I likely would have chosen to block > GRANT TABLE on a propgraph. The backward compatibility argument written for > SEQUENCE likely means that someone noticed GRANT TABLE worked on sequences and > decided both that it was a mistake and that reversing the mistake would be a > cure worse than the disease. There was a rebuttable presumption that when we > add a new grantable object with its own GRANT subtype, older GRANT subtypes > should reject that object. > > > > 2. pg_class_aclmask_ext(): this seems to be another omission. Probably > > > innocuous since we will test only SELECT privileges on a property > > > graph and ignore other default table privileges. > > Makes sense. Modified the patch to disallow GRANT ... TABLE on property graph. The patch uses errmsg("\"%s\" is a property graph", NameStr(pg_class_tuple->relname)) inline with other prohibited cases. Additionally it gives errhint("Use GRANT ... ON PROPERTY GRAPH instead.")), which may be obvious from the error message, but I thought clarity is better than brevity. There are some cases where we report ""%s" is not a sequence". Inline with that we could use errmsg("\"%s\" is not a table", NameStr(pg_class_tuple->relname)) with errhint("Use GRANT ... ON PROPERTY GRAPH instead.")). I prefer the first one, but I am ok with the second option as well. -- Best Wishes, Ashutosh Bapat Attachments: [text/x-patch] v20260708-0005-Prohibit-GRANT-.-ON-TABLE-on-a-property-gr.patch (9.3K, ../../CAExHW5shdPcFzrP=PcDUO=TiU80UHX+PDZ_w8AoV2=YCFe5=aw@mail.gmail.com/2-v20260708-0005-Prohibit-GRANT-.-ON-TABLE-on-a-property-gr.patch) download | inline diff: From 1739f6fca786a53ed451b35ad7593e4250555b41 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <[email protected]> Date: Wed, 1 Jul 2026 21:34:48 +0530 Subject: [PATCH v20260708 5/5] Prohibit GRANT ... ON TABLE on a property graph We allowed GRANT ... ON TABLE on sequences for backward compatibility. We don't need to consider backward compatibility in case of property graphs since we will be prohibiting its usage on property graph from the very release which introduced property graphs. Change regression tests that used GRANT ... ON (TABLE) on property graphs to use GRANT ... ON PROPERTY GRAPH instead. While here, add the missing RELKIND_PROPGRAPH cases in pg_class_aclmask_ext() and in the object-type switch in ExecGrant_Relation() so that the default ACL and the objtype passed to restrict_and_check_grant() are correct. Author: Ashutosh Bapat <[email protected]> Reviewed by: Noah Misch <[email protected]> Reviewed by: Tom Lane <[email protected]> Discussion: https://www.postgresql.org/message-id/[email protected] --- src/backend/catalog/aclchk.c | 20 ++++++++++++++++++- .../expected/create_property_graph.out | 3 +++ src/test/regress/expected/graph_table_rls.out | 6 +++--- src/test/regress/expected/privileges.out | 4 ++-- .../regress/sql/create_property_graph.sql | 1 + src/test/regress/sql/graph_table_rls.sql | 6 +++--- src/test/regress/sql/privileges.sql | 4 ++-- 7 files changed, 33 insertions(+), 11 deletions(-) diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 140cd1302f5..c7ab9cc5524 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -1784,7 +1784,7 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid, const char *relname, } /* - * This processes both sequences and non-sequences. + * This processes all pg_class entries including sequences and property graphs. */ static void ExecGrant_Relation(InternalGrant *istmt) @@ -1891,6 +1891,18 @@ ExecGrant_Relation(InternalGrant *istmt) this_privileges &= (AclMode) ACL_ALL_RIGHTS_SEQUENCE; } } + else if (pg_class_tuple->relkind == RELKIND_PROPGRAPH) + { + /* + * Do not allow GRANT ... TABLE on property graph. We allowed + * it on sequences for backward compatibility but there is no + * reason to continue that further. + */ + ereport(ERROR, + errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("\"%s\" is a property graph", NameStr(pg_class_tuple->relname)), + errhint("Use GRANT ... ON PROPERTY GRAPH instead.")); + } else { if (this_privileges & ~((AclMode) ACL_ALL_RIGHTS_RELATION)) @@ -1996,6 +2008,9 @@ ExecGrant_Relation(InternalGrant *istmt) case RELKIND_SEQUENCE: objtype = OBJECT_SEQUENCE; break; + case RELKIND_PROPGRAPH: + objtype = OBJECT_PROPGRAPH; + break; default: objtype = OBJECT_TABLE; break; @@ -3389,6 +3404,9 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask, case RELKIND_SEQUENCE: acl = acldefault(OBJECT_SEQUENCE, ownerId); break; + case RELKIND_PROPGRAPH: + acl = acldefault(OBJECT_PROPGRAPH, ownerId); + break; default: acl = acldefault(OBJECT_TABLE, ownerId); break; diff --git a/src/test/regress/expected/create_property_graph.out b/src/test/regress/expected/create_property_graph.out index 3efcce046cf..da148db0653 100644 --- a/src/test/regress/expected/create_property_graph.out +++ b/src/test/regress/expected/create_property_graph.out @@ -250,6 +250,9 @@ SET ROLE regress_graph_user1; GRANT SELECT ON PROPERTY GRAPH g1 TO regress_graph_user2; GRANT UPDATE ON PROPERTY GRAPH g1 TO regress_graph_user2; -- fail ERROR: invalid privilege type UPDATE for property graph +GRANT UPDATE ON TABLE g1 TO regress_graph_user2; -- warning +ERROR: "g1" is a property graph +HINT: Use GRANT ... ON PROPERTY GRAPH instead. RESET ROLE; -- collation CREATE TABLE tc1 (a int, b text); diff --git a/src/test/regress/expected/graph_table_rls.out b/src/test/regress/expected/graph_table_rls.out index 0e719c7ebd7..230ae4cdb01 100644 --- a/src/test/regress/expected/graph_table_rls.out +++ b/src/test/regress/expected/graph_table_rls.out @@ -74,7 +74,7 @@ CREATE PROPERTY GRAPH cabinet EDGE TABLES (accessed KEY (aid) SOURCE KEY (uid) REFERENCES users (uid) DESTINATION KEY (did) REFERENCES document (did)); -GRANT SELECT ON cabinet TO public; +GRANT SELECT ON PROPERTY GRAPH cabinet TO public; -- -- Basic RLS tests -- @@ -261,7 +261,7 @@ CREATE PROPERTY GRAPH cabinet EDGE TABLES (accessed KEY (aid) SOURCE KEY (uid) REFERENCES users (uid) DESTINATION KEY (did) REFERENCES document (did)); -GRANT SELECT ON cabinet TO public; +GRANT SELECT ON PROPERTY GRAPH cabinet TO public; SET row_security TO ON; -- viewpoint from regress_graph_rls_bob SET SESSION AUTHORIZATION regress_graph_rls_bob; @@ -456,7 +456,7 @@ CREATE PROPERTY GRAPH cabinet EDGE TABLES (accessed KEY (aid) SOURCE KEY (uid) REFERENCES users (uid) DESTINATION KEY (did) REFERENCES document (did)); -GRANT SELECT ON cabinet TO public; +GRANT SELECT ON PROPERTY GRAPH cabinet TO public; SET row_security TO ON; -- viewpoint from regress_graph_rls_bob SET SESSION AUTHORIZATION regress_graph_rls_bob; diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index f6cc1a1029c..fd18549e84e 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -3206,7 +3206,7 @@ select * from graph_table (ptg1 match (is atest5) COLUMNS (1 as value)) limit 0; ------- (0 rows) -grant select on ptg1 to regress_priv_user2; +grant select on property graph ptg1 to regress_priv_user2; set session role regress_priv_user2; select * from graph_table (ptg1 match (is atest1) COLUMNS (1 as value)) limit 0; -- ok value @@ -3232,7 +3232,7 @@ select * from graph_table (ptg1 match (v is lttc) COLUMNS (v.lttck)) limit 0; -- ------- (0 rows) -grant select on ptg1 to regress_priv_user4; +grant select on property graph ptg1 to regress_priv_user4; set session role regress_priv_user4; select * from graph_table (ptg1 match (a is atest5) COLUMNS (a.four)) limit 0; -- ok four diff --git a/src/test/regress/sql/create_property_graph.sql b/src/test/regress/sql/create_property_graph.sql index f98a9187451..d05062678a2 100644 --- a/src/test/regress/sql/create_property_graph.sql +++ b/src/test/regress/sql/create_property_graph.sql @@ -196,6 +196,7 @@ ALTER PROPERTY GRAPH g1 OWNER TO regress_graph_user1; SET ROLE regress_graph_user1; GRANT SELECT ON PROPERTY GRAPH g1 TO regress_graph_user2; GRANT UPDATE ON PROPERTY GRAPH g1 TO regress_graph_user2; -- fail +GRANT UPDATE ON TABLE g1 TO regress_graph_user2; -- warning RESET ROLE; -- collation diff --git a/src/test/regress/sql/graph_table_rls.sql b/src/test/regress/sql/graph_table_rls.sql index 5837eac402e..5c79ade68d4 100644 --- a/src/test/regress/sql/graph_table_rls.sql +++ b/src/test/regress/sql/graph_table_rls.sql @@ -87,7 +87,7 @@ CREATE PROPERTY GRAPH cabinet EDGE TABLES (accessed KEY (aid) SOURCE KEY (uid) REFERENCES users (uid) DESTINATION KEY (did) REFERENCES document (did)); -GRANT SELECT ON cabinet TO public; +GRANT SELECT ON PROPERTY GRAPH cabinet TO public; -- -- Basic RLS tests @@ -198,7 +198,7 @@ CREATE PROPERTY GRAPH cabinet EDGE TABLES (accessed KEY (aid) SOURCE KEY (uid) REFERENCES users (uid) DESTINATION KEY (did) REFERENCES document (did)); -GRANT SELECT ON cabinet TO public; +GRANT SELECT ON PROPERTY GRAPH cabinet TO public; SET row_security TO ON; @@ -267,7 +267,7 @@ CREATE PROPERTY GRAPH cabinet EDGE TABLES (accessed KEY (aid) SOURCE KEY (uid) REFERENCES users (uid) DESTINATION KEY (did) REFERENCES document (did)); -GRANT SELECT ON cabinet TO public; +GRANT SELECT ON PROPERTY GRAPH cabinet TO public; SET row_security TO ON; -- viewpoint from regress_graph_rls_bob diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index 6cd9bb840ff..6e0686da131 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -1890,7 +1890,7 @@ create property graph ptg1 label ltv properties (col1 as ltvk)); -- select privileges on property graph as well as table select * from graph_table (ptg1 match (is atest5) COLUMNS (1 as value)) limit 0; -- ok -grant select on ptg1 to regress_priv_user2; +grant select on property graph ptg1 to regress_priv_user2; set session role regress_priv_user2; select * from graph_table (ptg1 match (is atest1) COLUMNS (1 as value)) limit 0; -- ok -- select privileges on property graph but not table @@ -1904,7 +1904,7 @@ select * from graph_table (ptg1 match (is atest5) COLUMNS (1 as value)) limit 0; -- column privileges set session role regress_priv_user1; select * from graph_table (ptg1 match (v is lttc) COLUMNS (v.lttck)) limit 0; -- ok -grant select on ptg1 to regress_priv_user4; +grant select on property graph ptg1 to regress_priv_user4; set session role regress_priv_user4; select * from graph_table (ptg1 match (a is atest5) COLUMNS (a.four)) limit 0; -- ok select * from graph_table (ptg1 match (v is lttc) COLUMNS (v.lttck)) limit 0; -- fail -- 2.34.1 ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2026-07-08 11:37 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-06-30 02:33 PROPERTY GRAPH pg_dump ACL minimization Noah Misch <[email protected]> 2026-06-30 03:11 ` Tom Lane <[email protected]> 2026-07-01 16:21 ` Ashutosh Bapat <[email protected]> 2026-07-03 10:39 ` Ashutosh Bapat <[email protected]> 2026-07-04 14:46 ` Noah Misch <[email protected]> 2026-07-04 15:51 ` Tom Lane <[email protected]> 2026-07-06 11:55 ` Ashutosh Bapat <[email protected]> 2026-07-06 11:54 ` Ashutosh Bapat <[email protected]> 2026-07-08 11:37 ` GRANT ... TABLE for property graph Ashutosh Bapat <[email protected]> 2026-07-06 16:40 ` Robert Haas <[email protected]> 2026-07-06 20:44 ` Noah Misch <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox