From: Michael Paquier <michael@paquier.xyz>
To: Jacob Champion <pchampion@vmware.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Make jsonapi usable from libpq
Date: Thu, 24 Jun 2021 14:56:04 +0900
Message-ID: <YNQedHwheUaTnnGt@paquier.xyz> (raw)
In-Reply-To: <a250d475ba1c0cc0efb7dfec8e538fcc77cdcb8e.camel@vmware.com>
References: <a250d475ba1c0cc0efb7dfec8e538fcc77cdcb8e.camel@vmware.com>
On Tue, Jun 22, 2021 at 10:59:37PM +0000, Jacob Champion wrote:
> Hmm. I'm honestly hesitant to start splitting files apart, mostly
> because json_log_and_abort() is only called from two places, and both
> those places are triggered by programmer error as opposed to user
> error.
>
> Would it make more sense to switch to an fprintf-and-abort case, to
> match the approach taken by PGTHREAD_ERROR and the out-of-memory
> conditions in fe-print.c? Or is there already precedent for handling
> can't-happen code paths with in-band errors, through the PGconn?
Not really..
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.
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(), 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.
--
Michael
Attachments:jsonapi-libpq.patch (text/x-diff, 9.3K) downloaddiff --git a/src/include/common/jsonapi.h b/src/include/common/jsonapi.hindex ec3dfce9c3..f5e25ca3e3 100644--- a/src/include/common/jsonapi.h+++ b/src/include/common/jsonapi.h@@ -48,6 +48,7 @@ typedef enum
JSON_EXPECTED_OBJECT_NEXT,
JSON_EXPECTED_STRING,
JSON_INVALID_TOKEN,
+ JSON_OUT_OF_MEMORY,
JSON_UNICODE_CODE_POINT_ZERO,
JSON_UNICODE_ESCAPE_FORMAT,
JSON_UNICODE_HIGH_ESCAPE,
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.cindex d376ab152d..cf70a735cc 100644--- a/src/common/jsonapi.c+++ b/src/common/jsonapi.c@@ -19,19 +19,22 @@
#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)+/*+ * In backend, we will use palloc/pfree. In frontend, use malloc, and+ * return JSON_OUT_OF_MEMORY on out-of-memory.+ */+#ifndef FRONTEND+#define STRDUP(s) pstrdup(s)+#define ALLOC(size) palloc(size)+#define FREE(s) pfree(s)
#else
-#define json_log_and_abort(...) elog(ERROR, __VA_ARGS__)+#define STRDUP(s) strdup(s)+#define ALLOC(size) malloc(size)+#define FREE(s) free(s)
#endif
/*
@@ -291,12 +294,17 @@ parse_scalar(JsonLexContext *lex, JsonSemAction *sem)
if (lex_peek(lex) == JSON_TOKEN_STRING)
{
if (lex->strval != NULL)
- val = pstrdup(lex->strval->data);+ {+ val = STRDUP(lex->strval->data);+ if (val == NULL)+ return JSON_OUT_OF_MEMORY;+ }
}
else
{
int len = (lex->token_terminator - lex->token_start);
+ //allocation here
val = palloc(len + 1);
memcpy(val, lex->token_start, len);
val[len] = '\0';
@@ -332,7 +340,11 @@ parse_object_field(JsonLexContext *lex, JsonSemAction *sem)
if (lex_peek(lex) != JSON_TOKEN_STRING)
return report_parse_error(JSON_PARSE_STRING, lex);
if ((ostart != NULL || oend != NULL) && lex->strval != NULL)
- fname = pstrdup(lex->strval->data);+ {+ fname = STRDUP(lex->strval->data);+ if (fname == NULL)+ return JSON_OUT_OF_MEMORY;+ }
result = json_lex(lex);
if (result != JSON_SUCCESS)
return result;
@@ -378,7 +390,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 +492,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,76 +1063,138 @@ 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 */
}
/*
- * Construct a detail message for a JSON error.+ * Construct a detail message for a JSON error, palloc'd or malloc'd.+ * note that this may return NULL on OOM in the frontend.
*/
char *
json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
{
+ char *errstr;+ char *tok = NULL;++ /* allocate a string large enough to include the error */+ errstr = ALLOC(256 * sizeof(char));+ if (errstr == NULL)+ return NULL;++ /* Grab current token from the lexing context, if necessary */+ switch (error)+ {+ case JSON_ESCAPING_INVALID:+ case JSON_EXPECTED_ARRAY_FIRST:+ case JSON_EXPECTED_ARRAY_NEXT:+ case JSON_EXPECTED_COLON:+ case JSON_EXPECTED_END:+ case JSON_EXPECTED_JSON:+ case JSON_EXPECTED_OBJECT_FIRST:+ case JSON_EXPECTED_OBJECT_NEXT:+ case JSON_EXPECTED_STRING:+ case JSON_INVALID_TOKEN:+ tok = extract_token(lex);+ /* if allocating the token failed, just give up */+ if (tok == NULL)+ return NULL;+ break;+ default:+ break;+ }+
switch (error)
{
case JSON_SUCCESS:
/* fall through to the error code after switch */
break;
case JSON_ESCAPING_INVALID:
- return psprintf(_("Escape sequence \"\\%s\" is invalid."),- extract_token(lex));+ sprintf(errstr,+ _("Escape sequence \"\\%s\" is invalid."),+ tok);+ break;
case JSON_ESCAPING_REQUIRED:
- return psprintf(_("Character with value 0x%02x must be escaped."),- (unsigned char) *(lex->token_terminator));+ sprintf(errstr,+ _("Character with value 0x%02x must be escaped."),+ (unsigned char) *(lex->token_terminator));+ break;
case JSON_EXPECTED_END:
- return psprintf(_("Expected end of input, but found \"%s\"."),- extract_token(lex));+ sprintf(errstr,+ _("Expected end of input, but found \"%s\"."), tok);+ break;
case JSON_EXPECTED_ARRAY_FIRST:
- return psprintf(_("Expected array element or \"]\", but found \"%s\"."),- extract_token(lex));+ sprintf(errstr,+ _("Expected array element or \"]\", but found \"%s\"."),+ tok);+ break;
case JSON_EXPECTED_ARRAY_NEXT:
- return psprintf(_("Expected \",\" or \"]\", but found \"%s\"."),- extract_token(lex));+ sprintf(errstr,+ _("Expected \",\" or \"]\", but found \"%s\"."),+ tok);+ break;
case JSON_EXPECTED_COLON:
- return psprintf(_("Expected \":\", but found \"%s\"."),- extract_token(lex));+ sprintf(errstr,+ _("Expected \":\", but found \"%s\"."),+ tok);+ break;
case JSON_EXPECTED_JSON:
- return psprintf(_("Expected JSON value, but found \"%s\"."),- extract_token(lex));+ sprintf(errstr,+ _("Expected JSON value, but found \"%s\"."),+ tok);+ break;
case JSON_EXPECTED_MORE:
- return _("The input string ended unexpectedly.");+ sprintf(errstr,+ _("The input string ended unexpectedly."));+ break;
case JSON_EXPECTED_OBJECT_FIRST:
- return psprintf(_("Expected string or \"}\", but found \"%s\"."),- extract_token(lex));+ sprintf(errstr,+ _("Expected string or \"}\", but found \"%s\"."),+ tok);+ break;
case JSON_EXPECTED_OBJECT_NEXT:
- return psprintf(_("Expected \",\" or \"}\", but found \"%s\"."),- extract_token(lex));+ sprintf(errstr,+ _("Expected \",\" or \"}\", but found \"%s\"."),+ tok);+ break;
case JSON_EXPECTED_STRING:
- return psprintf(_("Expected string, but found \"%s\"."),- extract_token(lex));+ sprintf(errstr,+ _("Expected string, but found \"%s\"."),+ tok);+ break;
case JSON_INVALID_TOKEN:
- return psprintf(_("Token \"%s\" is invalid."),- extract_token(lex));+ sprintf(errstr,+ _("Token \"%s\" is invalid."),+ tok);+ break;+ case JSON_OUT_OF_MEMORY:+ sprintf(errstr, _("out of memory."));+ break;+
case JSON_UNICODE_CODE_POINT_ZERO:
- return _("\\u0000 cannot be converted to text.");+ sprintf(errstr, _("\\u0000 cannot be converted to text."));+ break;
case JSON_UNICODE_ESCAPE_FORMAT:
- return _("\"\\u\" must be followed by four hexadecimal digits.");+ sprintf(errstr,+ _("\"\\u\" must be followed by four hexadecimal digits."));+ break;
case JSON_UNICODE_HIGH_ESCAPE:
/* note: this case is only reachable in frontend not backend */
- return _("Unicode escape values cannot be used for code point values above 007F when the encoding is not UTF8.");+ sprintf(errstr,+ _("Unicode escape values cannot be used for code point values above 007F when the encoding is not UTF8."));+ break;
case JSON_UNICODE_HIGH_SURROGATE:
- return _("Unicode high surrogate must not follow a high surrogate.");+ sprintf(errstr,+ _("Unicode high surrogate must not follow a high surrogate."));+ break;
case JSON_UNICODE_LOW_SURROGATE:
- return _("Unicode low surrogate must follow a high surrogate.");+ sprintf(errstr,+ _("Unicode low surrogate must follow a high surrogate."));+ break;
}
- /*- * 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.- */- json_log_and_abort("unexpected json parse error type: %d", (int) error);- return NULL; /* silence stupider compilers */+ if (tok)+ FREE(tok);+ return errstr;
}
/*
@@ -1126,7 +1204,14 @@ static char *
extract_token(JsonLexContext *lex)
{
int toklen = lex->token_terminator - lex->token_start;
- char *token = palloc(toklen + 1);+ char *token = ALLOC(toklen + 1);++ /*+ * There is not much we can do here on OOM, so just return some dummy+ * value.+ */+ if (token == NULL)+ return NULL;
memcpy(token, lex->token_start, toklen);
token[toklen] = '\0';
diff --git a/src/bin/pg_verifybackup/parse_manifest.c b/src/bin/pg_verifybackup/parse_manifest.cindex 3b13ae5b84..e31a05dc35 100644--- a/src/bin/pg_verifybackup/parse_manifest.c+++ b/src/bin/pg_verifybackup/parse_manifest.c@@ -147,7 +147,10 @@ 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));+ {+ char *errstr = json_errdetail(json_error, lex);+ json_manifest_parse_failure(context, errstr ? errstr : "out of memory");+ }
if (parse.state != JM_EXPECT_EOF)
json_manifest_parse_failure(context, "manifest ended unexpectedly");
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, pchampion@vmware.com
Subject: Re: [PATCH] Make jsonapi usable from libpq
In-Reply-To: <YNQedHwheUaTnnGt@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox