Re: SSL information view

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.