2016-03-01 16:52 GMT+01:00 Joe Conway <m...@joeconway.com>: > On 03/01/2016 02:09 AM, Pavel Stehule wrote: > > 2016-02-29 2:40 GMT+01:00 Joe Conway <m...@joeconway.com > > <mailto:m...@joeconway.com>>: > > > > On 01/07/2016 09:08 AM, Joe Conway 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? > > 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. This works fine with the > patch as-is. > > > 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. > > I would to see more lines about just corner cases. Can/cannot to raise a exception there, can/cannot to access system catalogue there. I have negative experience with missing these corner cases with log hook :). 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." The thing I wish we had was a place in the docs where we list all the > user plugin hooks. But as far as I know that doesn't exist (please > correct me if I'm wrong) and I am not volunteering to create it just for > the sake of this patch ;-) > This doc can be nice, but it is out of scope of this patch, and I don't require it :) Regards Pavel > > Thanks for the review! > > Joe > > -- > Crunchy Data - http://crunchydata.com > PostgreSQL Support for Secure Enterprises > Consulting, Training, & Open Source Development > >
