Re: [PATCH] Make jsonapi usable from libpq

On Fri, Jun 25, 2021 at 08:58:46PM +0000, Jacob Champion wrote:
> On Thu, 2021-06-24 at 14:56 +0900, Michael Paquier wrote:
>> Looking more closely at that, I actually find a bit crazy the
>> requirement for any logging within jsonapi.c just to cope with the
>> fact that json_errdetail() and report_parse_error() just want to track
>> down if the caller is giving some garbage or not, which should never
>> be the case, really. So I would be tempted to eliminate this
>> dependency to begin with.
>
> I think that's a good plan.

We could do this cleanup first, as an independent patch. That's
simple enough. I am wondering if we'd better do this bit in 14
actually, so as the divergence between 15~ and 14 is lightly
minimized.

>> The second thing is how we should try to handle the way the error
>> message gets allocated in json_errdetail(). libpq cannot rely on
>> psprintf(),
>
> That surprised me. So there's currently no compiler-enforced
> prohibition, just a policy? It looks like the bar was lowered a little
> bit in commit c0cb87fbb6, as libpq currently has a symbol dependency on
> pg_get_line_buf() and pfree() on my machine.

Good point. That's worse than just pfree() which is just a plain call
to free() in the frontend. We could have more policies here, but my
take is that we'd better move fe_memutils.o to OBJS_FRONTEND in
src/common/Makefile so as shared libraries don't use those routines in
the long term.

In parseServiceFile(), initStringInfo() does a palloc() which would
simply exit() on OOM, in libpq. That's not good. The service file
parsing is the only piece in libpq using StringInfoData. @Tom,
@Daniel, you got involved in c0cb87f. It looks like this piece about
the limitations with service file parsing needs a rework. This code
is new in 14, which means a new open item.

> If our libpq-specific implementation is going to end up returning NULL
> on bad allocation anyway, could we just modify the behavior of the
> existing front-end palloc implementation to not exit() from inside
> libpq? That would save a lot of one-off code for future implementers.

Yeah, a side effect of that is to enforce a new rule for any frontend
code that calls palloc(), and these could be easily exposed to crashes
within knowing about it until their system is under resource
pressure. Silent breakages with very old guaranteed behaviors is
bad.
--
Michael