On 03/01/2016 08:18 AM, Pavel Stehule wrote:
> 2016-03-01 16:52 GMT+01:00 Joe Conway:
>     On 03/01/2016 02:09 AM, Pavel Stehule wrote:
>     >     > On 01/06/2016 10:36 AM, Tom Lane wrote:
>     >     >> I think a design that was actually somewhat robust would require 
> two
>     >     >> hooks, one at check_role and one at assign_role, wherein the 
> first one
>     >     >> would do any potentially-failing work and package all required 
> info into
>     >     >> a blob that could be passed through to the assign hook.
> 
>     > I see following issues:
>     >
>     > 1. Missing the possibility to pass custom data from SetRoleCheck_hook to
>     > SetRoleAssign_hook. Tom mentioned it in his comment.
> 
>     You can pass the data between the two plugin hook functions in your
>     extension itself, so I see no need to try to pass custom data through
>     the backend. Do any of the other hooks even do that?
> 
> I don't know about it, but these hooks are specific. is it ensured a
> order of calls of these hooks?

>     > 2. Missing little bit more comments and an explanation why and when to
>     > use these hooks.
> 
>     Doesn't look all that different from existing user hooks to me, but
>     sure, I'll add a bit more to the comments.

> some like "I think the main point was to have two hooks. The
> potentially-failing
> work could be done during the check_role() hook and the collected info
> could be used during the assign_role() hook."

Ok, I added a comment similar to that at the check_role() function hook
call site. Updated patch attached.

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 do however find myself wishing I could pass the action down from
set_config_option() to at least the assign_role() hook, but that seems
more invasive than I'd like.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 903b3a6..bcf5cc8 100644
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
***************
*** 32,37 ****
--- 32,41 ----
  #include "utils/timestamp.h"
  #include "mb/pg_wchar.h"
  
+ /* Hooks for plugins to get control in check_role() and assign_role() */
+ SetRoleCheck_hook_type SetRoleCheck_hook = NULL;
+ SetRoleAssign_hook_type SetRoleAssign_hook = NULL;
+ 
  /*
   * DATESTYLE
   */
*************** check_role(char **newval, void **extra,
*** 900,905 ****
--- 904,916 ----
  	myextra->is_superuser = is_superuser;
  	*extra = (void *) myextra;
  
+ 	/*
+ 	 * Potentially-failing work should be done here in the check_role()
+ 	 * hook and the collected info used during the assign_role() hook.
+ 	 */
+ 	if (SetRoleCheck_hook)
+ 		(*SetRoleCheck_hook) (GetSessionUserId(), roleid, is_superuser);
+ 
  	return true;
  }
  
*************** assign_role(const char *newval, void *ex
*** 908,913 ****
--- 919,931 ----
  {
  	role_auth_extra *myextra = (role_auth_extra *) extra;
  
+ 	/*
+ 	 * Any hooks defined here must be able to execute in a failed
+ 	 * transaction to restore a prior value of the ROLE GUC variable.
+ 	 */
+ 	if (SetRoleAssign_hook)
+ 		(*SetRoleAssign_hook) (myextra->roleid, myextra->is_superuser);
+ 
  	SetCurrentRoleId(myextra->roleid, myextra->is_superuser);
  }
  
diff --git a/src/include/commands/variable.h b/src/include/commands/variable.h
index 8105951..f3870e9 100644
*** a/src/include/commands/variable.h
--- b/src/include/commands/variable.h
***************
*** 12,17 ****
--- 12,22 ----
  
  #include "utils/guc.h"
  
+ /* Hooks for plugins to get control in check_role() and assign_role() */
+ typedef void (*SetRoleCheck_hook_type) (Oid, Oid, bool);
+ extern PGDLLIMPORT SetRoleCheck_hook_type SetRoleCheck_hook;
+ typedef void (*SetRoleAssign_hook_type) (Oid, bool);
+ extern PGDLLIMPORT SetRoleAssign_hook_type SetRoleAssign_hook;
  
  extern bool check_datestyle(char **newval, void **extra, GucSource source);
  extern void assign_datestyle(const char *newval, void *extra);

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to