Re: Add support for logging the current role - Mailing list pgsql-hackers

Is this something for the next commit-fest?

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

Stephen Frost wrote:
-- Start of PGP signed section.
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > Robert Haas <robertmhaas@gmail.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@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +