On Wed, May 21, 2014 at 4:53 AM, Heikki Linnakangas
<[email protected]> wrote:
> Hmm. The patch looks correct as far as it goes. But that function is still a
> bit funny. When it compares two identical arrays (or objects), and reaches
> the WJB_END_ARRAY token, it will still fall into the code that checks what
> the va and vb types are, and compares the last scalar values in that array
> again. That's wrong, and will fail if the compiler decides to clobber the
> local "va" or "vb" variables between iterations of the do-while loop,
> because JsonbIteratorNext() does not set the value when returning
> WJB_END_ARRAY.
That's an obsolete assumption that once actually applied during development.
Attached revision also adds handling for that case.
Thanks
--
Peter Geoghegan
*** a/src/backend/utils/adt/jsonb_util.c
--- b/src/backend/utils/adt/jsonb_util.c
*************** static void uniqueifyJsonbObject(JsonbVa
*** 62,74 ****
*
* There isn't a JsonbToJsonbValue(), because generally we find it more
* convenient to directly iterate through the Jsonb representation and only
! * really convert nested scalar values. formIterIsContainer() does this, so
! * that clients of the iteration code don't have to directly deal with the
! * binary representation (JsonbDeepContains() is a notable exception, although
! * all exceptions are internal to this module). In general, functions that
! * accept a JsonbValue argument are concerned with the manipulation of scalar
! * values, or simple containers of scalar values, where it would be
! * inconvenient to deal with a great amount of other state.
*/
Jsonb *
JsonbValueToJsonb(JsonbValue *val)
--- 62,74 ----
*
* There isn't a JsonbToJsonbValue(), because generally we find it more
* convenient to directly iterate through the Jsonb representation and only
! * really convert nested scalar values. JsonbIteratorNext() does this, so that
! * clients of the iteration code don't have to directly deal with the binary
! * representation (JsonbDeepContains() is a notable exception, although all
! * exceptions are internal to this module). In general, functions that accept
! * a JsonbValue argument are concerned with the manipulation of scalar values,
! * or simple containers of scalar values, where it would be inconvenient to
! * deal with a great amount of other state.
*/
Jsonb *
JsonbValueToJsonb(JsonbValue *val)
*************** compareJsonbContainers(JsonbContainer *a
*** 137,149 ****
ra = JsonbIteratorNext(&ita, &va, false);
rb = JsonbIteratorNext(&itb, &vb, false);
- /*
- * To a limited extent we'll redundantly iterate over an array/object
- * while re-performing the same test without any reasonable
- * expectation of the same container types having differing lengths
- * (as when we process a WJB_BEGIN_OBJECT, and later the corresponding
- * WJB_END_OBJECT), but no matter.
- */
if (ra == rb)
{
if (ra == WJB_DONE)
--- 137,142 ----
*************** compareJsonbContainers(JsonbContainer *a
*** 152,157 ****
--- 145,162 ----
break;
}
+ if (ra == WJB_END_ARRAY || ra == WJB_END_OBJECT)
+ {
+ /*
+ * There is no array or object to safely compare at this stage
+ * of processing. jbvArray/jbvObject values are only
+ * considered initially, during processing of WJB_BEGIN_ARRAY
+ * and WJB_BEGIN_OBJECT tokens. This doesn't matter, because a
+ * second comparison would be redundant.
+ */
+ continue;
+ }
+
if (va.type == vb.type)
{
switch (va.type)
*************** compareJsonbContainers(JsonbContainer *a
*** 194,212 ****
else
{
/*
! * It's safe to assume that the types differed.
*
* If the two values were the same container type, then there'd
* have been a chance to observe the variation in the number of
! * elements/pairs (when processing WJB_BEGIN_OBJECT, say). They
! * can't be scalar types either, because then they'd have to be
! * contained in containers already ruled unequal due to differing
! * numbers of pairs/elements, or already directly ruled unequal
! * with a call to the underlying type's comparator.
*/
Assert(va.type != vb.type);
! Assert(va.type == jbvArray || va.type == jbvObject);
! Assert(vb.type == jbvArray || vb.type == jbvObject);
/* Type-defined order */
res = (va.type > vb.type) ? 1 : -1;
}
--- 199,227 ----
else
{
/*
! * It's safe to assume that the types differed, and that the va and
! * vb values passed were set.
*
+ * We don't have to consider the WJB_END_ARRAY and WJB_END_OBJECT
+ * cases here, because in order to process those tokens we'd first
+ * have to process WJB_BEGIN_ARRAY or WJB_BEGIN_OBJECT, and that
+ * would be sufficient reason to end up here (with reliable values
+ * set for va and vb), which always results in finishing the
+ * comparison.
+ */
+ Assert(ra != WJB_END_ARRAY && ra != WJB_END_OBJECT);
+ Assert(rb != WJB_END_ARRAY && rb != WJB_END_OBJECT);
+
+ /*
* If the two values were the same container type, then there'd
* have been a chance to observe the variation in the number of
! * elements/pairs (when processing WJB_BEGIN_OBJECT, say). They're
! * either two heterogeneously-typed containers, or a container and
! * some scalar type.
*/
Assert(va.type != vb.type);
! Assert(va.type != jbvBinary);
! Assert(vb.type != jbvBinary);
/* Type-defined order */
res = (va.type > vb.type) ? 1 : -1;
}
*************** JsonbIteratorInit(JsonbContainer *contai
*** 630,636 ****
* It is our job to expand the jbvBinary representation without bothering them
* with it. However, clients should not take it upon themselves to touch array
* or Object element/pair buffers, since their element/pair pointers are
! * garbage.
*/
JsonbIteratorToken
JsonbIteratorNext(JsonbIterator **it, JsonbValue *val, bool skipNested)
--- 645,653 ----
* It is our job to expand the jbvBinary representation without bothering them
* with it. However, clients should not take it upon themselves to touch array
* or Object element/pair buffers, since their element/pair pointers are
! * garbage. Also, *val will not be set when returning WJB_END_ARRAY or
! * WJB_END_OBJECT, on the assumption that it's only useful to access values
! * when recursing in.
*/
JsonbIteratorToken
JsonbIteratorNext(JsonbIterator **it, JsonbValue *val, bool skipNested)
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
