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:
/messages/by-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
Attachments:
jsonapi-cleanup.ctext/x-csrc; charset=us-asciiDownload
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index d376ab152d..624b300994 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -20,20 +20,10 @@
#include "common/jsonapi.h"
#include "mb/pg_wchar.h"
-#ifdef FRONTEND
-#include "common/logging.h"
-#else
+#ifndef FRONTEND
#include "miscadmin.h"
#endif
-#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
-
/*
* The context of the parser is maintained by the recursive descent
* mechanism, but is passed explicitly to the error reporting routine
@@ -378,7 +368,9 @@ parse_object(JsonLexContext *lex, JsonSemAction *sem)
JsonTokenType tok;
JsonParseErrorType result;
+#ifndef FRONTEND
check_stack_depth();
+#endif
if (ostart != NULL)
(*ostart) (sem->semstate);
@@ -478,7 +470,9 @@ parse_array(JsonLexContext *lex, JsonSemAction *sem)
json_struct_action aend = sem->array_end;
JsonParseErrorType result;
+#ifndef FRONTEND
check_stack_depth();
+#endif
if (astart != NULL)
(*astart) (sem->semstate);
@@ -1047,7 +1041,6 @@ report_parse_error(JsonParseContext ctx, JsonLexContext *lex)
* unhandled enum values. But this needs to be here anyway to cover the
* possibility of an incorrect input.
*/
- json_log_and_abort("unexpected json parse state: %d", (int) ctx);
return JSON_SUCCESS; /* silence stupider compilers */
}
@@ -1115,8 +1108,7 @@ json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
* unhandled enum values. But this needs to be here anyway to cover the
* possibility of an incorrect input.
*/
- json_log_and_abort("unexpected json parse error type: %d", (int) error);
- return NULL; /* silence stupider compilers */
+ return psprintf("unexpected json parse error type: %d", (int) error);
}
/*
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]: /messages/by-id/a250d475ba1c0cc0efb7dfec8e538fcc77cdcb8e.camel@vmware.com
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]: /messages/by-id/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