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
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
