From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Magnus Hagander <magnus(at)hagander(dot)net> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Bernd Helmle <mailings(at)oopsware(dot)de>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: SSL information view |
Date: | 2015-04-10 12:14:37 |
Message-ID: | CAB7nPqTG7QtoXKAyHiSZf_X0KgSsmLp02qQLnA_BysOT=PO5Fg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Etsuro Fujita | 2015-04-10 12:40:49 | Re: EvalPlanQual behaves oddly for FDW queries involving system columns |
Previous Message | Michael Paquier | 2015-04-10 12:00:35 | Re: PATCH:do not set Win32 server-side socket buffer size on windows 2012 |