Confusing #if nesting in hmac_openssl.c

On 2 Apr 2024, at 02:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:

hmac_openssl.c:90:1: warning: unused function 'ResourceOwnerRememberHMAC' [-Wunused-function]
hmac_openssl.c:95:1: warning: unused function 'ResourceOwnerForgetHMAC' [-Wunused-function]

Looking at the code, this is all from commit e6bdfd970, and apparently
batfish is our only animal that doesn't HAVE_HMAC_CTX_NEW.

Thanks for looking at this, it's been on my TODO for some time. It's a warning
which only shows up when building against 1.0.2, the functions are present in
1.1.0 and onwards (while deprecated in 3.0).

I don't think
it is helpful to put the resource owner manipulations inside #ifdef
HAVE_HMAC_CTX_NEW and HAVE_HMAC_CTX_FREE --- probably, it would never
be the case that only one of those is defined,

Correct, no version of OpenSSL has only one of them defined.

What do you think of rearranging it as attached?

+1 on this patch, it makes the #ifdef soup more readable. We could go even
further and remove the HAVE_HMAC defines completely with USE_RESOWNER_FOR_HMAC
being set by autoconf/meson? I've attached an untested sketch diff to
illustrate.

A related tangent. If we assembled the data to calculate on ourselves rather
than rely on OpenSSL to do it with subsequent _update calls we could instead
use the simpler HMAC() API from OpenSSL. That would remove the need for the
HMAC_CTX and resource owner tracking entirely and just have our pg_hmac_ctx.
Thats clearly not for this patch though, just thinking out loud that we set up
OpenSSL infrastructure that we don't really use.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:

On 2 Apr 2024, at 02:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I don't think
it is helpful to put the resource owner manipulations inside #ifdef
HAVE_HMAC_CTX_NEW and HAVE_HMAC_CTX_FREE --- ...
What do you think of rearranging it as attached?

+1 on this patch, it makes the #ifdef soup more readable.

Thanks for looking at it.

We could go even
further and remove the HAVE_HMAC defines completely with USE_RESOWNER_FOR_HMAC
being set by autoconf/meson? I've attached an untested sketch diff to
illustrate.

I'm inclined to think that won't work, because we need the HAVE_
macros separately to compile correct frontend code.

A related tangent. If we assembled the data to calculate on ourselves rather
than rely on OpenSSL to do it with subsequent _update calls we could instead
use the simpler HMAC() API from OpenSSL. That would remove the need for the
HMAC_CTX and resource owner tracking entirely and just have our pg_hmac_ctx.
Thats clearly not for this patch though, just thinking out loud that we set up
OpenSSL infrastructure that we don't really use.

Simplifying like that could be good, but I'm not volunteering.
For the moment I'd just like to silence the buildfarm warning,
so I'll go ahead with what I have.

regards, tom lane

On 2 Apr 2024, at 15:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'll go ahead with what I have.

+1

--
Daniel Gustafsson

On Tue, Apr 02, 2024 at 03:56:13PM +0200, Daniel Gustafsson wrote:

On 2 Apr 2024, at 15:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'll go ahead with what I have.

+1

+#ifdef USE_RESOWNER_FOR_HMAC

Why not, that's cleaner. Thanks for the commit. The interactions
between this code and b8bff07da are interesting.
--
Michael