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
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 |