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:

Previous
From: Jan Urbański
Date:
Subject: Re: libpq's multi-threaded SSL callback handling is busted
Next
From: chenhj
Date:
Subject: PATCH:do not set Win32 server-side socket buffer size on windows 2012