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.

diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index a576843234ebc1678eee7a223cba1fbac4a07291..1d6b752a28b6ad40d5cf088c14ad3e481dcde716 100644 (file)
--- 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;
 }
 
 /*