On 2016-03-16, Dmitry Dolgov <9erthali...@gmail.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).

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

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.

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:

or even create a new constant (there can be better name for it):

and use it like:

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

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

> 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)

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.

Please, return the "skip" comment.

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

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

Reply via email to