Re: Add support for logging the current role

* 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