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 <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]:
/messages/by-id/95c30f96-4060-2f48-98b5-a4392d3b6066@enterprisedb.com
[1]: /messages/by-id/20330.1652105397@antos

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

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.

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]https://commitfest.postgresql.org/41/3641/ soon.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

[1]: https://commitfest.postgresql.org/41/3641/