Re: [PATCH] Include application_name in "connection authorized" log message

Greetings Don!

* Don Seiler (don(at)seiler(dot)us) wrote:
> On Tue, Aug 7, 2018 at 12:32 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Don Seiler <don(at)seiler(dot)us> writes:
> >
> > > 1. We want to make a generic, central ascii-lobotomizing function similar
> > > to check_application_name that we can re-use there and for other checks
> > (eg
> > > user name).
> > > 2. Change check_application_name to call this function (or just call this
> > > function instead of check_application_name()?)
> >
> > check_application_name's API is dictated by the GUC check-hook interface,
> > and doesn't really make sense for this other use. So the first part of
> > that, not the second.
> >
> > > 3. Call this function when storing the value in the port struct.
> >
> > I'm not sure where exactly is the most sensible place to call it,
> > but trying to minimize the number of places that know about this
> > kluge seems like a good principle.
>
> OK I created a new function called clean_ascii() in common/string.c. I call
> this from my new logic in postmaster.c as well as replacing the logic in
> guc.c's check_application_name() and check_cluster_name().

Since we're putting it into common/string.c (which seems pretty
reasonable to me, at least), I went ahead and changed it to be
'pg_clean_ascii'. I didn't see any other obvious cases where we could
use this function (though typecmds.c does have an interesting ASCII
check for type categories..).

Otherwise, I added some comments, added application_name to the
replication 'connection authorized' messages (seems like we really
should be consistent across all of them...), ran it through pgindent,
and updated a variable name or two here and there.

> I've been fighting my own confusion with git and rebasing and fighting the
> same conflicts over and over and over, but this patch should be what I
> want. If anyone has time to review my git process, I would appreciate it. I
> must be doing something wrong to have these same conflicts every time I
> rebase (or I completely misunderstand what it actually does).

I'd be happy to chat about it sometime, of course, just have to find
time when we both have a free moment. :)

Attached is the updated patch. If you get a chance to look over it
again and make sure it looks good to you, that'd be great. I did a bit
of testing of it myself but wouldn't complain if someone else wanted to
also.

One thing I noticed while testing is that our 'disconnection' message
still emits 'database=' for replication connections even though the
'connection authorized' message doesn't (presumably because we realized
it's a bit silly to do so when we say 'replication connection'...).
Seems like it'd be nice to have the log_connection / log_disconnection
messages have some consistency about them but that's really a different
discussion from this.

Thanks!

Stephen