On Thu, Mar 17, 2022 at 11:21 PM Mark Dilger <mark.dil...@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 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. > > Second, I added a new test directory, src/test/modules/test_oat_hooks, which > includes a new loadable module with hook implementations and a regression > test for testing the object access hooks. The main point of the test is to > log which hooks get called in which order, and which hooks do or do not get > called when other hooks allow or deny access. That information shows 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 the effort. Please note that this is > a quick WIP patch in response to the patch Joshua posted earlier today. > Sorry for sometimes missing function comments, etc. The goal, if this design > seems acceptable, is to polish this, hopefully with Joshua's assistance, and > get it committed *before* my GUCs patch, so that my patch can be rebased to > use it. Otherwise, if this 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 that is about.) > > > > [1] > https://www.postgresql.org/message-id/flat/664799.1647456444%40sss.pgh.pa.us#c9721c2da88d59684ac7ac5fc36f09c1 >
