Re: Proposal: SET ROLE hook

On 6 March 2016 at 04:49, Joe Conway <mail(at)joeconway(dot)com> wrote:

> I still don't see any point in trying to pass data back from the hooks
> as the extension can maintain that state just fine, although it looks
> like it would be pretty trivial to do using a new void* member added to
> role_auth_extra. Tom (or anyone else), any comment on that?

I think it's possibly worth doing, but since auth is simple and linear I'm
not overly bothered. It'd hardly be the first place Pg relied on passing
information between functions using globals.

If you expect to daisy-chain hooks then a private-data member isn't too
helpful, since a prior hook in the call chain may have set it already and
you don't know what's there or how to manage it. So overall, I'm -1 for a
private_data arg, I don't think it gains us anything in lifetime
management, code clarity etc and it makes daisy-chaining way more complex.

I don't see what the point of SetRoleAssign_hook is, since it returns void
and doesn't have out parameters. If it's expected to takes some action,
what is it? If it's meant to modify myextra->roleid and
myextra->is_superuser prior to their use by SetCurrentRoleId they should be
passed pointer-indirected so they can be modified by the hook.

I'd like to see parameter names specified in the hook type definition.

It needs tests added, along with a note somewhere on usage of the hook that
mentions the usual pattern for using it, possibly in the test/example.
Something like:

static SetRoleCheck_hook_type previous_SetRoleCheck_hook = NULL;

void my_check_role(Oid sessionuserid, Oid wanted_roleid, Oid issuper)
{
if (previous_SetRoleCheck_hook)
previous_SetRoleCheck_hook(sessionuserid, wanted_roleid, issuper);

/* do my stuff here */
}

_PG_init()
{
if (SetRoleCheck_hook)
previous_SetRoleCheck_hook = SetRoleCheck_hook;
SetRoleCheck_hook = my_check_role;
}

Also behaviour of each hook should be more completely specified. Can it
elog(ERROR)? What happens if it does? What can it safely change and what
not? Are there restrictions to what it can do in terms of access to
syscache/relcache/heap etc?

Why do you pass GetSessionUserId() to the hook given that the caller can
look it up directly? Just a convenience?

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services