Add access hook #1399
Add access hook #1399
Conversation
There was a problem hiding this comment.
In-principle, this seems like it should be fine. Even though I want to rewrite most of the hooks API, this one looks like it would probably be identical even after the rewrite. However, new API surface should be thoroughly documented, even if it's primarily pointers to other documentation.
| @@ -176,6 +176,24 @@ pub trait PgHooks { | |||
| prev_hook(pstate, query, jumble_state) | |||
| } | |||
|
|
|||
| fn object_access_hook( | |||
There was a problem hiding this comment.
Please document when this gets run, in principle. Each of these hooks is in a different header file, so we cannot simply point to a single place in the Postgres documentation or even headers for all of them.
There was a problem hiding this comment.
Note that pointing to a header for each of them is at least hypothetically fine, and would be okay here.
| access: pg_sys::ObjectAccessType, | ||
| class_id: pg_sys::Oid, | ||
| object_id: pg_sys::Oid, | ||
| sub_id: ::std::os::raw::c_int, | ||
| arg: *mut ::std::os::raw::c_void, |
There was a problem hiding this comment.
re: docs, What do these arguments control?
There was a problem hiding this comment.
Note that it doesn't have to go deep into practical applications or sophisticated examples.
| arg: *mut ::std::os::raw::c_void, | ||
| ) -> HookResult<()>, | ||
| ) -> HookResult<()> { | ||
| if access == pg_sys::ObjectAccessType_OAT_POST_CREATE { |
There was a problem hiding this comment.
Since this is now becoming part of our public API, please explicitly specify this enum should be generated as a "module const", so that it uses the appropriate style, ahead of
| static mut HOOK: TestHook = TestHook { events: 0 }; | ||
| Spi::run("CREATE TABLE test_hooks_table (bar int)").expect("SPI failed"); | ||
|
|
||
| static mut HOOK: TestHook = TestHook { events: 0, accesses: 0 }; |
There was a problem hiding this comment.
I find myself less-than-excited about our use of a static mut for this test but this is not a problem you introduced so you do not have to fix it, I simply needed to note this for my own purposes.
|
Hi @orf! Looks like Jubilee wants a few documentation/comment changes before we merge this PR. Are you up for that? |
This PR adds support for
object_access_hookin much the same way aspgrx_emit_log.