Re: SSL information view - Mailing list pgsql-hackers
| From | Magnus Hagander |
|---|---|
| Subject | Re: SSL information view |
| Date | |
| Msg-id | CABUevExoT4+AkQFoXZia9Qj4D1rs37fc_ADD36TxCtF289BvFA@mail.gmail.com Whole thread Raw |
| In response to | Re: SSL information view (Michael Paquier <michael.paquier@gmail.com>) |
| Responses |
Re: SSL information view
|
| List | pgsql-hackers |
On Fri, Apr 10, 2015 at 7:55 AM, Michael Paquier <michael.paquier@gmail.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?
Attachment
pgsql-hackers by date: