Re: jsonb and nested hstore - Mailing list pgsql-hackers

On 2014-02-26 16:23:12 -0500, Andrew Dunstan wrote:
> On 02/10/2014 09:11 PM, Andres Freund wrote:

> >Is it just me or is jsonapi.h not very well documented?
> 
> 
> What about it do you think is missing? In any case, it's hardly relevant to
> this patch, so I'll take that as obiter dicta.

It's relevant insofer because I tried to understand it, to understand
whether this patch's usage is sensible.

O n a quick reread of the header, what I am missing is:
* what's semstate in JsonSemAction? Private data?
* what's object_start and object_field_start? Presumably object vs keypair? Why not use element as ifor the array?
* scalar_action is called for which types of tokens?
* what's exactly the meaning of the isnull parameter for ofield_action and aelem_action?
* How is one supposed to actually access data in the callbacks, not obvious for all the callbacks.
* are scalar callbacks triggered for object keys, object/array values?
...

> >>+static void
> >>+putEscapedValue(StringInfo out, JsonbValue *v)
> >>+{
> >>+    switch (v->type)
> >>+    {
> >>+        case jbvNull:
> >>+            appendBinaryStringInfo(out, "null", 4);
> >>+            break;
> >>+        case jbvString:
> >>+            escape_json(out, pnstrdup(v->string.val, v->string.len));
> >>+            break;
> >>+        case jbvBool:
> >>+            if (v->boolean)
> >>+                appendBinaryStringInfo(out, "true", 4);
> >>+            else
> >>+                appendBinaryStringInfo(out, "false", 5);
> >>+            break;
> >>+        case jbvNumeric:
> >>+            appendStringInfoString(out, DatumGetCString(DirectFunctionCall1(numeric_out,
PointerGetDatum(v->numeric))));
> >>+            break;
> >>+        default:
> >>+            elog(ERROR, "unknown jsonb scalar type");
> >>+    }
> >>+}
> >Hm, will the jbvNumeric always result in correct correct quoting?
> >datum_to_json() does extra hangups for that case, any reason we don't
> >need that here?

> Yes, there is a reason we don't need it here. datum_to_json is converting
> SQL numerics to json, and these might be strings such as 'Nan'. But we never
> store something in a jsonb numeric field unless it came in as a json numeric
> format, which never needs quoting. The json parser will never parse 'NaN' as
> a numeric value.

Ah, yuck. Makes sense. Not your fault at all, but I do dislike json's
definition of numeric values.

> >>+char *
> >>+JsonbToCString(StringInfo out, char *in, int estimated_len)
> >>+{
> >...
> >>+    while (redo_switch || ((type = JsonbIteratorGet(&it, &v, false)) != 0))
> >>+    {
> >>+        redo_switch = false;
> >Not sure if I see the advantage over the goto here. A comment explaining
> >what the reason for the goto is wouldhave sufficed.
> 
> I think you're being pretty damn picky here. You whined about the goto, I
> removed it, now you don't like that either. Personally I think this is
> cleaner.

Sorry, should perhaps have been a bit more precise in my disagreement
abou the goto version. I didn't dislike the goto itself, but that it was
a undocumented and unobvious change in control flow.

It's the reviewers job to be picky, I pretty damn sure don't expect you
to agree with all my points and I am perfectly fine if you disregard
several of them. I've just read through the patch quickly, so it's not
surprising if I misidentify some.


> >
> >>+            case WJB_KEY:
> >>+                if (first == false)
> >>+                    appendBinaryStringInfo(out, ", ", 2);
> >>+                first = true;
> >>+
> >>+                putEscapedValue(out, &v);
> >>+                appendBinaryStringInfo(out, ": ", 2);
> >putEscapedValue doesn't gurantee only strings are output, but
> >datum_to_json does extra hangups for that case.
> 
> But the key here will always be a string. It's enforced by the JSON rules. I
> suppose we could call escape_json directly here and save a function call,
> but I don't agree that there is any problem here.

Ah, yes, it will already have been converted to a string during the
initial conversion, right. /* json rules guarantee this is a string */
or something?

> >
> >>+                type = JsonbIteratorGet(&it, &v, false);
> >>+                if (type == WJB_VALUE)
> >>+                {
> >>+                    first = false;
> >>+                    putEscapedValue(out, &v);
> >>+                }
> >>+                else
> >>+                {
> >>+                    Assert(type == WJB_BEGIN_OBJECT || type == WJB_BEGIN_ARRAY);
> >>+                    /*
> >>+                     * We need to rerun current switch() due to put
> >>+                     * in current place object which we just got
> >>+                     * from iterator.
> >>+                     */
> >"due to put"?
> 
> 
> I think that's due to the author not being a native English speaker. I've
> tried to improve it a bit.

Oh, I perfectly understand that problem, believe me... I make many of
those myself, and I often don't see them in my own patches without them
being pointed out...
> >>+    if (va->string.len == vb->string.len)
> >>+    {
> >>+        res = memcmp(va->string.val, vb->string.val, va->string.len);
> >>+        if (res == 0 && arg)
> >>+            *(bool *) arg = true;
> >Should be NULL, not 0.
> 
> No, the compiler doesn't like that for int values.

Yes, please disregard, I misread. I think I wanted actually to say that
the test for arg should be arg != NULL, because we don't usually do
pointer truth tests (which I personally find odd, but well).


Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services