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

On 2016-03-16, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
> Hi Vitaly, thanks for the review. I've attached a new version of path with
> improvements. Few notes:
>
>> 7. Why did you remove "skip"? It is a comment what "true" means...
>
> Actually, I thought that this comment was about skipping an element from
> jsonb in order to change/delete it,

As I got it, it is "skipNested", skipping iterating over nested
containers, not skipping an element.

> not about the last argument. E.g. you can find several occurrences of
> `JsonbIteratorNext` with `true` as the last
> argument but without a "last argument is about skip" comment.

I think it is not a reason for deleting this comment. ;-)

> And there is a piece of code in the function `jsonb_delete` with a "skip
> element" commentary:
>
> ```
> /* skip corresponding value as well */
> if (r == WJB_KEY)
> JsonbIteratorNext(&it, &v, true);
> ```

The comment in your example is for the extra "JsonbIteratorNext" call
(the first one is in a "while" statement outside your example, but
over it in the code), not for the "skip" parameter in the
"JsonbIteratorNext" call here.
===

Notes for the jsonb_insert_v3.patch applied on the f6bd0da:

1a. Please, rebase your patch to the current master (it is easy to
resolve conflict, but still necessary).

1b. Now OID=3318 is "pg_stat_get_progress_info" (Hint: 3324 is free now).

2.
The documentation, func.sgml:
> If<replaceable>target</replaceable>
Here must be a space after the "If" word.

3.
There is a little odd formulating me in the documentation part
(without formatting for better readability):
> If target section designated by path is a JSONB array, new_value will be inserted before it, or after if after is true (defailt is false).

a. "new_value" is not inserted before "it" (a JSONB array), it is
inserted before target;
b. s/or after if/or after target if/;
c. s/defailt/default/

> If ... is a JSONB object, new_value will be added just like a regular key.

d. "new_value" is not a regular key: key is the final part of the "target";
e. "new_value" replaces target if it exists;
f. "new_value" is added if target is not exists as if jsonb_set is
called with create_missing=true.
g. "will" is about possibility. "be" is about an action.

4.
function setPathObject, block with "op_type = JB_PATH_CREATE"
(jsonfuncs.c, lines 3833..3840).
It seems it is not necessary now since you can easily check for all
three options like:
op_type & (JB_PATH_INSERT_BEFORE | JB_PATH_INSERT_AFTER | JB_PATH_CREATE)

or even create a new constant (there can be better name for it):
#define JB_PATH_CREATE_OR_INSERT (JB_PATH_INSERT_BEFORE |
JB_PATH_INSERT_AFTER | JB_PATH_CREATE)

and use it like:
op_type & JB_PATH_CREATE_OR_INSERT

all occurrences of JB_PATH_CREATE in the function are already with the
"(level == path_len - 1)" condition, there is no additional check
needed.

also the new constant JB_PATH_CREATE_OR_INSERT can be used at lines 3969, 4025.

5.
> if (op_type != JB_PATH_DELETE)
It seems strange direct comparison among bitwise operators (lines 3987..3994)

You can leave it as is, but I'd change it (and similar one at the line
3868) to a bitwise operator:
if (!op_type & JB_PATH_DELETE)

6.
I'd like to see a test with big negative index as a reverse for big
positive index:
select jsonb_insert('{"a": [0,1,2]}', '{a, -10}', '"new_value"');

I know now it works as expected, it is just as a defense against
further changes that can be destructive for this special case.

7.
Please, return the "skip" comment.

8.
The documentation: add "jsonb_insert" to the note about importance of
existing intermediate keys. Try to reword it since the function
doesn't have a "create_missing" parameter support.
> All the items of the path parameter of jsonb_set must be present in the target,
> ... in which case all but the last item must be present.

Currently I can't break the code, so I think it is close to the final state. ;-)

--
Best regards,
Vitaly Burovoy