Re: mogrify and indent features for jsonb

Hi,

I looked at the patch and have several comments.

First let me say that modifying the individual paths of the json is the feature I miss the most in the current implementation so I am happy that this patch was submitted.

I would prefer slightly the patch split into two parts, one for the indent printing and one with the manipulation functions, but it's not too big patch so it's not too bad as it is.

There is one compiler warning that I see:
jsonfuncs.c:3533:1: warning: no previous prototype for ‘jsonb_delete_path’ [-Wmissing-prototypes]
 jsonb_delete_path(PG_FUNCTION_ARGS)

I think it would be better if the ident printing didn't put the start of array ([) and start of dictionary ({) on separate line since most "pretty" printing implementations outside of Postgres (like the JSON.stringify or python's json module) don't do that. This is purely about consistency with the rest of the world.

The json_ident might be better named as json_pretty perhaps?

I don't really understand the point of h_atoi() function. What's wrong with using strtol like pg_atoi does? Also there is no integer overflow check anywhere in that function.

There is tons of end of line whitespace mess in jsonb_indent docs.

Otherwise everything I tried so far works as expected. The code looks ok as well except maybe the replacePath could use couple of comments (for example the line which uses the h_atoi) to make it easier to follow.

About the:

+       /* XXX : why do we need this assertion? The functions is declared to take text[] */
+       Assert(ARR_ELEMTYPE(path) == TEXTOID);

I wonder about this also, some functions do that, some don't, I am not really sure what is the rule there myself.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers