Re: [PoC] Federated Authn/z with OAUTHBEARER

On 28.08.24 18:31, Jacob Champion wrote:
> On Mon, Aug 26, 2024 at 4:23 PM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
>> I was having trouble reasoning about the palloc-that-isn't-palloc code
>> during the first few drafts, so I will try a round with the jsonapi_
>> prefix.
> 
> v27 takes a stab at that. I have kept the ALLOC/FREE naming to match
> the strategy in other src/common source files.

This looks pretty good to me.  Maybe on the naming side, this seems like 
a gratuitous divergence:

+#define jsonapi_createStringInfo           makeStringInfo

> The name of the variable JSONAPI_USE_PQEXPBUFFER leads to sections of
> code that look like this:
> 
> +#ifdef JSONAPI_USE_PQEXPBUFFER
> +    if (!new_prediction || !new_fnames || !new_fnull)
> +        return false;
> +#endif
> 
> To me it wouldn't be immediately obvious why "using PQExpBuffer" has
> anything to do with this code; the key idea is that we expect any
> allocations to be able to fail. Maybe a name like JSONAPI_ALLOW_OOM or
> JSONAPI_SHLIB_ALLOCATIONS or...?

Seems ok to me as is.  I think the purpose of JSONAPI_USE_PQEXPBUFFER is 
adequately explained by this comment

+/*
+ * By default, we will use palloc/pfree along with StringInfo.  In libpq,
+ * use malloc and PQExpBuffer, and return JSON_OUT_OF_MEMORY on 
out-of-memory.
+ */
+#ifdef JSONAPI_USE_PQEXPBUFFER

For some of the other proposed names, I'd be afraid that someone might 
think you are free to mix and match APIs, OOM behavior, and compilation 
options.


Some comments on src/include/common/jsonapi.h:

-#include "lib/stringinfo.h"

I suspect this will fail headerscheck?  Probably needs an exception 
added there.

+#ifdef JSONAPI_USE_PQEXPBUFFER
+#define StrValType PQExpBufferData
+#else
+#define StrValType StringInfoData
+#endif

Maybe use jsonapi_StrValType here.

+typedef struct StrValType StrValType;

I don't think that is needed.  It would just duplicate typedefs that 
already exist elsewhere, depending on what StrValType is set to.

+       bool            parse_strval;
+       StrValType *strval;                     /* only used if 
parse_strval == true */

The parse_strval field could use a better explanation.

I actually don't understand the need for this field.  AFAICT, this is
just used to record whether strval is valid.  But in the cases where
it's not valid, why do we need to record that?  Couldn't you just return
failed_oom in those cases?