Re: [PATCH] Reload SSL certificates on SIGHUP

From: Andreas Karlsson <andreas(at)proxel(dot)se>
To: Michael Banck <michael(dot)banck(at)credativ(dot)de>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Peter Geoghegan <pg(at)heroku(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Peter Eisentraut <peter_e(at)gmx(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Reload SSL certificates on SIGHUP
Date: 2016-11-07 01:36:00
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Thanks for the review! I have fixed all your feedback in the attached patch.

On 11/03/2016 04:24 PM, Michael Banck wrote:
> It does not update the documentation, I think at least section 18.9
> "Secure TCP/IP Connections with SSL" needs updating as it says: "The
> files server.key, server.crt, root.crt, and root.crl (or their
> configured alternative names) are only examined during server start; so
> you must restart the server for changes in them to take effect".

Changed this.

> However see below for segfaults during testing.

Fixed, this was due to me not setting SSL_Context to NULL after freeing it.

>> [...]
> I am not sure this '!!' operator is according to project policy, it
> seems to be not used elsewhere in the codebase except for imported or
> auto-generated code. At least there should be a comment here, methinks.

I changed the code to compare with '\0' instead.

> [...]
> Missing semicolon at the end of the line.
> [...]
> Opening braces should be on the next line.
> [...]
> Same.


>> [...]
> All the delarations above this one are global variables for GUCs (see
> postmaster.h, lines 16-31). So ISTM this static variable declaration
> should be introduced by a comment in order to logically seperate it from
> the previous ones, like the following static variables are:


>> [...]
> This hunk baffled me at first because two lines below your added
> secure_destroy() declaration, the same line is already present. I did
> some digging and it turns out we had a secure_destroy() in the ancient
> past, but its implementation got removed in 2008 in 4e8162865 as there
> were no (more) users of it, however, the declaration was kept on until
> now.
> So this hunk should be removed I guess.



Attachment Content-Type Size
reload-ssl-v04.patch text/x-patch 21.5 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Etsuro Fujita 2016-11-07 02:24:51 Re: Push down more full joins in postgres_fdw
Previous Message Franck Verrot 2016-11-07 01:26:15 Re: Mention column name in error messages