Is this something for the next commit-fest? ---------------------------------------------------------------------------
Stephen Frost wrote: -- Start of PGP signed section. > * Tom Lane (t...@sss.pgh.pa.us) wrote: > > Robert Haas <robertmh...@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 <br...@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
