Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add access hook #1399

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Add access hook #1399

wants to merge 2 commits into from

Conversation

orf
Copy link

@orf orf commented Nov 18, 2023

This PR adds support for object_access_hook in much the same way as pgrx_emit_log.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that pointing to a header for each of them is at least hypothetically fine, and would be okay here.

Comment on lines +181 to +185
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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: docs, What do these arguments control?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eeeebbbbrrrr
Copy link
Contributor

Hi @orf! Looks like Jubilee wants a few documentation/comment changes before we merge this PR. Are you up for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants