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

Greetings,

* Don Seiler (don(at)seiler(dot)us) wrote:
> In trying to troubleshoot the source of a recent connection storm, I was
> frustrated to find that the app name was not included in the connection
> messages. It is there in the log_line_prefix on the disconnection messages
> but I would prefer it be directly visible with the connection itself. With
> some guidance from Stephen Frost I've put together this patch which does
> that.

Yeah, I tend to agree that it'd be extremely useful to have this
included in the 'connection authorized' message.

> It adds a new application_name field to the Port struct, stores the
> application name there when processing the startup packet, and then reads
> from there when logging the "connection authorized" message.

Right, to have this included in the 'connection authorized' message, we
need to have it be captured early on, prior to generic GUC handling, and
stored momentairly to be used by the 'connection authorized' message.
Using Port for that seems reasonable (and we already store other things
like user and database there).

Taking a look at the patch itself...

diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 7698cd1f88..088ef346a8 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -135,6 +135,7 @@ typedef struct Port
*/
char *database_name;
char *user_name;
+ char *application_name;
char *cmdline_options;
List *guc_options;

We should have some comments here about how this is the "startup packet
application_name" and that it's specifically used for the "connection
authorized" message and that it shouldn't really be used
post-connection-startup (the GUC should be used instead, as applications
can change it post-startup).

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a4b53b33cd..8e75c80728 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2094,6 +2094,10 @@ retry1:
pstrdup(nameptr));
port->guc_options = lappend(port->guc_options,
pstrdup(valptr));
+
+ /* Copy application_name to port as well */
+ if (strcmp(nameptr, "application_name") == 0)
+ port->application_name = pstrdup(valptr);
}
offset = valoffset + strlen(valptr) + 1;
}

That comment feels a bit lacking- this is a pretty special case which
should be explained.

diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 09e0df290d..86db7630d5 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -266,8 +266,8 @@ PerformAuthentication(Port *port)
#ifdef USE_SSL
if (port->ssl_in_use)
ereport(LOG,
- (errmsg("connection authorized: user=%s database=%s SSL enabled (protocol=%s, cipher=%s, bits=%d, compression=%s)",
- port->user_name, port->database_name,
+ (errmsg("connection authorized: user=%s database=%s application=%s SSL enabled (protocol=%s, cipher=%s, bits=%d, compression=%s)",
+ port->user_name, port->database_name, port->application_name
be_tls_get_version(port),
be_tls_get_cipher(port),
be_tls_get_cipher_bits(port),
@@ -275,8 +275,8 @@ PerformAuthentication(Port *port)
else
#endif
ereport(LOG,
- (errmsg("connection authorized: user=%s database=%s",
- port->user_name, port->database_name)));
+ (errmsg("connection authorized: user=%s database=%s application=%s",
+ port->user_name, port->database_name, port->application_name)));
}
}

You definitely need to be handling the case where application_name is
*not* passed in more cleanly. I don't think we should output anything
different from what we do today in those cases.

Also, these don't need to be / shouldn't be 3 seperate patches/commits,
and there should be a sensible commit message which explains what the
change is entirely.

If you could update the patch accordingly and re-send, that'd be
awesome. :)

Thanks!

Stephen