Message-ID: <032148cd-fcdb-6d81-58b2-137a1b77e0e6@dunslane.net>
Date: Mon, 21 Mar 2022 11:41:04 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: New Object Access Type hooks
Content-Language: en-US
To: Mark Dilger <mark.dilger@enterprisedb.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Joshua Brindle <joshua.brindle@crunchydata.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
 Jeff Davis <pgsql@j-davis.com>, Joe Conway <joe@crunchydata.com>
References: <47F87A0E-C0E5-43A6-89F6-D403F2B45175@enterprisedb.com>
From: Andrew Dunstan <andrew@dunslane.net>
In-Reply-To: <47F87A0E-C0E5-43A6-89F6-D403F2B45175@enterprisedb.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/032148cd-fcdb-6d81-58b2-137a1b77e0e6%40dunslane.net>
Precedence: bulk


On 3/17/22 23:21, Mark Dilger wrote:
> Hackers,
>
> Over in [1], Joshua proposed a new set of Object Access Type hooks based on strings rather than Oids.
>
> His patch was written to be applied atop my patch for granting privileges on gucs.
>
> On review of his patch, I became uncomfortable with the complete lack of regression test coverage.  To be fair, he did paste a bit of testing logic to the thread, but it appears to be based on pgaudit, and it is unclear how to include such a test in the core project, where pgaudit is not assumed to be installed.
>
> First, I refactored his patch to work against HEAD and not depend on my GUCs patch.  Find that as v1-0001.  The refactoring exposed a bit of a problem.  To call the new hook for SET and ALTER SYSTEM commands, I need to pass in the Oid of a catalog table.  But since my GUC patch isn't applied yet, there isn't any such table (pg_setting_acl or whatnot) to pass.  So I'm passing InvalidOid, but I don't know if that is right.  In any event, if we want a new API like this, we should think a bit harder about whether it can be used to check operations where no table Oid is applicable.


My first inclination is to say it's probably ok. The immediately obvious
alternative would be to create yet another set of functions that don't
have classId parameters. That doesn't seem attractive.

Modulo that issue I think patch 1 is basically ok, but we should fix the
comments in objectaccess.c.  Rather than "It is [the] entrypoint ..." we
should have something like "Oid variant entrypoint ..." and "Name
variant entrypoint ...", and also fix the function names in the comments.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com