Re: Add support for logging the current role

Is this something for the next commit-fest?

---------------------------------------------------------------------------

Stephen Frost wrote:
-- Start of PGP signed section.
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > > It seems there's at least one more thing to worry about here, which is
> > > the overhead of this computation when CSV logging is in use. If no
> > > SET ROLE or SET SESSION AUTHORIZATION commands are in use, the code
> > > will call show_role(), which will return "none". We'll then strcmp()
> > > that against "none" and decide to call show_session_authorization(),
> > > which will call strtoul() to find the comma separator and then return
> > > a pointer to the string that follows it. Now, none of that is
> > > enormously expensive, so maybe it's not worth worrying about, but
> > > since logging can be a hotspot, I thought I'd mention it and solicit
> > > an opinion on whether that's likely to be a problem in practice.
> >
> > Well, in the first place, going through two not-very-related APIs in
> > order to reverse-engineer what miscinit.c already knows is pretty silly
> > (not to mention full of possible bugs). We ought to be looking at the
> > GetUserId state directly.
>
> GetUserId can end up being set in a number of places though, often in
> places where we can't fail (SetUserIdAndSecContext has some nice
> comments on this).
>
> > Now you will complain that elog.c mustn't try to map that OID back to
> > string form, which is true. But IIRC, we used to keep the current
> > userid stored in both OID and string form. The string form was removed
> > as unnecessary overhead, but maybe it'd be a good idea to put that back.
>
> The OID and the string are kept in the role_string and
> session_authorization_string GUCs respectively. They're just not in a
> terribly useful format, and because SetUserId() can change things w/o
> the GUCs getting updated, there's a risk that they're wrong, which is
> why show_role() does the stroul() dance to check if GetCurrentRoleId()
> matches to what it stuffed into role_string.
>
> > In short, add a bit of overhead at SetUserId time in order to make this
> > cheap (and accurate) in elog.c.
>
> We can't do the lookup in SetUserIDAndSecContext(), and I'm not
> convinced we actually want to anyway, since that would end up returning
> what the role is inside of security definer functions and the like.
> We're already setting a variable in assign_session_authorization and
> assign_role that has the information we need. We could inspect
> role_string ourselves (including the strcmp() and strtoul()) instead
> of asking show_role() to do it for us but that doesn't strike me as all
> *that* much of an improvement and goes around the API that at least
> exists.
>
> We could certainly have a second set of variables which are set by
> assign_role/assign_session_authorization that are in a format we can
> more easily use but what would that mean for the GUC variables..? I
> don't know that we'd want to keep them duplicating the data.. Would it
> be possible to actually use a struct instead of a straight-up string
> there? Is there any particular reason we keep monkeying around with
> storing the OID, superuser bit, role name, etc, as a string anyway..?
>
> Thanks,
>
> Stephen
-- End of PGP section, PGP failed!

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +