From: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
---|---|
To: | Joe Conway <mail(at)joeconway(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de> |
Subject: | Re: Proposal: SET ROLE hook |
Date: | 2016-03-01 16:18:25 |
Message-ID: | CAFj8pRAcOTTR_TqN+-Z8gmxphg3Nk_LOEh_A2mguU1bN2f3qnA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
2016-03-01 16:52 GMT+01:00 Joe Conway <mail(at)joeconway(dot)com>:
> On 03/01/2016 02:09 AM, Pavel Stehule wrote:
> > 2016-02-29 2:40 GMT+01:00 Joe Conway <mail(at)joeconway(dot)com
> > <mailto:mail(at)joeconway(dot)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
>
>
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2016-03-01 16:30:59 | Re: PROPOSAL: Fast temporary tables |
Previous Message | Joe Conway | 2016-03-01 16:16:23 | Re: pg_dump dump catalog ACLs |