On Thu, Nov 24, 2016 at 11:15 PM, Andreas Karlsson <andr...@proxel.se> wrote: > Never mind, I had not fetched the latest master. Once I had done so these > tests were both broken in my rebased branch and in the master branch without > any modifications. I guess I will have to bisect this once I get home. > > Inof for myself or anyone else who feels like bisecting this: the last known > good commit is 67dc4ccbb2e1c27da823eced66d9217a5652cbb0.
Actually, it is a bit stupid of me to not move on with this patch as this is backend-only, and the commit causing the regression failures is 9a1d0af which is a frontend-only changes. So I have done a review of this patch after reverting 9a1d0af, and things are working honestly well. Looking at the latest patch at code-level, there is some refactoring to introduce initialize_context(). But it is actually not necessary (perhaps this is the remnant of a past version?) as be_tls_init() is its only caller. This patch makes hard to look at the diffs, and it makes future back-patching more complicated, so I would suggest simplifying the patch as much as possible. You could use for example a goto block for the error code path to free the context with SSL_CTX_free(), and set up ssl_loaded_verify_locations once the CA is loaded. The same applies to initialize_ecdh(). + if (secure_initialize() != 0) + ereport(FATAL, + (errmsg("could not load ssl context"))); + LoadedSSL = true; In case of a failure, a LOG message would have been already generated, so this duplicates the information. Worse, if log_min_messages is set to a level higher than LOG, users *lose* information on what has happened. I think that secure_initialize() should have an argument to define elevel and that this routine should be in charge of generating an error message. Having it return a status code is necessary, but you could cast secure_initialize() with (void) to show that we don't care about the status code in this case. There is no need to care about freeing the new context when the FATAL code path is used as process would just shut down. config.sgml needs to be updated to reflect that the SSL parameters are updated at server reload (mentioned already upthread, just re-mentioning it to group all the comments in one place). -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers