Re: SSL information view

On Wed, Dec 17, 2014 at 9:19 PM, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com
> wrote:

> On 11/19/2014 02:36 PM, Magnus Hagander wrote:
>
>> + /* Create or attach to the shared SSL status buffers */
>> + size = mul_size(NAMEDATALEN, MaxBackends);
>> + BackendSslVersionBuffer = (char *)
>> + ShmemInitStruct("Backend SSL Version Buffer", size,
>> &found);
>> +
>> + if (!found)
>> + {
>> + MemSet(BackendSslVersionBuffer, 0, size);
>> +
>> + /* Initialize st_ssl_version pointers. */
>> + buffer = BackendSslVersionBuffer;
>> + for (i = 0; i < MaxBackends; i++)
>> + {
>> + BackendStatusArray[i].st_ssl_version = buffer;
>> + buffer += NAMEDATALEN;
>> + }
>> + }
>> +
>> + size = mul_size(NAMEDATALEN, MaxBackends);
>> + BackendSslCipherBuffer = (char *)
>> + ShmemInitStruct("Backend SSL Cipher Buffer", size,
>> &found);
>> +
>> + if (!found)
>> + {
>> + MemSet(BackendSslCipherBuffer, 0, size);
>> +
>> + /* Initialize st_ssl_cipher pointers. */
>> + buffer = BackendSslCipherBuffer;
>> + for (i = 0; i < MaxBackends; i++)
>> + {
>> + BackendStatusArray[i].st_ssl_cipher = buffer;
>> + buffer += NAMEDATALEN;
>> + }
>> + }
>> +
>> + size = mul_size(NAMEDATALEN, MaxBackends);
>> + BackendSslClientDNBuffer = (char *)
>> + ShmemInitStruct("Backend SSL Client DN Buffer", size,
>> &found);
>> +
>> + if (!found)
>> + {
>> + MemSet(BackendSslClientDNBuffer, 0, size);
>> +
>> + /* Initialize st_ssl_clientdn pointers. */
>> + buffer = BackendSslClientDNBuffer;
>> + for (i = 0; i < MaxBackends; i++)
>> + {
>> + BackendStatusArray[i].st_ssl_clientdn = buffer;
>> + buffer += NAMEDATALEN;
>> + }
>> + }
>>
>
> This pattern gets a bit tedious. We do that already for application_names,
> client hostnames, and activity status but this adds three more such
> strings. Why are these not just regular char arrays in PgBackendStatus
> struct, anyway? The activity status is not, because its size is
> configurable with the pgstat_track_activity_query_size GUC, but all those
> other things are fixed-size.
>
> Also, it would be nice if you didn't allocate the memory for all those SSL
> strings, when SSL is disabled altogether. Perhaps put the SSL-related
> information into a separate struct:
>
> struct
> {
> /* Information about SSL connection */
> int st_ssl_bits;
> bool st_ssl_compression;
> char st_ssl_version[NAMEDATALEN]; /* MUST be
> null-terminated */
> char st_ssl_cipher[NAMEDATALEN]; /* MUST be
> null-terminated */
> char st_ssl_clientdn[NAMEDATALEN]; /* MUST be
> null-terminated */
> } PgBackendSSLStatus;
>
> Those structs could be allocated like you allocate the string buffers now,
> with a pointer to that struct from PgBackendStatus. When SSL is disabled,
> the structs are not allocated and the pointers in PgBackendStatus structs
> are NULL.

Finally, I found time to do this. PFA a new version of this patch.

It takes into account the changes suggested by Heikki and Alex (minus the
renaming of fields - I think that's a separate thing to do, and we should
stick to existing naming conventions for now - but I changed the order of
the fields). Also the documentation changes suggested by Peter (but still
not the contrib/sslinfo part, as that should be a separate patch - but I
can look at that once we agree on this one). And resolves the inevitable
oid conflict for a patch that's been delayed that long.

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