Thread: refactor ExecGrant_*() functions
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. [0]: https://www.postgresql.org/message-id/95c30f96-4060-2f48-98b5-a4392d3b6066@enterprisedb.com [1]: https://www.postgresql.org/message-id/flat/20330.1652105397@antos
Attachment
Hi, On 2022-12-02 08:30:55 +0100, Peter Eisentraut wrote: > From 200879e5edfc1ce93b7af3cbfafc1f618626cbe9 Mon Sep 17 00:00:00 2001 > From: Peter Eisentraut <peter@eisentraut.org> > 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
Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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-a4392d3b6066@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); /*
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 <peter@eisentraut.org> >> 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.
Attachment
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.
Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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
On 12.12.22 10:44, Antonin Houska wrote: > Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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.
Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > On 12.12.22 10:44, Antonin Houska wrote: > > Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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/