Обсуждение: Dependency to logging in jsonapi.c

Hi all,

jsonapi.c includes the following code bits to enforce the use of
logging:
#ifdef FRONTEND
#define check_stack_depth()
#define json_log_and_abort(...) \
   do { pg_log_fatal(__VA_ARGS__); exit(1); } while(0)
#else
#define json_log_and_abort(...) elog(ERROR, __VA_ARGS__)
#endif

This has been mentioned here:
https://www.postgresql.org/message-id/YNfXpFeBVfU2HsVe@paquier.xyz

This requires any tools in the frontend to use pg_logging_init(),
which is recommended, but not enforced.  Perhaps that's fine in
itself to require frontends to register to the central logging APIs,
but json_log_and_abort() gets only called when dealing with incorrect
error codes even if we rely on JsonParseErrorType in all the places
doing error handling with the JSON parsing.  And requiring a
dependency on logging just for unlikely-to-happen cases seems a bit
crazy to me.

Attached is a suggestion of patch to rework that a bit.  Some extra
elog()s could be added for the backend, as well as a new error code to
use as default of report_parse_error(), but that does not seem to gain
much.  And this item looks independent of switching this code to use
pqexpbuffer.h to be more portable with issues like OOM problems.

Thoughts?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> Attached is a suggestion of patch to rework that a bit.  Some extra
> elog()s could be added for the backend, as well as a new error code to
> use as default of report_parse_error(), but that does not seem to gain
> much.  And this item looks independent of switching this code to use
> pqexpbuffer.h to be more portable with issues like OOM problems.

> Thoughts?

+1 in general, but I think I'd replace the one in report_parse_error
with "Assert(false)", rather than just dropping it.

It does not look to me like json_errdetail can sensibly be used in
frontend, since it returns palloc'd strings in some paths and
constants in others.  There'd be no way to avoid a memory leak
in a frontend usage.  So I think the dependency on psprintf there
is not really a problem, but maybe we should make the entire function
"#ifndef FRONTEND" to clarify the intended usage and avoid building
useless code into clients.

            regards, tom lane

On Wed, 2021-06-30 at 11:03 -0400, Tom Lane wrote:
> It does not look to me like json_errdetail can sensibly be used in
> frontend, since it returns palloc'd strings in some paths and
> constants in others.  There'd be no way to avoid a memory leak
> in a frontend usage.  So I think the dependency on psprintf there
> is not really a problem, but maybe we should make the entire function
> "#ifndef FRONTEND" to clarify the intended usage and avoid building
> useless code into clients.

FWIW this is one of the fixes (patch 0002) in the JSON-for-libpq thread
[1]. It ensures that all returned error strings are freeable by the
caller. That in turn was the impetus for the asprintf port suggestion.

But until/unless that is changed, an #ifndef seems like a good way to
prevent issues for the current code.

--Jacob

[1] https://www.postgresql.org/message-id/flat/a250d475ba1c0cc0efb7dfec8e538fcc77cdcb8e.camel@vmware.com

On Wed, Jun 30, 2021 at 03:47:19PM +0000, Jacob Champion wrote:
> On Wed, 2021-06-30 at 11:03 -0400, Tom Lane wrote:
>> It does not look to me like json_errdetail can sensibly be used in
>> frontend, since it returns palloc'd strings in some paths and
>> constants in others.  There'd be no way to avoid a memory leak
>> in a frontend usage.  So I think the dependency on psprintf there
>> is not really a problem, but maybe we should make the entire function
>> "#ifndef FRONTEND" to clarify the intended usage and avoid building
>> useless code into clients.

That sounds sensible from here.  One thing to be aware of is
json_parse_manifest() in pg_verifybackup that uses it, but we could
just replace the error by a plain "failed to parse manifest"".
Backup manifests are generated by the backend, so failures should not
happen there anyway.

> FWIW this is one of the fixes (patch 0002) in the JSON-for-libpq thread
> [1]. It ensures that all returned error strings are freeable by the
> caller. That in turn was the impetus for the asprintf port suggestion.

Yes.

> But until/unless that is changed, an #ifndef seems like a good way to
> prevent issues for the current code.

Sounds sensible to do that as well for 14 before the release.  Any
thoughts about that?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Jun 30, 2021 at 03:47:19PM +0000, Jacob Champion wrote:
>> But until/unless that is changed, an #ifndef seems like a good way to
>> prevent issues for the current code.

> Sounds sensible to do that as well for 14 before the release.  Any
> thoughts about that?

If this code were new in v14, I'd be +1, but it looks like it was
there in 13 too.  So maybe there's somebody external depending on
it, which would make it a bit unfriendly to remove it post-beta.
Let's just add the #ifndef in HEAD.

            regards, tom lane

On Wed, Jun 30, 2021 at 07:00:31PM -0400, Tom Lane wrote:
> If this code were new in v14, I'd be +1, but it looks like it was
> there in 13 too.  So maybe there's somebody external depending on
> it, which would make it a bit unfriendly to remove it post-beta.
> Let's just add the #ifndef in HEAD.

Right, I needed more caffeine at this point in time.  I have cleaned
up that on HEAD, adding an assert at the end of report_parse_error()
as you suggested.
--
Michael