Re: [PATH] Jsonb, insert a new value into an array at arbitrary position

On 2016-02-29 17:19+00, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
On 2016-02-24 19:37+00, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>> Also this patch needs documentation.
> I've added new version in attachments, thanks.

Hello! The first pass of a review is below.

1. Rename the "flag" variable to something more meaningful. (e.g.
"op_type" - "operation_type")

2. There is two extra spaces after the "_DELETE" word
+#define JB_PATH_DELETE 0x0002

3.
res = setPath(&it, path_elems, path_nulls, path_len, &st,
- 0, newval, create);
+ 0, newval, create ? JB_PATH_CREATE : 0x0);

What is the magic constant "0x0"? If not "create", then what?
Introduce something like JB_PATH_NOOP = 0x0...

4. In the "setPathArray" the block:
if (newval != NULL)
"newval == NULL" is considered as "to be deleted" from the previous
convention and from the comments for the "setPath" function.
I think since you've introduced special constants one of which is
JB_PATH_DELETE (which is not used now) you should replace convention
from checking for a value to checking for a constant:
if (flag != JB_PATH_DELETE)
or even better:
if (!flag & JB_PATH_DELETE)

5. Change checking for equality (to bit constants) to bit operations:
(flag == JB_PATH_INSERT_BEFORE || flag == JB_PATH_INSERT_AFTER)
which can be replaced to:
(flag & (JB_PATH_INSERT_BEFORE | JB_PATH_INSERT_AFTER))

also here:
(flag == JB_PATH_CREATE || flag == JB_PATH_INSERT_BEFORE || flag
== JB_PATH_INSERT_AFTER))
can be:
(flag & (JB_PATH_CREATE | JB_PATH_INSERT_BEFORE | JB_PATH_INSERT_AFTER))

6. Pay attention to parenthesises to make the code looks like similar
one around:
+ if ((npairs == 0) && flag == JB_PATH_CREATE && (level == path_len - 1))
should be:
+ if ((npairs == 0) && (flag == JB_PATH_CREATE) && (level == path_len - 1))

7. Why did you remove "skip"? It is a comment what "true" means...
- r = JsonbIteratorNext(it, &v, true); /* skip */
+ r = JsonbIteratorNext(it, &v, true);

8. After your changes some statements exceed 80-column threshold...
The same rules for the documentation.

9. And finally... it does not work as expected in case of:
postgres=# select jsonb_insert('{"a":[0,1,2,3]}', '{"a", 10}', '"4"');
jsonb_insert
---------------------
{"a": [0, 1, 2, 3]}
(1 row)

wheras jsonb_set works:
postgres=# select jsonb_set('{"a":[0,1,2,3]}', '{"a", 10}', '"4"');
jsonb_set
--------------------------
{"a": [0, 1, 2, 3, "4"]}
(1 row)

--
Best regards,
Vitaly Burovoy