Re: Proposal: Save user's original authenticated identity for logging

On Mon, Mar 29, 2021 at 11:53:03PM +0000, Jacob Champion wrote:
> It's not a matter of the tests being stable, but of the tests needing
> to change and evolve as the implementation changes. A big part of that
> is visibility into what the tests are doing, so that you can debug
> them.

Sure, but I still don't quite see why this applies here? At the point
of any test, like() or unlink() print the contents of the comparison
if there is a failure, so there is no actual loss of data. That's
what issues_sql_like() does, for one.

> I'm sorry I don't have any explicit examples; the NSS work is pretty
> broad.

Yeah, I saw that..

> I agree that it would be good in general, as long as the consistency
> isn't at the expense of usefulness.
>
> Keep in mind that the rotate-restart-slurp method comes from an
> existing test. I assume Andrew chose that method for the same reasons I
> did -- it works with what we currently have.

PostgresNode::rotate_logfile got introduced in c098509, and it is just
used in t/017_shm.pl on HEAD. There could be more simplifications
with 019_replslot_limit.pl, I certainly agree with that.

> I agree that it'd be ideal not to have to restart the server. But 20%
> of less than ten seconds is less than two seconds, and the test suite
> has to run thousands of times to make up a single hour of debugging
> time that would be (hypothetically) lost by missing log files. (These
> are not easy tests for me to debug and maintain, personally -- maybe
> others have a different experience.)
>
> Is the additional effort to create (and maintain) that new system worth
> two seconds per run? I feel like it's not -- but if you feel strongly
> then I can definitely look into it.

I fear that heavily parallelized runs could feel the difference. Ask
Andres about that, he has been able to trigger in parallel a failure
with pg_upgrade wiping out testtablespace while the main regression
test suite just began :)

> Personally, I'd rather spend time making it easy for tests to get the
> log entries associated with a given connection or query. It seems like
> every suite has had to cobble together its own method of checking the
> log files, with varying levels of success/correctness. Maybe something
> with session_preload_libraries and the emit_log_hook? But that would be
> a job for a different changeset.

Maybe.

>> Causes each attempted connection to the server to be logged,
>> - as well as successful completion of client authentication.
>> + as well as successful completion of client authentication and authorization.
>> I am wondering if this paragraph can be confusing for the end-user
>> without more explanation and a link to the "User Name Maps" section,
>> and if we actually need this addition at all. The difference is that
>> the authenticated log is logged before the authorized log, with user
>> name map checks in-between for some of the auth methods. HEAD refers
>> to the existing authorized log as "authentication" in the logs, while
>> you correct that.
>
> Which parts would you consider confusing/in need of change? I'm happy
> to expand where needed. Would an inline sample be more helpful than a
> textual explanation?

That's with the use of "authentication and authorization". How can
users make the difference between what one or this other is without
some explanation with the name maps? It seems that there is no place
in the existing docs where this difference is explained. I am
wondering if it would be better to not change this paragraph, or
reword it slightly to outline that this may cause more than one log
entry, say:
"Causes each attempted connection to the server, and each
authentication activity to be logged."
--
Michael