Thread

Commits

Refactor recordExtObjInitPriv()
- 2a1d7071c4ed 16.0 landed
Remove AggregateRelationId from recordExtObjInitPriv()
- 13b345df6438 16.0 landed

Refactor recordExtObjInitPriv()

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-12-27T08:56:10Z

Another aclchk.c refactoring patch, similar to [0] and [1].

Refactor recordExtObjInitPriv():  Instead of half a dozen of 
mostly-duplicate conditional branches, write one common one that can 
handle most catalogs.  We already have all the information we need, such 
as which system catalog corresponds to which catalog table and which 
column is the ACL column.


[0]: 
https://www.postgresql.org/message-id/95c30f96-4060-2f48-98b5-a4392d3b6066@enterprisedb.com
[1]: 
https://www.postgresql.org/message-id/flat/22c7e802-4e7d-8d87-8b71-cba95e6f4bcf%40enterprisedb.com

Re: Refactor recordExtObjInitPriv()

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-12T00:04:37Z

On Tue, Dec 27, 2022 at 09:56:10AM +0100, Peter Eisentraut wrote:
> Refactor recordExtObjInitPriv():  Instead of half a dozen of
> mostly-duplicate conditional branches, write one common one that can handle
> most catalogs.  We already have all the information we need, such as which
> system catalog corresponds to which catalog table and which column is the
> ACL column.

This seems reasonable.

> +	/* This will error on unsupported classoid. */
> +	else if (get_object_attnum_acl(classoid))

nitpick: I would suggest explicitly checking that it isn't
InvalidAttrNumber instead of relying on it always being 0.

> -			 classoid == AggregateRelationId ||

I noticed that AggregateRelationId isn't listed in the ObjectProperty
array, so I think recordExtObjInitPriv() will begin erroring for that
classoid instead of ignoring it like we do today.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Refactor recordExtObjInitPriv()

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-12T17:15:14Z

On 12.01.23 01:04, Nathan Bossart wrote:
>> -			 classoid == AggregateRelationId ||
> I noticed that AggregateRelationId isn't listed in the ObjectProperty
> array, so I think recordExtObjInitPriv() will begin erroring for that
> classoid instead of ignoring it like we do today.

Hmm, we do have some extensions in contrib that add aggregates (citext, 
intagg).  I suspect that the aggregate function is actually registered 
into the extension via its pg_proc entry, so this wouldn't actually 
matter.  But maybe the commenting should be clearer?

Re: Refactor recordExtObjInitPriv()

Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-12T17:20:50Z

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 12.01.23 01:04, Nathan Bossart wrote:
> -			 classoid == AggregateRelationId ||
>> I noticed that AggregateRelationId isn't listed in the ObjectProperty
>> array, so I think recordExtObjInitPriv() will begin erroring for that
>> classoid instead of ignoring it like we do today.

> Hmm, we do have some extensions in contrib that add aggregates (citext, 
> intagg).  I suspect that the aggregate function is actually registered 
> into the extension via its pg_proc entry, so this wouldn't actually 
> matter.  But maybe the commenting should be clearer?

Yeah, I don't believe that AggregateRelationId is used in object
addresses; we just refer to pg_proc for any kind of function including
aggregates.  Note that there is no "oid" column in pg_aggregate.

			regards, tom lane

Re: Refactor recordExtObjInitPriv()

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-12T17:40:49Z

On Thu, Jan 12, 2023 at 12:20:50PM -0500, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> On 12.01.23 01:04, Nathan Bossart wrote:
>> -			 classoid == AggregateRelationId ||
>>> I noticed that AggregateRelationId isn't listed in the ObjectProperty
>>> array, so I think recordExtObjInitPriv() will begin erroring for that
>>> classoid instead of ignoring it like we do today.
> 
>> Hmm, we do have some extensions in contrib that add aggregates (citext, 
>> intagg).  I suspect that the aggregate function is actually registered 
>> into the extension via its pg_proc entry, so this wouldn't actually 
>> matter.  But maybe the commenting should be clearer?
> 
> Yeah, I don't believe that AggregateRelationId is used in object
> addresses; we just refer to pg_proc for any kind of function including
> aggregates.  Note that there is no "oid" column in pg_aggregate.

Got it, thanks for clarifying.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Refactor recordExtObjInitPriv()

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-16T11:01:47Z

On 12.01.23 18:40, Nathan Bossart wrote:
> On Thu, Jan 12, 2023 at 12:20:50PM -0500, Tom Lane wrote:
>> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>>> On 12.01.23 01:04, Nathan Bossart wrote:
>>> -			 classoid == AggregateRelationId ||
>>>> I noticed that AggregateRelationId isn't listed in the ObjectProperty
>>>> array, so I think recordExtObjInitPriv() will begin erroring for that
>>>> classoid instead of ignoring it like we do today.
>>
>>> Hmm, we do have some extensions in contrib that add aggregates (citext,
>>> intagg).  I suspect that the aggregate function is actually registered
>>> into the extension via its pg_proc entry, so this wouldn't actually
>>> matter.  But maybe the commenting should be clearer?
>>
>> Yeah, I don't believe that AggregateRelationId is used in object
>> addresses; we just refer to pg_proc for any kind of function including
>> aggregates.  Note that there is no "oid" column in pg_aggregate.
> 
> Got it, thanks for clarifying.

I have updated the patch as you suggested and split out the aggregate 
issue into a separate patch for clarity.

Re: Refactor recordExtObjInitPriv()

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-16T22:43:21Z

On Mon, Jan 16, 2023 at 12:01:47PM +0100, Peter Eisentraut wrote:
> I have updated the patch as you suggested and split out the aggregate issue
> into a separate patch for clarity.

LGTM

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Refactor recordExtObjInitPriv()

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-17T19:16:08Z

On 16.01.23 23:43, Nathan Bossart wrote:
> On Mon, Jan 16, 2023 at 12:01:47PM +0100, Peter Eisentraut wrote:
>> I have updated the patch as you suggested and split out the aggregate issue
>> into a separate patch for clarity.
> 
> LGTM

committed