Horton

Thread

Commits

  1. Refactor ExecGrant_*() functions

  1. refactor ExecGrant_*() functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-12-02T07:30:55Z

    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

  2. Re: refactor ExecGrant_*() functions

    Andres Freund <andres@anarazel.de> — 2022-12-02T17:28:29Z

    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

  3. Re: refactor ExecGrant_*() functions

    Antonin Houska <ah@cybertec.at> — 2022-12-06T08:41:58Z

    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

  4. Re: refactor ExecGrant_*() functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-12-08T09:26:35Z

    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.

  5. Re: refactor ExecGrant_*() functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-12-08T09:27:25Z

    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.

  6. Re: refactor ExecGrant_*() functions

    Antonin Houska <ah@cybertec.at> — 2022-12-12T09:44:39Z

    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

  7. Re: refactor ExecGrant_*() functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-12-13T06:54:07Z

    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.

  8. Re: refactor ExecGrant_*() functions

    Antonin Houska <ah@cybertec.at> — 2022-12-13T15:03:57Z

    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/