Re: refactor ExecGrant_*() functions
Peter Eisentraut wrote: > On 12.12.22 10:44, Antonin Houska wrote: > > Peter Eisentraut wrote: > > > >> On 06.12.22 09:41, Antonin Houska wrote: > >>> Attached are my proposals for improvements. One is to avoid memory leak, > >>> the > >>> other tries to improve readability a little bit. > >> > >> I added the readability improvement to my v2 patch. The pfree() calls > >> aren't > >> necessary AFAICT. > > It's something to consider, but since this is a refactoring patch and the old > code didn't do it either, I think it's out of scope. Well, the reason I brought this topic up is that the old code didn't even palloc() those arrays. (Because the were located in the stack.) > > I see that memory contexts exist and that the amount of memory freed is not > > huge, but my style is to free the memory explicitly if it's allocated in a > > loop. > > v2 looks good to me. > > Committed, thanks. ok, I'll post rebased "USAGE privilege on PUBLICATION" patch [1] soon. -- Antonin Houska Web: https://www.cybertec-postgresql.com [1] https://commitfest.postgresql.org/41/3641/
Re: refactor ExecGrant_*() functions
On 12.12.22 10:44, Antonin Houska wrote: Peter Eisentraut wrote: On 06.12.22 09:41, Antonin Houska wrote: Attached are my proposals for improvements. One is to avoid memory leak, the other tries to improve readability a little bit. I added the readability improvement to my v2 patch. The pfree() calls aren't necessary AFAICT. It's something to consider, but since this is a refactoring patch and the old code didn't do it either, I think it's out of scope. I see that memory contexts exist and that the amount of memory freed is not huge, but my style is to free the memory explicitly if it's allocated in a loop. v2 looks good to me. Committed, thanks.
Re: refactor ExecGrant_*() functions
Peter Eisentraut wrote: > On 06.12.22 09:41, Antonin Houska wrote: > > Attached are my proposals for improvements. One is to avoid memory leak, the > > other tries to improve readability a little bit. > > I added the readability improvement to my v2 patch. The pfree() calls aren't > necessary AFAICT. I see that memory contexts exist and that the amount of memory freed is not huge, but my style is to free the memory explicitly if it's allocated in a loop. v2 looks good to me. -- Antonin Houska Web: https://www.cybertec-postgresql.com
Re: refactor ExecGrant_*() functions
On 06.12.22 09:41, Antonin Houska wrote: Attached are my proposals for improvements. One is to avoid memory leak, the other tries to improve readability a little bit. I added the readability improvement to my v2 patch. The pfree() calls aren't necessary AFAICT.
Re: refactor ExecGrant_*() functions
On 02.12.22 18:28, Andres Freund wrote: Hi, On 2022-12-02 08:30:55 +0100, Peter Eisentraut wrote: From 200879e5edfc1ce93b7af3cbfafc1f618626cbe9 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Fri, 2 Dec 2022 08:16:53 +0100 Subject: [PATCH] Refactor ExecGrant_*() functions Instead of half a dozen of mostly-duplicate ExecGrant_Foo() functions, write one common function ExecGrant_generic() that can handle most of them. I'd name it ExecGrant_common() or such instead - ExecGrant_generic() sounds like it will handle arbitrary things, which it doesn't. And, as you mention, we could implement e.g. ExecGrant_Language() as using ExecGrant_common() + additional checks. Done Perhaps it'd be useful to add a callback to ExecGrant_generic() that can perform additional checks, so that e.g. ExecGrant_Language() can easily be implemented using ExecGrant_generic()? Done. This allows getting rid of ExecGrant_Language and ExecGrant_Type in addition to the previous patch. From 7c4c3bd5d1cb776506b157c01c7fd975d700e8d9 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Thu, 8 Dec 2022 10:23:07 +0100 Subject: [PATCH v2] Refactor ExecGrant_*() functions Instead of half a dozen of mostly-duplicate ExecGrant_Foo() functions, write one common function ExecGrant_generic() that can handle most of them. We already have all the information we need, such as which system catalog corresponds to which catalog table and which column is the ACL column. Discussion: https://www.postgresql.org/message-id/flat/22c7e802-4e7d-8d87-8b71-cba95e6f4bcf%40enterprisedb.com --- src/backend/catalog/aclchk.c | 948 +++ 1 file changed, 73 insertions(+), 875 deletions(-) diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 8d84a7b056..43968d569c 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -106,15 +106,11 @@ bool binary_upgrade_record_init_privs = false; static void ExecGrantStmt_oids(InternalGrant *istmt); static void ExecGrant_Relation(InternalGrant *istmt); -static void ExecGrant_Database(InternalGrant *istmt); -static void ExecGrant_Fdw(InternalGrant *istmt); -static void ExecGrant_ForeignServer(InternalGrant *istmt); -static void ExecGrant_Function(InternalGrant *istmt); -static void ExecGrant_Language(InternalGrant *istmt); +static void ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs, +void (*object_check) (InternalGrant *istmt, HeapTuple tuple)); +static void ExecGrant_Language_check(InternalGrant *istmt, HeapTuple tuple); static void ExecGrant_Largeobject(InternalGrant *istmt); -static void ExecGrant_Namespace(InternalGrant *istmt); -static void ExecGrant_Tablespace(InternalGrant *istmt); -static void ExecGrant_Type(InternalGrant *istmt); +static void ExecGrant_Type_check(InternalGrant *istmt, HeapTuple tuple); static void ExecGrant_Parameter(InternalGrant *istmt); static void SetDefaultACLsInSchemas(InternalDefaultACL *iacls, List *nspnames); @@ -602,34 +598,34 @@ ExecGrantStmt_oids(InternalGrant *istmt) ExecGrant_Relation(istmt); break; case OBJECT_DATABASE: - ExecGrant_Database(istmt); + ExecGrant_common(istmt, DatabaseRelationId, ACL_ALL_RIGHTS_DATABASE, NULL); break; case OBJECT_DOMAIN: case OBJECT_TYPE: - ExecGrant_Type(istmt); + ExecGrant_common(istmt, TypeRelationId, ACL_ALL_RIGHTS_TYPE, ExecGrant_Type_check); break; case OBJECT_FDW: - ExecGrant_Fdw(istmt); + ExecGrant_common(istmt, ForeignDataWrapperRelationId, ACL_ALL_RIGHTS_FDW, NULL); break; case OBJECT_FOREIGN_SERVER: - ExecGrant_ForeignServer(istmt); + ExecGrant_common(istmt, ForeignServerRelationId, ACL_ALL_RIGHTS_FOREIGN_SERVER, NULL); break; case OBJECT_FUNCTION: case OBJECT_PROCEDURE: case OBJECT_ROUTINE: - ExecGrant_Function(istmt); + ExecGrant_common(istmt, ProcedureRelationId, ACL_ALL_RIGHTS_FUNCTION, NULL); break; case OBJECT_LANGUAGE: - ExecGrant_Language(istmt); + ExecGrant_common(istmt, LanguageRelationId, ACL_ALL_RIGHTS_LANGUAGE, ExecGrant_Language_check); break; case OBJECT_LARGEOBJECT: ExecGrant_Largeobject(istmt); break; case OBJECT_SCHEMA: - ExecGrant_Namespace(istmt); + ExecGrant_common(istmt, NamespaceRelationId, ACL_ALL_RIGHT
Re: refactor ExecGrant_*() functions
Peter Eisentraut wrote:
> Continuing the ideas in [0], this patch refactors the ExecGrant_Foo()
> functions and replaces many of them by a common function that is driven by the
> ObjectProperty table.
>
> It would be nice to do more here, for example ExecGrant_Language(), which has
> additional non-generalizable checks, but I think this is already a good start.
> For example, the work being discussed on privileges on publications [1] would
> be able to take good advantage of this.
Right, I mostly copy and pasted the code when writing
ExecGrant_Publication(). I agree that your refactoring is very useful.
Attached are my proposals for improvements. One is to avoid memory leak, the
other tries to improve readability a little bit.
> [0]:
> https://www.postgresql.org/message-id/95c30f96-4060-2f48-98b5-a4392d3b6...@enterprisedb.com
> [1]: https://www.postgresql.org/message-id/flat/20330.1652105397@antos
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index d3121a469f..ac4490c0b8 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -2228,6 +2234,9 @@ ExecGrant_generic(InternalGrant *istmt, Oid classid, AclMode default_privs)
newtuple = heap_modify_tuple(tuple, RelationGetDescr(relation), values,
nulls, replaces);
+ pfree(values);
+ pfree(nulls);
+ pfree(replaces);
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index d3121a469f..ac4490c0b8 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -2144,6 +2144,7 @@ ExecGrant_generic(InternalGrant *istmt, Oid classid, AclMode default_privs)
{
Oid objectid = lfirst_oid(cell);
Datum aclDatum;
+ Datum nameDatum;
bool isNull;
AclMode avail_goptions;
AclMode this_privileges;
@@ -2197,6 +2198,11 @@ ExecGrant_generic(InternalGrant *istmt, Oid classid, AclMode default_privs)
old_acl, ownerId,
&grantorId, &avail_goptions);
+ nameDatum = SysCacheGetAttr(cacheid, tuple,
+ get_object_attnum_name(classid),
+ &isNull);
+ Assert(!isNull);
+
/*
* Restrict the privileges to what we can actually grant, and emit the
* standards-mandated warning and error messages.
@@ -2205,7 +2211,7 @@ ExecGrant_generic(InternalGrant *istmt, Oid classid, AclMode default_privs)
restrict_and_check_grant(istmt->is_grant, avail_goptions,
istmt->all_privs, istmt->privileges,
objectid, grantorId, get_object_type(classid, objectid),
- NameStr(*DatumGetName(SysCacheGetAttr(cacheid, tuple, get_object_attnum_name(classid), &isNull))),
+ NameStr(*DatumGetName(nameDatum)),
0, NULL);
/*
Re: refactor ExecGrant_*() functions
Hi, On 2022-12-02 08:30:55 +0100, Peter Eisentraut wrote: > From 200879e5edfc1ce93b7af3cbfafc1f618626cbe9 Mon Sep 17 00:00:00 2001 > From: Peter Eisentraut > Date: Fri, 2 Dec 2022 08:16:53 +0100 > Subject: [PATCH] Refactor ExecGrant_*() functions > > Instead of half a dozen of mostly-duplicate ExecGrant_Foo() functions, > write one common function ExecGrant_generic() that can handle most of > them. I'd name it ExecGrant_common() or such instead - ExecGrant_generic() sounds like it will handle arbitrary things, which it doesn't. And, as you mention, we could implement e.g. ExecGrant_Language() as using ExecGrant_common() + additional checks. Perhaps it'd be useful to add a callback to ExecGrant_generic() that can perform additional checks, so that e.g. ExecGrant_Language() can easily be implemented using ExecGrant_generic()? > 1 file changed, 34 insertions(+), 628 deletions(-) Very neat. It seems wrong that most (all?) ExecGrant_* functions have the foreach(cell, istmt->objects) loop. But that's a lot easier to address once the code has been deduplicated radically. Greetings, Andres Freund
