On Wed, May 21, 2014 at 4:53 AM, Heikki Linnakangas <hlinnakan...@vmware.com> 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 (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers