On 05/19/2015 07:11 PM, Andrew Dunstan wrote:
On 05/18/2015 10:52 PM, Peter Geoghegan wrote:
On Mon, May 18, 2015 at 7:11 AM, Andrew Dunstan <and...@dunslane.net>
wrote:
Here's an patch along those lines. It seems to do the trick, at
least for
your test case, and it has the merit of being very small, so small
I'd like
to backpatch it - accepting jbvBinary as is in pushJsonbValue seems
like a
bug to me.
Isn't that for the benefit of raw scalar pseudo arrays? The existing
comments above pushJsonbValue() acknowledge such callers.
Umm, no, the raw scalar pseudo arrays are of type jbvArray, not
jbvBinary. And they are pushed with WJB_BEGIN_ARRAY, not with WJB_ELEM
or WJB_VALUE, as the comment notes. See this fragment of code from
JsonbValueToJsonb:
scalarArray.type = jbvArray;
scalarArray.val.array.rawScalar = true;
scalarArray.val.array.nElems = 1;
pushJsonbValue(&pstate, WJB_BEGIN_ARRAY, &scalarArray);
I tested this by removing the assert test for jbvBinary in the
WJB_ELEM and WJB_VALUE switch beranches and then running the
regression suite. No assertion failure was triggered. While that's not
guaranteed to be a perfect test, it doesn't seem like a bad one. Can
you pose a counter example where this will break?
Why are you passing the skipNested variable to JsonbIteratorNext()
within jsonb_delete()? I'm not seeing a need for that.
If you don't the logic gets more complex, as you need to keep track of
what level of the object you are at. The virtue of this change is that
it will simplify a lot of such processing by removing the unnecessary
restriction on passing jbvBinary values to pushJsonbValue().
If you have a better fix for the bug you complained about I'll be
happy to take a look.
OK, here's a slightly updated version of the patch. The current
behaviour in which pushJsonbValue() happily pushes a jbvBinary value,
only to have it fail later when we try to get a Jsonb out of it, seems
like a plain bug. The fact that none of our present 9.4 code exercises
that bug is lucky, but since this is an exposed API, and has been the
cause of past complaint, I propose, in the absence of further objections
or comments, to apply it to both master and 9.4, once the new releases
are out of the way.
cheers
andrew
diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index 5cd9140..974e386 100644
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -57,6 +57,9 @@ static void appendElement(JsonbParseState *pstate, JsonbValue *scalarVal);
static int lengthCompareJsonbStringValue(const void *a, const void *b);
static int lengthCompareJsonbPair(const void *a, const void *b, void *arg);
static void uniqueifyJsonbObject(JsonbValue *object);
+static JsonbValue *pushJsonbValueScalar(JsonbParseState **pstate,
+ JsonbIteratorToken seq,
+ JsonbValue *scalarVal);
/*
* Turn an in-memory JsonbValue into a Jsonb for on-disk storage.
@@ -503,10 +506,43 @@ fillJsonbValue(JsonbContainer *container, int index,
*
* Only sequential tokens pertaining to non-container types should pass a
* JsonbValue. There is one exception -- WJB_BEGIN_ARRAY callers may pass a
- * "raw scalar" pseudo array to append that.
+ * "raw scalar" pseudo array to append it - the actual scalar should be passed
+ * next and it will be added as the only member of the array.
+ *
+ * Values of type jvbBinary, which are rolled up arrays and objects,
+ * are unpacked before being added to the result.
*/
JsonbValue *
pushJsonbValue(JsonbParseState **pstate, JsonbIteratorToken seq,
+ JsonbValue *jbval)
+{
+ JsonbIterator *it;
+ JsonbValue *res = NULL;
+ JsonbValue v;
+ JsonbIteratorToken tok;
+
+ if (!jbval || (seq != WJB_ELEM && seq != WJB_VALUE) ||
+ jbval->type != jbvBinary)
+ {
+ /* drop through */
+ return pushJsonbValueScalar(pstate, seq, jbval);
+ }
+
+ /* unpack the binary and add each piece to the pstate */
+ it = JsonbIteratorInit(jbval->val.binary.data);
+ while ((tok = JsonbIteratorNext(&it, &v, false)) != WJB_DONE)
+ res = pushJsonbValueScalar(pstate, tok,
+ tok < WJB_BEGIN_ARRAY ? &v : NULL);
+
+ return res;
+}
+
+/*
+ * Do the actual pushing, with only scalar or pseudo-scalar-array values
+ * accepted.
+ */
+static JsonbValue *
+pushJsonbValueScalar(JsonbParseState **pstate, JsonbIteratorToken seq,
JsonbValue *scalarVal)
{
JsonbValue *result = NULL;
@@ -549,13 +585,11 @@ pushJsonbValue(JsonbParseState **pstate, JsonbIteratorToken seq,
appendKey(*pstate, scalarVal);
break;
case WJB_VALUE:
- Assert(IsAJsonbScalar(scalarVal) ||
- scalarVal->type == jbvBinary);
+ Assert(IsAJsonbScalar(scalarVal));
appendValue(*pstate, scalarVal);
break;
case WJB_ELEM:
- Assert(IsAJsonbScalar(scalarVal) ||
- scalarVal->type == jbvBinary);
+ Assert(IsAJsonbScalar(scalarVal));
appendElement(*pstate, scalarVal);
break;
case WJB_END_OBJECT:
diff --git a/src/include/utils/jsonb.h b/src/include/utils/jsonb.h
index 7b56175..b02934a 100644
--- a/src/include/utils/jsonb.h
+++ b/src/include/utils/jsonb.h
@@ -418,7 +418,7 @@ extern JsonbValue *findJsonbValueFromContainer(JsonbContainer *sheader,
extern JsonbValue *getIthJsonbValueFromContainer(JsonbContainer *sheader,
uint32 i);
extern JsonbValue *pushJsonbValue(JsonbParseState **pstate,
- JsonbIteratorToken seq, JsonbValue *scalarVal);
+ JsonbIteratorToken seq, JsonbValue *jbVal);
extern JsonbIterator *JsonbIteratorInit(JsonbContainer *container);
extern JsonbIteratorToken JsonbIteratorNext(JsonbIterator **it, JsonbValue *val,
bool skipNested);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
