Re: SSL information view

On Fri, Apr 10, 2015 at 7:55 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Fri, Apr 10, 2015 at 4:09 AM, Magnus Hagander wrote:
> > Attached is a patch which does this, at least the way I think you meant.
> And
> > also fixes a nasty crash bug in the previous one that for some reason my
> > testing missed (can't set a pointer to null and then expect to use it
> later,
> > no... When it's in shared memory, it survives into a new backend.)
>
> Good to see that you are back on cleaning up your shame list. Here are
> some comments.
>

:)

This patch has an unused variable when compiled without SSL:
> pgstat.c:2482:28: warning: unused variable 'BackendSslStatusBuffer'
> [-Wunused-variable]
> static PgBackendSSLStatus *BackendSslStatusBuffer = NULL;
>

Hmm. Yeah, clearly I never build without SSL. Added #ifdef protection.

>
> + localsslstatus = (PgBackendSSLStatus *)
> + MemoryContextAlloc(pgStatLocalContext,
> +
> sizeof(PgBackendSSLStatus) * MaxBackends);
> I don't think that it is necessary to do this allocation if !USE_SSL.
> I would just set it to NULL.
>

Well, actually, we don't even *need* localsslstatus if we're not building
with USE_SSL. As the st_ssl value will always be false then we're never
going to look at the buffer, so we might as well skip setting it.

>
> Instead of having both st_ssl and st_sslstatus, I think that you could
> set st_sslstatus = NULL if !USE_SSL and put the ssl status flag
> showing if ssl is enabled or disabled directly in st_sslstatus. This
> will minimize the number of fields related to SSL in PgBackendStatus
> to 1. This mat be a matter of coding style though..
>

Yeah, does it actually matter which struct that field is in? I think the
code becomes more clear this way - as we can now just directly test against
the st_ssl value, and not have to do an "if (x->st_ssl status != NULL &&
x->sslstatus->ssl)" (maybe not exactly that way, but you get what I mean).

>
> +typedef struct PgBackendSSLStatus
> +{
> + /* Information about SSL connection */
> + int ssl_bits;
> + bool ssl_compression;
> + char ssl_version[NAMEDATALEN]; /* MUST be
> null-terminated */
> + char ssl_cipher[NAMEDATALEN]; /* MUST be
> null-terminated */
> + char ssl_clientdn[NAMEDATALEN]; /* MUST be
> null-terminated */
> +} PgBackendSSLStatus;
> git diff is showing in red here, spaces should be replaced with a tab.
>

Ugh. Fixed. One too many copy/pastes I think.

make check is broken, rules.out complaining since you merged the SSL
> fields into pg_stat_get_activity().
>

Good point. I updated it once, but not after the latest change.

New version with those things fixed attached.

(Note that, contrary to what Andres suggested, I would have separated
> the fields of SSL into a separate function and then do a join on PID
> to not bloat pg_stat_activity for users who do not use SSL,
> particularly when the DB is embedded).
>

The latest version *doesn't* bloat pg_stat_activity - it only changes the
resultset of pg_stat_get_activity(), doesn't change the actual view at all.
Or did I get that wrong?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/