Thread: 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
Attachment
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
Attachment
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