Feedback-ID: ic6394509:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-566-g3812ddbbc-fm-20240627.001-g3812ddbb
MIME-Version: 1.0
Message-Id: <aa46ce00-17ad-4a87-938f-c66196800129@app.fastmail.com>
In-Reply-To: <ZoukijRBef4m-VMO@paquier.xyz>
References: <E1sPHWu-000GPc-0z@gemulon.postgresql.org>
 <3408967.1720107891@sss.pgh.pa.us>
 <f2688fb7-add6-43da-b1df-97312a451893@app.fastmail.com>
 <ZoctyZzE3UlUSer_@paquier.xyz>
 <f2539bff-64be-47f0-9f0b-df85d3cc0432@app.fastmail.com>
 <ZoukijRBef4m-VMO@paquier.xyz>
Date: Mon, 08 Jul 2024 11:55:28 +0200
From: "Joel Jacobson" <joel@compiler.org>
To: "Michael Paquier" <michael@paquier.xyz>
Cc: "Tom Lane" <tgl@sss.pgh.pa.us>,
 "Postgres hackers" <pgsql-hackers@lists.postgresql.org>
Subject: Re: pgsql: Add pg_get_acl() to get the ACL for a database object
Content-Type: text/plain
Archived-At: 
 <https://www.postgresql.org/message-id/aa46ce00-17ad-4a87-938f-c66196800129%40app.fastmail.com>
Precedence: bulk

On Mon, Jul 8, 2024, at 10:34, Michael Paquier wrote:
> Thanks for the patch.  I have been looking at it for a few hours,
> eyeing a bit on the ObjectProperty parts a bit if we were to extend it
> for sub-object IDs, and did not like the complexity this introduces,
> so I'd be OK to live with the extra handling in pg_get_acl() itself.
>
> +       /* ignore dropped columns */
> +       if (atttup->attisdropped)
> +       {
> +           ReleaseSysCache(tup);
> +           PG_RETURN_NULL();
> +       }
>
> Hmm.  This is an important bit and did not consider it first.  That
> makes the use of ObjectProperty less flexible because we want to look
> at the data in the pg_attribute tuple to be able to return NULL in
> this case.
>
> I've tweaked a bit what you are proposing, simplifying the code and
> removing the first batch of queries in the tests as these were less
> interesting.  How does that look?

Thanks, nice simplifications.
I agree the tests you removed are not that interesting.

Looks good to me.

Patch didn't apply to HEAD nor on top of any of the previous commits either,
but I couldn't figure out why based on the .rej files, strange.
I copy/pasted the parts from the patch to test it. Let me know if you need a
rebased version of it, in case you will need to do the same, to save some work.

Also noted the below in your last commit:

    a6417078c414 has introduced as project policy that new features
    committed during the development cycle should use new OIDs in the
    [8000,9999] range.

    4564f1cebd43 did not respect that rule, so let's renumber pg_get_acl()
    to use an OID in the correct range.

Thanks for the info!
Will make sure to use the `src/include/catalog/renumber_oids.pl` tool
for future patches.

Regards,
Joel