Re: refactor ExecGrant_*() functions - Mailing list pgsql-hackers

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);
 
         /*