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

On Wed, Jun 20, 2018 at 2:45 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

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

Makes sense. I've now moved that variable declaration underneath the others
with a small comment block above it.

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

I've added longer comment explaining again why we are doing this here.

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

I've added some conditional logic similar to the ternary operator usage
in src/backend/catalog/pg_collation.c:92. If application_name is set, we'll
include "application=%s" in the log message, otherwise we make no mention
of it now (output will be identical to what we currently do).

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

After much head scratching/banging on both our parts yesterday, I've
finally figured this out. Thanks again for your patience and time.

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

See attached.

--
Don Seiler
www.seiler.us