Simplify error handing of jsonapi.c for the frontend

michaelpq · michaelpq · commit b44669b2ca6a · 2021-07-02T09:35:12.000+09:00
This commit removes a dependency to the central logging facilities in the JSON parsing routines of src/common/, which existed to log errors when seeing error codes that do not match any existing values in JsonParseErrorType, which is not something that should never happen. The routine providing a detailed error message based on the error code is made backend-only, the existing code being unsafe to use in the frontend as the error message may finish by being palloc'd or point to a static string, so there is no way to know if the memory of the message should be pfree'd or not. The only user of this routine in the frontend was pg_verifybackup, that is changed to use a more generic error message on parsing failure. Note that making this code more resilient to OOM failures if used in shared libraries would require much more work as a lot of code paths still rely on palloc() & friends, but we are not sure yet if we need to go down to that. Still, removing the dependency to logging is a step toward more portability. This cleans up the handling of check_stack_depth() while on it, as it exists only in the backend. Per discussion with Jacob Champion and Tom Lane. Discussion: https://postgr.es/m/YNwL7kXwn3Cckbd6@paquier.xyz
diff --git a/src/bin/pg_verifybackup/parse_manifest.c b/src/bin/pg_verifybackup/parse_manifest.c
@@ -147,7 +147,7 @@ json_parse_manifest(JsonManifestParseContext *context, char *buffer,
 	/* Run the actual JSON parser. */
 	json_error = pg_parse_json(lex, &sem);
 	if (json_error != JSON_SUCCESS)
-		json_manifest_parse_failure(context, json_errdetail(json_error, lex));
+		json_manifest_parse_failure(context, "parsing failed");
 	if (parse.state != JM_EXPECT_EOF)
 		json_manifest_parse_failure(context, "manifest ended unexpectedly");
 
diff --git a/src/bin/pg_verifybackup/t/005_bad_manifest.pl b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
@@ -16,7 +16,7 @@
 
 test_bad_manifest(
 	'input string ended unexpectedly',
-	qr/could not parse backup manifest: The input string ended unexpectedly/,
+	qr/could not parse backup manifest: parsing failed/,
 	<<EOM);
 {
 EOM
diff --git 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
@@ -61,7 +51,6 @@ static JsonParseErrorType parse_object(JsonLexContext *lex, JsonSemAction *sem);
 static JsonParseErrorType parse_array_element(JsonLexContext *lex, JsonSemAction *sem);
 static JsonParseErrorType parse_array(JsonLexContext *lex, JsonSemAction *sem);
 static JsonParseErrorType report_parse_error(JsonParseContext ctx, JsonLexContext *lex);
-static char *extract_token(JsonLexContext *lex);
 
 /* the null action object used for pure validation */
 JsonSemAction nullSemAction =
@@ -378,7 +367,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 +469,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);
@@ -1044,15 +1037,34 @@ report_parse_error(JsonParseContext ctx, JsonLexContext *lex)
 
 	/*
 	 * We don't use a default: case, so that the compiler will warn about
-	 * unhandled enum values.  But this needs to be here anyway to cover the
-	 * possibility of an incorrect input.
+	 * unhandled enum values.
 	 */
-	json_log_and_abort("unexpected json parse state: %d", (int) ctx);
+	Assert(false);
 	return JSON_SUCCESS;		/* silence stupider compilers */
 }
 
+
+#ifndef FRONTEND
+/*
+ * Extract the current token from a lexing context, for error reporting.
+ */
+static char *
+extract_token(JsonLexContext *lex)
+{
+	int			toklen = lex->token_terminator - lex->token_start;
+	char	   *token = palloc(toklen + 1);
+
+	memcpy(token, lex->token_start, toklen);
+	token[toklen] = '\0';
+	return token;
+}
+
 /*
  * Construct a detail message for a JSON error.
+ *
+ * Note that the error message generated by this routine may not be
+ * palloc'd, making it unsafe for frontend code as there is no way to
+ * know if this can be safery pfree'd or not.
  */
 char *
 json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
@@ -1115,20 +1127,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 */
-}
-
-/*
- * Extract the current token from a lexing context, for error reporting.
- */
-static char *
-extract_token(JsonLexContext *lex)
-{
-	int			toklen = lex->token_terminator - lex->token_start;
-	char	   *token = palloc(toklen + 1);
-
-	memcpy(token, lex->token_start, toklen);
-	token[toklen] = '\0';
-	return token;
+	elog(ERROR, "unexpected json parse error type: %d", (int) error);
+	return NULL;
 }
+#endif

Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,7 @@`
`16`	`16`
`17`	`17`	`test_bad_manifest(`
`18`	`18`	`'input string ended unexpectedly',`
`19`		`- qr/could not parse backup manifest: The input string ended unexpectedly/,`
	`19`	`+ qr/could not parse backup manifest: parsing failed/,`
`20`	`20`	`<<EOM);`
`21`	`21`	`{`
`22`	`22`	`EOM`