Re: New Object Access Type hooks

On Thu, Mar 17, 2022 at 11:21 PM Mark Dilger
<mark.dilger@enterprisedb.com> 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
didpaste a bit of testing logic to the thread, but it appears to be based on pgaudit, and it is unclear how to include
sucha 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
refactoringexposed a bit of a problem.  To call the new hook for SET and ALTER SYSTEM commands, I need to pass in the
Oidof 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
likethis, we should think a bit harder about whether it can be used to check operations where no table Oid is
applicable.
>
> Second, I added a new test directory, src/test/modules/test_oat_hooks, which includes a new loadable module with hook
implementationsand a regression test for testing the object access hooks.  The main point of the test is to log which
hooksget called in which order, and which hooks do or do not get called when other hooks allow or deny access.  That
informationshows up in the expected output as NOTICE messages. 
>
> This second patch has gotten a little long, and I'd like another pair of eyes on this before spending a second day on
theeffort.  Please note that this is a quick WIP patch in response to the patch Joshua posted earlier today.  Sorry for
sometimesmissing function comments, etc.  The goal, if this design seems acceptable, is to polish this, hopefully with
Joshua'sassistance, and get it committed *before* my GUCs patch, so that my patch can be rebased to use it.  Otherwise,
ifthis is rejected, I can continue on the GUC patch without this. 
>

This is great, thank you for doing this. I didn't even realize the OAT
hooks had no regression tests.

It looks good to me, I reviewed both and tested the module. I wonder
if the slight abuse of subid is warranted with brand new hooks going
in but not enough to object, I just hope this doesn't rise to the too
large to merge this late level.

> (FYI, I got a test failure from src/test/recovery/t/013_crash_restart.pl when testing v1-0001.  I'm not sure yet what
thatis about.) 
>
>
>
> [1] https://www.postgresql.org/message-id/flat/664799.1647456444%40sss.pgh.pa.us#c9721c2da88d59684ac7ac5fc36f09c1

>