Re: [PATCH] Make jsonapi usable from libpq

Michael Paquier <michael(at)paquier(dot)xyz> writes:
> On Fri, Jun 25, 2021 at 08:58:46PM +0000, Jacob Champion wrote:
>> 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.

Ugh. Not only is that bad, but your proposed fix doesn't fix it.
At least in psql, and probably in most/all of our other clients,
removing fe_memutils.o from libpq's link just causes it to start
relying on the copy in the psql executable :-(. So I agree that
some sort of mechanical enforcement would be a really good thing,
but I'm not sure what it would look like.

> 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.

I concur with Daniel that the easiest fix for v14 is to revert
c0cb87f. Allowing unlimited-length lines in the service file seems
like a nice-to-have, but it's not worth a lot. (Looking at the patch,
I'm inclined to keep much of the code rearrangement, just remove the
dependency on stringinfo.c. Also I'm tempted to set the fixed buffer
size at 1024 not 256, after which we might never need to improve it.)

I spent some time looking for other undesirable symbol dependencies
in libpq, and soon found a couple. PGTHREAD_ERROR potentially calls
abort(), which seems even worse than exit-on-OOM, although I don't
think we've ever heard a report of that being hit. Also,
fe-print.c's handling of OOM isn't nice at all:

fprintf(stderr, libpq_gettext("out of memory\n"));
abort();

Although fe-print.c is semi-deprecated, it still seems like it'd
be a good idea to clean that up.

BTW, so far as the original topic of this thread is concerned,
it sounds like Jacob's ultimate goal is to put some functionality
into libpq that requires JSON parsing. I'm going to say up front
that that sounds like a terrible idea. As we've just seen, libpq
operates under very tight constraints, not all of which are
mechanically enforced. I am really doubtful that anything that
would require a JSON parser has any business being in libpq.
Unless you can sell us on that point, I do not think it's worth
complicating the src/common JSON code to the point where it can
work under libpq's constraints.

regards, tom lane