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). 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers