On Fri, Apr 10, 2015 at 6:39 PM, Magnus Hagander wrote:
> On Fri, Apr 10, 2015 at 7:55 AM, Michael Paquier wrote:
>> 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).
That's purely a matter of taste :) I would have done without two
fields in PgBackendStatus with the more complicated if condition...
But well, it doesn't really matter.
>> +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.
>
You forgot one here:
+ /* Information about SSL connection */
>> 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?
My words were visibly incorrect: any callers of pg_stat_get_activity()
would get a bunch of NULL fields for a server built without SSL.
Except for those style comments (feel free to ignore them), I tested
the patch and it is doing what it claims. As I don't have more
comments, let's switch that to "Ready for Committer" then...
Regards,
--
Michael