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;
+ 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.
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..
+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.
make check is broken, rules.out complaining since you merged the SSL
fields into pg_stat_get_activity().
(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).
Regards,
--
Michael