Re: __pg_log_level in anonynous enum should be initialized? (Was: pgsql: Change SHA2 implementation based on OpenSSL to use EVP digest ro)

On Mon, Sep 28, 2020 at 12:49:29PM -0400, Tom Lane wrote:
> I experimented and confirmed that explicitly initializing __pg_log_level
> would suppress this build error on prairiedog. However, I'm not sure
> that doing so is a good idea, because it seems to me that this animal
> has accidentally saved us from a serious design error. IMO it is quite
> unacceptable for libpq to contain either a forced write to stderr or a
> forced exit(1). It should be reporting the failure back to the calling
> application, instead. So the error handling in this patch needs to be
> reconsidered.

Yeah, I have arrived at the same conclusion after a night of sleep and
after looking at the OpenSSL code for a couple of hours to see if it
would be possible to use a static state of EVP and not have to do all
this error handling. But, we are going to need much more than that,
so I have reverted the patch:
- Any allocation done with EVP is going to need a callback for the
cleanup in the backend, because we basically have to do the allocation
directly in OpenSSL. Not only for the higher EVP structure we get to
use in sha2_openssl.c, but also for some other parts at lower level,
like MD with EVP_MD_CTX_FLAG_NO_INIT.
- It will be better to have those routines return a status and just
let the caller handle the error. This impacts the internals of SCRAM,
particularly for the secret and internal HMAC implementations. And
this needs a change in the fallback implementation.
- We will most likely need to split the existing init and destroy
routines so as the initial allocation and final free happen in
different paths, even for the fallback implementation so as we have a
1:1 mapping with what OpenSSL does.

> Since prairiedog is not likely to be around forever, I propose that
> we ought to enforce this going forward by arranging for common/logging.c
> to not get built into the libpgcommon_shlib library variant.

Agreed. This makes sense in the long term, so attached is a proposal
of patch. I did not really see the point in complicating the MSVC
scripts for that though. The new variable names look natural to me
like that, the new comment may need some tweaks.
--
Michael