Re: [PoC] Federated Authn/z with OAUTHBEARER

On 13.08.24 23:11, Jacob Champion wrote:
> On Sun, Aug 11, 2024 at 11:37 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>> I have committed 0002 now.
>
> Thanks Peter! Rebased over both in v26.

I have looked again at the jsonapi memory management patch (v26-0001).
As previously mentioned, I think adding a third or fourth (depending
on how you count) memory management API is maybe something we should
avoid. Also, the weird layering where src/common/ now (sometimes)
depends on libpq seems not great.

I'm thinking, maybe we leave the use of StringInfo at the source code
level, but #define the symbols to use PQExpBuffer. Something like

#ifdef JSONAPI_USE_PQEXPBUFFER

#define StringInfo PQExpBuffer
#define appendStringInfo appendPQExpBuffer
#define appendBinaryStringInfo appendBinaryPQExpBuffer
#define palloc malloc
//etc.

#endif

(simplified, the argument lists might differ)

Or, if people find that too scary, something like

#ifdef JSONAPI_USE_PQEXPBUFFER

#define jsonapi_StringInfo PQExpBuffer
#define jsonapi_appendStringInfo appendPQExpBuffer
#define jsonapi_appendBinaryStringInfo appendBinaryPQExpBuffer
#define jsonapi_palloc malloc
//etc.

#else

#define jsonapi_StringInfo StringInfo
#define jsonapi_appendStringInfo appendStringInfo
#define jsonapi_appendBinaryStringInfo appendBinaryStringInfo
#define jsonapi_palloc palloc
//etc.

#endif

That way, it's at least more easy to follow the source code because
you see a mostly-familiar API.

Also, we should make this PQExpBuffer-using mode only used by libpq,
not by frontend programs. So libpq takes its own copy of jsonapi.c
and compiles it using JSONAPI_USE_PQEXPBUFFER. That will make the
libpq build descriptions a bit more complicated, but everyone who is
not libpq doesn't need to change.

Once you get past all the function renaming, the logic changes in
jsonapi.c all look pretty reasonable. Refactoring like
allocate_incremental_state() makes sense.

You could add pg_nodiscard attributes to
makeJsonLexContextCstringLen() and makeJsonLexContextIncremental() so
that callers who are using the libpq mode are forced to check for
errors. Or maybe there is a clever way to avoid even that: Create a
fixed JsonLexContext like

static const JsonLexContext failed_oom;

and on OOM you return that one from makeJsonLexContext*(). And then
in pg_parse_json(), when you get handed that context, you return
JSON_OUT_OF_MEMORY immediately.

Other than that detail and the need to use freeJsonLexContext(), it
looks like this new mode doesn't impose any additional burden on
callers, since during parsing they need to check for errors anyway,
and this just adds one more error type for out of memory. That's a good
outcome.