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

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Don Seiler <don(at)seiler(dot)us>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: [PATCH] Include application_name in "connection authorized" log message
Date: 2018-08-07 14:57:59
Message-ID: 20180807145759.GU27724@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Greetings Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> >> Moreover, if you don't check it then the appname recorded
> >> by log_connections would not match appearances for the same session
> >> later in the log, which puts the entire use-case for this patch into
> >> question. So no, this concern must not be dismissed.
>
> > If the call to check_application_name() fails then I had been expecting
> > the connection to fail. If we continue to let the connection go on then
> > we've already got an issue as someone might pass in an application name
> > that isn't being set to the GUC and isn't being therefore used in the
> > existing log_line_prefix where it should be.
>
> No, check_application_name doesn't reject funny names, it just silently
> modifies them in-place.

Blargh, that doesn't seem particularly good to me, but I guess no one
has been comlaining about it either.

> >> However ... I've not looked at the patch, but I thought the idea was to
> >> allow assignment of that GUC to occur before the log_connections log entry
> >> is emitted, so that it'd show up in the entry's log_line_prefix. Wouldn't
> >> check_application_name happen automatically at that point?
>
> > We log that message quite early and it certainly didn't look trivial to
> > set up the GUC to be already in place at that point, so the plan was to
> > simply spit out what gets passed in (as we were doing for "user", if I'm
> > remembering that code correctly...).
>
> Hm. Well, the code isn't exactly complicated, you could duplicate it.
> Or maybe better refactor to allow it to be called from $wherever.
> Looks like check_cluster_name, for one, could also share use of
> an ascii-lobotomizing subroutine.

Yeah, I'd be alright with either of those approaches.

> But having said that, I don't exactly see why you couldn't force it
> with an ultimately-redundant SetConfigOption call to put the value
> in place before the ereport happens. The GUC machinery is surely
> functional before we do authorization.

If that's the approach you think makes the most sense, I wouldn't object
to it. I will point out that we'd end up with the application name in
the log line if it's also included in log_line_prefix, but that's what
happens with "user" anyway, isn't it?, so that doesn't seem to be a big
deal. I do think it's still good to have appplication_name explicitly
in the log message for users who want to just log application_name on
connection and not have it on every single log line.

Thanks!

Stephen

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Stephen Frost 2018-08-07 15:42:04 Re: Standby trying "restore_command" before local WAL
Previous Message Tom Lane 2018-08-07 14:42:51 Re: [PATCH] Include application_name in "connection authorized" log message