Horton

Thread

Commits

  1. Refactor aclcheck functions

  2. Refactor ownercheck functions

  3. Unify drop-by-OID functions

  1. refactor ownercheck and aclcheck functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-10-14T07:39:26Z

    These patches take the dozens of mostly-duplicate pg_foo_ownercheck() 
and pg_foo_aclcheck() functions and replace (most of) them by common 
functions that are driven by the ObjectProperty table.  All the required 
information is already in that table.

This is similar to the consolidation of the drop-by-OID functions that 
we did a while ago (b1d32d3e3230f00b5baba08f75b4f665c7d6dac6).

  2. Re: refactor ownercheck and aclcheck functions

    Corey Huinker <corey.huinker@gmail.com> — 2022-10-19T23:24:25Z

    On Fri, Oct 14, 2022 at 3:39 AM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:

> These patches take the dozens of mostly-duplicate pg_foo_ownercheck()
> and pg_foo_aclcheck() functions and replace (most of) them by common
> functions that are driven by the ObjectProperty table.  All the required
> information is already in that table.
>
> This is similar to the consolidation of the drop-by-OID functions that
> we did a while ago (b1d32d3e3230f00b5baba08f75b4f665c7d6dac6).


Nice reduction in footprint!

I'd be inclined to remove the highly used ones as well. That way the
codebase would have more examples of object_ownercheck() for readers to
see. Seeing the existence of pg_FOO_ownercheck implies that a
pg_BAR_ownercheck might exist, and if BAR is missing they might be inclined
to re-add it.

If we do keep them, would it make sense to go the extra step and turn the
remaining six "regular" into static inline functions or even #define-s?

  3. Re: refactor ownercheck and aclcheck functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-10-21T19:17:47Z

    On 20.10.22 01:24, Corey Huinker wrote:
> I'd be inclined to remove the highly used ones as well. That way the 
> codebase would have more examples of object_ownercheck() for readers to 
> see. Seeing the existence of pg_FOO_ownercheck implies that a 
> pg_BAR_ownercheck might exist, and if BAR is missing they might be 
> inclined to re-add it.

We do have several ownercheck and aclcheck functions that can't be 
refactored into this framework right now, so we do have to keep some 
special-purpose functions around anyway.  I'm afraid converting all the 
callers would blow up this patch quite a bit, but it could be done as a 
follow-up patch.

> If we do keep them, would it make sense to go the extra step and turn 
> the remaining six "regular" into static inline functions or even #define-s?

That could make sense.

  4. Re: refactor ownercheck and aclcheck functions

    Antonin Houska <ah@cybertec.at> — 2022-11-07T13:19:42Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

> These patches take the dozens of mostly-duplicate pg_foo_ownercheck() and
> pg_foo_aclcheck() functions and replace (most of) them by common functions
> that are driven by the ObjectProperty table.  All the required information is
> already in that table.
> 
> This is similar to the consolidation of the drop-by-OID functions that we did
> a while ago (b1d32d3e3230f00b5baba08f75b4f665c7d6dac6).

I've reviewed this patch, as it's related to my patch [1] (In particular, it
reduces the size of my patch a little bit). I like the idea to reduce the
amount of (almost) copy & pasted code. I haven't found any problem in your
patch that would be worth mentioning, except that the 0001 part does not apply
to the current master branch.

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

  5. Re: refactor ownercheck and aclcheck functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-11-08T11:16:41Z

    On 21.10.22 21:17, Peter Eisentraut wrote:
> On 20.10.22 01:24, Corey Huinker wrote:
>> I'd be inclined to remove the highly used ones as well. That way the 
>> codebase would have more examples of object_ownercheck() for readers 
>> to see. Seeing the existence of pg_FOO_ownercheck implies that a 
>> pg_BAR_ownercheck might exist, and if BAR is missing they might be 
>> inclined to re-add it.
> 
> We do have several ownercheck and aclcheck functions that can't be 
> refactored into this framework right now, so we do have to keep some 
> special-purpose functions around anyway.  I'm afraid converting all the 
> callers would blow up this patch quite a bit, but it could be done as a 
> follow-up patch.
> 
>> If we do keep them, would it make sense to go the extra step and turn 
>> the remaining six "regular" into static inline functions or even 
>> #define-s?
> 
> That could make sense.

After considering this again, I decided to brute-force this and get rid 
of all the trivial wrapper functions and also several of the special 
cases.  That way, there is less confusion at the call sites about why 
this or that style is used in a particular case.  Also, it now makes 
sure you can't accidentally use the generic functions when a particular 
one should be used.

  6. Re: refactor ownercheck and aclcheck functions

    Corey Huinker <corey.huinker@gmail.com> — 2022-11-09T18:12:36Z

    >
> After considering this again, I decided to brute-force this and get rid
> of all the trivial wrapper functions and also several of the special
> cases.  That way, there is less confusion at the call sites about why
> this or that style is used in a particular case.  Also, it now makes
> sure you can't accidentally use the generic functions when a particular
> one should be used.
>

+1

However, the aclcheck patch isn't applying for me now. That patch modifies
37 files, so it's hard to say just which commit conflicts.

  7. Re: refactor ownercheck and aclcheck functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-11-13T09:26:43Z

    On 09.11.22 19:12, Corey Huinker wrote:
>     After considering this again, I decided to brute-force this and get rid
>     of all the trivial wrapper functions and also several of the special
>     cases.  That way, there is less confusion at the call sites about why
>     this or that style is used in a particular case.  Also, it now makes
>     sure you can't accidentally use the generic functions when a particular
>     one should be used.
> 
> 
> +1

committed