From f0eeba6f83ed16687d8743b34e519de45c3915ec Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 2 Dec 2014 15:02:40 -0500 Subject: [PATCH] Fix JSON aggregates to work properly when final function is re-executed. Davide S. reported that json_agg() sometimes produced multiple trailing right brackets. This turns out to be because json_agg_finalfn() attaches the final right bracket, and was doing so by modifying the aggregate state in-place. That's verboten, though unfortunately it seems there's no way for nodeAgg.c to check for such mistakes. Fix that back to 9.3 where the broken code was introduced. In 9.4 and HEAD, likewise fix json_object_agg(), which had copied the erroneous logic. Make some cosmetic cleanups as well. --- src/backend/utils/adt/json.c | 45 +++++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c index a576843234..1d6b752a28 100644 --- a/src/backend/utils/adt/json.c +++ b/src/backend/utils/adt/json.c @@ -94,6 +94,7 @@ static void datum_to_json(Datum val, bool is_null, StringInfo result, bool key_scalar); static void add_json(Datum val, bool is_null, StringInfo result, Oid val_type, bool key_scalar); +static text *catenate_stringinfo_string(StringInfo buffer, const char *addon); /* the null action object used for pure validation */ static JsonSemAction nullSemAction = @@ -175,7 +176,7 @@ lex_expect(JsonParseContext ctx, JsonLexContext *lex, JsonTokenType token) /* utility function to check if a string is a valid JSON number */ extern bool -IsValidJsonNumber(const char * str, int len) +IsValidJsonNumber(const char *str, int len) { bool numeric_error; JsonLexContext dummy_lex; @@ -200,7 +201,7 @@ IsValidJsonNumber(const char * str, int len) json_lex_number(&dummy_lex, dummy_lex.input, &numeric_error); - return ! numeric_error; + return !numeric_error; } /* @@ -1370,7 +1371,7 @@ datum_to_json(Datum val, bool is_null, StringInfo result, text *jsontext; /* callers are expected to ensure that null keys are not passed in */ - Assert( ! (key_scalar && is_null)); + Assert(!(key_scalar && is_null)); if (is_null) { @@ -1404,6 +1405,7 @@ datum_to_json(Datum val, bool is_null, StringInfo result, break; case JSONTYPE_NUMERIC: outputstr = OidOutputFunctionCall(outfuncoid, val); + /* * Don't call escape_json for a non-key if it's a valid JSON * number. @@ -1798,6 +1800,8 @@ to_json(PG_FUNCTION_ARGS) /* * json_agg transition function + * + * aggregate input column as a json array value. */ Datum json_agg_transfn(PG_FUNCTION_ARGS) @@ -1884,18 +1888,18 @@ json_agg_finalfn(PG_FUNCTION_ARGS) state = PG_ARGISNULL(0) ? NULL : (StringInfo) PG_GETARG_POINTER(0); + /* NULL result for no rows in, as is standard with aggregates */ if (state == NULL) PG_RETURN_NULL(); - appendStringInfoChar(state, ']'); - - PG_RETURN_TEXT_P(cstring_to_text_with_len(state->data, state->len)); + /* Else return state with appropriate array terminator added */ + PG_RETURN_TEXT_P(catenate_stringinfo_string(state, "]")); } /* * json_object_agg transition function. * - * aggregate two input columns as a single json value. + * aggregate two input columns as a single json object value. */ Datum json_object_agg_transfn(PG_FUNCTION_ARGS) @@ -1909,7 +1913,7 @@ json_object_agg_transfn(PG_FUNCTION_ARGS) if (!AggCheckCallContext(fcinfo, &aggcontext)) { /* cannot be called directly because of internal-type argument */ - elog(ERROR, "json_agg_transfn called in non-aggregate context"); + elog(ERROR, "json_object_agg_transfn called in non-aggregate context"); } if (PG_ARGISNULL(0)) @@ -1976,7 +1980,6 @@ json_object_agg_transfn(PG_FUNCTION_ARGS) /* * json_object_agg final function. - * */ Datum json_object_agg_finalfn(PG_FUNCTION_ARGS) @@ -1988,12 +1991,32 @@ json_object_agg_finalfn(PG_FUNCTION_ARGS) state = PG_ARGISNULL(0) ? NULL : (StringInfo) PG_GETARG_POINTER(0); + /* NULL result for no rows in, as is standard with aggregates */ if (state == NULL) PG_RETURN_NULL(); - appendStringInfoString(state, " }"); + /* Else return state with appropriate object terminator added */ + PG_RETURN_TEXT_P(catenate_stringinfo_string(state, " }")); +} + +/* + * Helper function for aggregates: return given StringInfo's contents plus + * specified trailing string, as a text datum. We need this because aggregate + * final functions are not allowed to modify the aggregate state. + */ +static text * +catenate_stringinfo_string(StringInfo buffer, const char *addon) +{ + /* custom version of cstring_to_text_with_len */ + int buflen = buffer->len; + int addlen = strlen(addon); + text *result = (text *) palloc(buflen + addlen + VARHDRSZ); + + SET_VARSIZE(result, buflen + addlen + VARHDRSZ); + memcpy(VARDATA(result), buffer->data, buflen); + memcpy(VARDATA(result) + buflen, addon, addlen); - PG_RETURN_TEXT_P(cstring_to_text_with_len(state->data, state->len)); + return result; } /* -- 2.40.0