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.
> 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.
> , so I can think about two options here:
> - Let the caller of json_errdetail() allocate the memory area of the
> error message by itself, pass it down to the function.
> - Do the allocation within json_errdetail(), and let callers cope the
> case where json_errdetail() itself fails on OOM for any frontend code
> using it.
>
> Looking at HEAD, the OAUTH patch and the token handling, the second
> option looks more interesting. I have to admit that handling the
> token part makes the patch a bit special, but that avoids duplicating
> all those error messages for libpq. Please see the idea as attached.
I prefer the second approach as well. Looking at the sample
implementation -- has an allocating sprintf() for libpq really not been
implemented before? Doing it ourselves on the stack gives me some
heartburn; at the very least we'll have to make careful use of snprintf
so as to not smash the stack while parsing malicious JSON.
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.
--Jacob