On Wed, Mar 13, 2024 at 11:20:16AM -0700, Jacob Champion wrote: > Sounds good, split into v2-0002. (That gives me room to switch other > call sites to the new API, too.) Thanks both!
That looks OK to me. I can see 7~8 remaining sites where StringInfo
data is freed, like in the syslogger, but we should not touch the
StringInfo.
>> case JSON_EXPECTED_STRING:
>> - return psprintf(_("Expected string, but found \"%s\"."),
>> - extract_token(lex));
>> + appendStringInfo(lex->errormsg,
>> + _("Expected string, but found \"%.*s\"."),
>> + toklen, lex->token_start);
>>
>> Maybe this could be wrapped in a macro to avoid copying around
>> token_start and toklen for all the error cases.
>
> I gave it a shot in 0001; see what you think.
That's cleaner, thanks.
Hmm, I think that it is cleaner to create the new API first, and then
rely on it, reversing the order of both patches (perhaps the extra
destroyStringInfo in freeJsonLexContext() should have been moved in
0001). See the attached with few tweaks to 0001, previously 0002 @-@.
I'd still need to do a more serious lookup of 0002, previously 0001.
--
Michael
From 78bfbfe3cbdfd4095bc9ef220dfa11dfdc404556 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 14 Mar 2024 16:58:06 +0900
Subject: [PATCH v3 1/2] Add a helper function for cleaning up StringInfos
destroyStringInfo() is a counterpart to makeStringInfo(): it frees a
palloc'd StringInfo and its data. API originally by Daniel Gustafsson,
with docs and tweaks added by Jacob Champion.
Co-authored-by: Daniel Gustafsson <dan...@yesql.se>
---
src/include/lib/stringinfo.h | 9 ++++++++-
src/backend/backup/basebackup.c | 3 +--
src/backend/commands/subscriptioncmds.c | 3 +--
src/backend/utils/adt/jsonb.c | 3 +--
src/backend/utils/adt/xml.c | 6 ++----
src/common/stringinfo.c | 18 +++++++++++++++++-
src/bin/pg_combinebackup/pg_combinebackup.c | 5 +----
src/test/regress/pg_regress.c | 3 +--
8 files changed, 32 insertions(+), 18 deletions(-)
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 2cd636b01c..cd9632e3fc 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -87,7 +87,8 @@ typedef StringInfoData *StringInfo;
* to be len + 1 in size.
*
* To destroy a StringInfo, pfree() the data buffer, and then pfree() the
- * StringInfoData if it was palloc'd. There's no special support for this.
+ * StringInfoData if it was palloc'd. For StringInfos created with
+ * makeStringInfo(), destroyStringInfo() is provided for this purpose.
* However, if the StringInfo was initialized using initReadOnlyStringInfo()
* then the caller will need to consider if it is safe to pfree the data
* buffer.
@@ -233,4 +234,10 @@ extern void appendBinaryStringInfoNT(StringInfo str,
*/
extern void enlargeStringInfo(StringInfo str, int needed);
+/*------------------------
+ * destroyStringInfo
+ * Frees a StringInfo and its buffer (opposite of makeStringInfo()).
+ */
+extern void destroyStringInfo(StringInfo str);
+
#endif /* STRINGINFO_H */
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 5fbbe5ffd2..5fea86ad0f 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -397,8 +397,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
endtli = backup_state->stoptli;
/* Deallocate backup-related variables. */
- pfree(tablespace_map->data);
- pfree(tablespace_map);
+ destroyStringInfo(tablespace_map);
pfree(backup_state);
}
PG_END_ENSURE_ERROR_CLEANUP(do_pg_abort_backup, BoolGetDatum(false));
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index a05d69922d..5a47fa984d 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -506,8 +506,7 @@ check_publications(WalReceiverConn *wrconn, List *publications)
appendStringInfoChar(cmd, ')');
res = walrcv_exec(wrconn, cmd->data, 1, tableRow);
- pfree(cmd->data);
- pfree(cmd);
+ destroyStringInfo(cmd);
if (res->status != WALRCV_OK_TUPLES)
ereport(ERROR,
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index a5e48744ac..a941654d5a 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -133,8 +133,7 @@ jsonb_send(PG_FUNCTION_ARGS)
pq_begintypsend(&buf);
pq_sendint8(&buf, version);
pq_sendtext(&buf, jtext->data, jtext->len);
- pfree(jtext->data);
- pfree(jtext);
+ destroyStringInfo(jtext);
PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
}
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index beecd0c2ac..3e4ca874d8 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -2163,8 +2163,7 @@ xml_errorHandler(void *data, PgXmlErrorPtr error)
appendBinaryStringInfo(&xmlerrcxt->err_buf, errorBuf->data,
errorBuf->len);
- pfree(errorBuf->data);
- pfree(errorBuf);
+ destroyStringInfo(errorBuf);
return;
}
@@ -2195,8 +2194,7 @@ xml_errorHandler(void *data, PgXmlErrorPtr error)
(errmsg_internal("%s", errorBuf->data)));
}
- pfree(errorBuf->data);
- pfree(errorBuf);
+ destroyStringInfo(errorBuf);
}
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index c61d5c58f3..2910799bfd 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -70,7 +70,7 @@ initStringInfo(StringInfo str)
*
* Reset the StringInfo: the data buffer remains valid, but its
* previous content, if any, is cleared.
- *
+*
* Read-only StringInfos as initialized by initReadOnlyStringInfo cannot be
* reset.
*/
@@ -350,3 +350,19 @@ enlargeStringInfo(StringInfo str, int needed)
str->maxlen = newlen;
}
+
+/*
+ * destroyStringInfo
+ *
+ * Frees a StringInfo and its buffer (opposite of makeStringInfo()).
+ * This must only be called on palloc'd StringInfos.
+ */
+void
+destroyStringInfo(StringInfo str)
+{
+ /* don't allow destroys of read-only StringInfos */
+ Assert(str->maxlen != 0);
+
+ pfree(str->data);
+ pfree(str);
+}
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index 6f0814d9ac..74f8be9eea 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -526,10 +526,7 @@ check_backup_label_files(int n_backups, char **backup_dirs)
/* Free memory that we don't need any more. */
if (lastbuf != buf)
- {
- pfree(buf->data);
- pfree(buf);
- }
+ destroyStringInfo(buf);
/*
* Return the data from the first backup_info that we read (which is the
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index f1f6011ae0..76f01c73f5 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1174,8 +1174,7 @@ psql_end_command(StringInfo buf, const char *database)
}
/* Clean up */
- pfree(buf->data);
- pfree(buf);
+ destroyStringInfo(buf);
}
/*
--
2.43.0
From 348b32ff2d3886783c173b4b6d721bfabbf9798b Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 14 Mar 2024 17:00:33 +0900
Subject: [PATCH v3 2/2] common/jsonapi: support json_errdetail in FRONTEND
json_errdetail() now allocates its error message inside memory owned by
the JsonLexContext, so clients don't need to worry about freeing it. We
can now partially revert b44669b2ca.
---
src/include/common/jsonapi.h | 1 +
src/common/jsonapi.c | 121 ++++++++++--------
src/common/parse_manifest.c | 2 +-
src/bin/pg_verifybackup/t/005_bad_manifest.pl | 3 +-
4 files changed, 73 insertions(+), 54 deletions(-)
diff --git a/src/include/common/jsonapi.h b/src/include/common/jsonapi.h
index 02943cdad8..86a0fc2d00 100644
--- a/src/include/common/jsonapi.h
+++ b/src/include/common/jsonapi.h
@@ -89,6 +89,7 @@ typedef struct JsonLexContext
int line_number; /* line number, starting from 1 */
char *line_start; /* where that line starts within input */
StringInfo strval;
+ StringInfo errormsg;
} JsonLexContext;
typedef JsonParseErrorType (*json_struct_action) (void *state);
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 32931ded82..ec58109837 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -170,23 +170,27 @@ makeJsonLexContextCstringLen(JsonLexContext *lex, char *json,
lex->strval = makeStringInfo();
lex->flags |= JSONLEX_FREE_STRVAL;
}
+ lex->errormsg = NULL;
return lex;
}
/*
- * Free memory in a JsonLexContext. There's no need for this if a *lex
- * pointer was given when the object was made and need_escapes was false,
- * or (in backend environment) a memory context delete/reset is imminent.
+ * Free memory in a JsonLexContext.
+ *
+ * There's no need for this if a *lex pointer was given when the object was
+ * made, need_escapes was false, and json_errdetail() was not called; or if (in
+ * backend environment) a memory context delete/reset is imminent.
*/
void
freeJsonLexContext(JsonLexContext *lex)
{
if (lex->flags & JSONLEX_FREE_STRVAL)
- {
- pfree(lex->strval->data);
- pfree(lex->strval);
- }
+ destroyStringInfo(lex->strval);
+
+ if (lex->errormsg)
+ destroyStringInfo(lex->errormsg);
+
if (lex->flags & JSONLEX_FREE_STRUCT)
pfree(lex);
}
@@ -1145,72 +1149,71 @@ report_parse_error(JsonParseContext ctx, JsonLexContext *lex)
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 an (already translated) 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 safely pfree'd or not.
+ * The returned allocation is either static or owned by the JsonLexContext and
+ * should not be freed.
*/
char *
json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
{
+ if (lex->errormsg)
+ resetStringInfo(lex->errormsg);
+ else
+ lex->errormsg = makeStringInfo();
+
+ /*
+ * A helper for error messages that should print the current token. format
+ * must contain exactly one %.*s specifier.
+ */
+#define token_error(lex, format) \
+ appendStringInfo((lex)->errormsg, _(format), \
+ (int) ((lex)->token_terminator - (lex)->token_start), \
+ (lex)->token_start);
+
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));
+ token_error(lex, "Escape sequence \"\\%.*s\" is invalid.");
+ break;
case JSON_ESCAPING_REQUIRED:
- return psprintf(_("Character with value 0x%02x must be escaped."),
- (unsigned char) *(lex->token_terminator));
+ appendStringInfo(lex->errormsg,
+ _("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));
+ token_error(lex, "Expected end of input, but found \"%.*s\".");
+ break;
case JSON_EXPECTED_ARRAY_FIRST:
- return psprintf(_("Expected array element or \"]\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected array element or \"]\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_ARRAY_NEXT:
- return psprintf(_("Expected \",\" or \"]\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected \",\" or \"]\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_COLON:
- return psprintf(_("Expected \":\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected \":\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_JSON:
- return psprintf(_("Expected JSON value, but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected JSON value, but found \"%.*s\".");
+ break;
case JSON_EXPECTED_MORE:
return _("The input string ended unexpectedly.");
case JSON_EXPECTED_OBJECT_FIRST:
- return psprintf(_("Expected string or \"}\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected string or \"}\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_OBJECT_NEXT:
- return psprintf(_("Expected \",\" or \"}\", but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected \",\" or \"}\", but found \"%.*s\".");
+ break;
case JSON_EXPECTED_STRING:
- return psprintf(_("Expected string, but found \"%s\"."),
- extract_token(lex));
+ token_error(lex, "Expected string, but found \"%.*s\".");
+ break;
case JSON_INVALID_TOKEN:
- return psprintf(_("Token \"%s\" is invalid."),
- extract_token(lex));
+ token_error(lex, "Token \"%.*s\" is invalid.");
+ break;
case JSON_UNICODE_CODE_POINT_ZERO:
return _("\\u0000 cannot be converted to text.");
case JSON_UNICODE_ESCAPE_FORMAT:
@@ -1219,9 +1222,19 @@ json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
/* 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.");
case JSON_UNICODE_UNTRANSLATABLE:
- /* note: this case is only reachable in backend not frontend */
+
+ /*
+ * note: this case is only reachable in backend not frontend.
+ * #ifdef it away so the frontend doesn't try to link against
+ * backend functionality.
+ */
+#ifndef FRONTEND
return psprintf(_("Unicode escape value could not be translated to the server's encoding %s."),
GetDatabaseEncodingName());
+#else
+ Assert(false);
+ break;
+#endif
case JSON_UNICODE_HIGH_SURROGATE:
return _("Unicode high surrogate must not follow a high surrogate.");
case JSON_UNICODE_LOW_SURROGATE:
@@ -1230,13 +1243,17 @@ json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
/* fall through to the error code after switch */
break;
}
+#undef token_error
/*
* 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.
*/
- elog(ERROR, "unexpected json parse error type: %d", (int) error);
- return NULL;
+ if (lex->errormsg->len == 0)
+ appendStringInfo(lex->errormsg,
+ _("unexpected json parse error type: %d"),
+ (int) error);
+
+ return lex->errormsg->data;
}
-#endif
diff --git a/src/common/parse_manifest.c b/src/common/parse_manifest.c
index 3f6d1dfd3d..40ec3b4f58 100644
--- a/src/common/parse_manifest.c
+++ b/src/common/parse_manifest.c
@@ -152,7 +152,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, "parsing failed");
+ json_manifest_parse_failure(context, json_errdetail(json_error, lex));
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
index 77fdfbb9d0..2068babac5 100644
--- a/src/bin/pg_verifybackup/t/005_bad_manifest.pl
+++ b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
@@ -13,7 +13,8 @@ use Test::More;
my $tempdir = PostgreSQL::Test::Utils::tempdir;
test_bad_manifest('input string ended unexpectedly',
- qr/could not parse backup manifest: parsing failed/, <<EOM);
+ qr/could not parse backup manifest: The input string ended unexpectedly/,
+ <<EOM);
{
EOM
--
2.43.0
signature.asc
Description: PGP signature
