Обсуждение: [PATH] Jsonb, insert a new value into an array at arbitrary position

On 18/02/16 15:38, Dmitry Dolgov wrote:
> Hi
>
> As far as I see there is one basic update function for jsonb, that can't be
> covered by `jsonb_set` - insert a new value into an array at arbitrary
> position.
> Using `jsonb_set` function we can only append into array at the end/at the
> beginning, and it looks more like a hack:
>
> ```
> =# select jsonb_set('{"a": [1, 2, 3]}', '{a, 100}', '4');
>        jsonb_set
> ---------------------
>   {"a": [1, 2, 3, 4]}
> (1 row)
>
>
> =# select jsonb_set('{"a": [1, 2, 3]}', '{a, -100}', '4');
>        jsonb_set
> ---------------------
>   {"a": [4, 1, 2, 3]}
> (1 row)
> ```
>
> I think the possibility to insert into arbitrary position will be quite
> useful,
> something like `json_array_insert` in MySql:
>
> ```
> mysql> set @j = '["a", {"b": [1, 2]}, [3, 4]]';
> mysql> select json_array_insert(@j, '$[1].b[0]', 'x');
>
>   json_array_insert(@j, '$[1].b[0]', 'x')
> +-----------------------------------------+
>   ["a", {"b": ["x", 1, 2]}, [3, 4]]
> ```
>
> It can look like `jsonb_insert` function in our case:
>
> ```
> =# select jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"');
>           jsonb_insert
> -------------------------------
>   {"a": [0, "new_value", 1, 2]}
> (1 row)
> ```
>

I think it makes sense to have interface like this, I'd strongly prefer 
the jsonb_array_insert naming though.


> I attached possible implementation, which is basically quite small (all
> logic-related
> modifications is only about 4 lines in `setPath` function). This
> implementation
> assumes a flag to separate "insert before"/"insert after" operations, and an
> alias to `jsonb_set` in case if we working with a jsonb object, not an
> array.
>

I don't think it's a good idea to use set when this is used on object, I 
think that we should throw error in that case.

Also this patch needs documentation.

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

On 29/02/16 18:19, Dmitry Dolgov wrote:
>  > I'd strongly prefer the jsonb_array_insert naming though
>  > I don't think it's a good idea to use set when this is used on
> object, I think that we should throw error in that case.
>
> Well, I thought it's wrong to introduce different functions and
> behaviour patterns for objects and arrays, because it's the one data
> type after all. But it's just my opinion of course.
>

The problem I see with that logic is because we don't keep order of keys 
in the jsonb object the "insert" in the name will have misleading 
implications from the point of the user. Also it does not do anything 
for object that "set" doesn't do already, so why have two interfaces for 
doing same thing.

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

On 2016-02-29 17:19+00, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
On 2016-02-24 19:37+00, Petr Jelinek <petr@2ndquadrant.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

Hi Dmitry,

On 3/7/16 8:50 PM, Vitaly Burovoy wrote:

> On 2016-02-29 17:19+00, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> 
> Hello! The first pass of a review is below.

It's be over a week since Vitaly posted this review.  Please respond
and/or provide a new patch as soon as you can.

Thanks,
-- 
-David
david@pgmasters.net

I still don't like that this works on path leading to an object given 
that we can't fulfill the promise of inserting to an arbitrary position 
there.

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

<div dir="ltr">> <span style="font-size:12.8px">I still don't like that this works on path leading to an object
giventhat we can't fulfill the promise of inserting to an arbitrary position there.<br /><br />I'm thinking about this
followingway - `jsonb_insert` is just a function to insert a new value, which has specific options to enforce arbitrary
positionin case of jsonb array. I don't think this can confuse someone since it's not something like
`jsonb_insert_at_position`function. But of course, if I'm wrong we can easy change the function name and make it
availableonly for arrays.</span></div> 

On 2016-03-16, Dmitry Dolgov <9erthalion6@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

On 3/25/16, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> Here is a new version of path, I hope I didn't miss anything. Few notes:
>
>> 4.
>> 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)
>
> Good idea, thanks.

You're welcome.
===

I'm sorry for the late letter.

Unfortunately, it seems one more round is necessary.
The documentation changes still has to be fixed.

The main description points to a "target section designated by path is
a JSONB array". It is a copy-paste from jsonb_set, but here it has
wrong context.
The main idea in jsonb_set is replacing any object which is pointed
(designated) by "path" no matter where (array or object) it is
located.
But in the phrase for jsonb_insert above you want to point to an upper
object _where_ "section designated by path" is located.

I'd write something like "If target section designated by path is _IN_
a JSONB array, ...". The same thing with "a JSONB object".

Also I'd rewrite "will act like" as "behaves as".

Last time I missed the argument's name "insert_before_after". It is
better to replace it as just "insert_after". Also reflect that name in
the documentation properly.

To be consistent change the constant "JB_PATH_NOOP" as "0x0000"
instead of just "0x0".

Also the variable's name "bool before" is incorrect because it
contradicts with the SQL function's argument "after" (from the
documentation) or "insert_after" (proposed above) or tests (de-facto
behavior). Moreover it seems the logic in the code is correct, so I
have no idea why "before ? JB_PATH_INSERT_BEFORE :
JB_PATH_INSERT_AFTER" works.
I think either proper comment should be added or lack in the code must be found.
Anyway the variable's name must reflect the SQL argument's name.

-- 
Best regards,
Vitaly Burovoy

On 3/30/16, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> On 31 March 2016 at 05:04, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:
>> The documentation changes still has to be fixed.
> Thanks for help. Looks like I'm not so good at text formulation. Fixed.
Never mind. I'm also not so good at it. It is still should be
rewritten by a native English speaker, but it is better to give him
good source for it.

===
I'm almost ready to mark it as "Ready for committer".

Few notes again.
1.
> a current item was pushed to parse state after JB_PATH_INSERT_BEFORE

I paid attention that the function's name 'pushJsonbValue' looks as
working with stack (push/pop), but there is no function "pop*" neither
in jsonfuncs.c nor jsonb_util.c.
That's why I wrote "it seems the logic in the code is correct" - it is
logical to insert new value if "before", then current value, then new
value if "after".
But yes, following by executing path to the "pushState" function
anyone understands "JsonbParseState" is constructed as a stack, not a
queue. So additional comment is needed why "JB_PATH_INSERT_AFTER"
check is placed before inserting curent value and
JB_PATH_INSERT_BEFORE check.

The comment can be located just before "if (op_type &
JB_PATH_INSERT_AFTER)" (line 3986) and look similar to:

/** JsonbParseState struct behaves as a stack -- see the "pushState" function,* so check for
JB_PATH_INSERT_BEFORE/JB_PATH_INSERT_AFTERin a reverse order.*/
 

2.
A comment for the function "setPath" is obsolete: "If newval is null"
check and "If create is true" name do not longer exist.
Since I answered so late last time I propose the next formulating to
avoid one (possible) round:

/** Do most of the heavy work for jsonb_set/jsonb_insert** If JB_PATH_DELETE bit is set in op_type, the element is to
beremoved.** If any bit mentioned in JB_PATH_CREATE_OR_INSERT is set in op_type,* we create the new value if the key or
arrayindex does not exist.** Bits JB_PATH_INSERT_BEFORE and JB_PATH_INSERT_AFTER in op_type* behave as JB_PATH_CREATE
ifnew value is inserted in JsonbObject.** All path elements before the last must already exist* whatever bits in
op_typeare set, or nothing is done.*/
 

===
I hope that notes are final (additional check in formulating is appreciated).

P.S.: if it is not hard for you, please rebase the patch to the
current master to avoid offset in func.sgml.

-- 
Best regards,
Vitaly Burovoy

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            tested, passed

I have reviewed this patch.
It applies cleanly at the top of current master (3501f71), compiles silently and implements declared feature.

The documentation describes behavior of the function with pointing to a difference between inserting into JsonbObject
andJsonbArray.
 

The code is clean and commented. Linked comment is changed too.

Regression tests cover possible use cases and edge cases.


Notes for a committer:
1. pg_proc.h has changed, so the CATALOG_VERSION_NO must also be changed.
2. Code comments and changes in the documentation need proof-reading by a native
English speaker, which the Author and I are not.


The new status of this patch is: Ready for Committer

On 03/31/2016 09:00 AM, Dmitry Dolgov wrote:
> On 31 March 2016 at 17:31, Vitaly Burovoy <vitaly.burovoy@gmail.com 
> <mailto:vitaly.burovoy@gmail.com>> wrote:
>
>     it is logical to insert new value if "before", then current value,
>     then new
>     value if "after".
>
>
> Oh, I see now. There is a slightly different logic: `v` is a current 
> value and `newval` is a new value.
> So basically we insert a current item in case of "after", then a new 
> value (if it's not a delete operation),
> then a current item in case of "before". But I agree, this code can be 
> more straightforward. I've attached
> a new version, pls take a look (it contains the same logic that you've 
> mentioned).



I haven't been following this thread due to pressure of time, so my 
apologies in advance if these comments have already been covered.

I've been asked to look at and comment on the SQL API of the feature. I 
think it's basically sound, although there is one thing that's not clear 
from the regression tests: what happens if we're inserting into an 
object and the key already exists? e.g.:

select jsonb_insert('{"a": {"b": "value"}}', '{a, b}', '"new_value"');

I think this should be forbidden, i.e. the function shouldn't ever 
overwrite an existing value. If that's not handled it should be, and 
either way there should be a regression test for it.

cheers

andrew

> I've been asked to look at and comment on the SQL API of the feature. I think
> it's basically sound, although there is one thing that's not clear from the
> regression tests: what happens if we're inserting into an object and the key
> already exists? e.g.:
>
> select jsonb_insert('{"a": {"b": "value"}}', '{a, b}', '"new_value"');
>
> I think this should be forbidden, i.e. the function shouldn't ever overwrite an
> existing value. If that's not handled it should be, and either way there should
> be a regression test for it.

I'm agree about covering this case by tests, but I think it should be allowed.
In this case it will work exactly as jsbonb_set
-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
  WWW: http://www.sigaev.ru/
 

Andrew Dunstan wrote:

> I haven't been following this thread due to pressure of time, so my
> apologies in advance if these comments have already been covered.
> 
> I've been asked to look at and comment on the SQL API of the feature. I
> think it's basically sound, although there is one thing that's not clear
> from the regression tests: what happens if we're inserting into an object
> and the key already exists? e.g.:
> 
> select jsonb_insert('{"a": {"b": "value"}}', '{a, b}', '"new_value"');

It has been covered: Petr said, and I agree with him, that this new
interface is for arrays, not objects, and so the above example should be
rejected altogether.  For objects we already have jsonb_set(), so what
is the point of having this work there?  It feels too much like
TIMTOWTDI, which isn't a principle we adhere to.

I think it'd be much more sensible if we were just to make this function
work on arrays only.  There, the solution to Andrew's question is
trivial: if you insert a value in a position that's already occupied,
the elements to its right are "shifted" one position upwards in the
array (this is why this is an "insert" operation and not "replace" or
"set").

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 04/05/2016 12:42 PM, Teodor Sigaev wrote:
>> I've been asked to look at and comment on the SQL API of the feature. 
>> I think
>> it's basically sound, although there is one thing that's not clear 
>> from the
>> regression tests: what happens if we're inserting into an object and 
>> the key
>> already exists? e.g.:
>>
>> select jsonb_insert('{"a": {"b": "value"}}', '{a, b}', '"new_value"');
>>
>> I think this should be forbidden, i.e. the function shouldn't ever 
>> overwrite an
>> existing value. If that's not handled it should be, and either way 
>> there should
>> be a regression test for it.
>
> I'm agree about covering this case by tests, but I think it should be 
> allowed.
> In this case it will work exactly as jsbonb_set


It seems to me a violation of POLA to allow something called "insert" to 
do a "replace" instead.

cheers

andre

Andrew Dunstan <andrew@dunslane.net> writes:
> On 04/05/2016 12:42 PM, Teodor Sigaev wrote:
>> I'm agree about covering this case by tests, but I think it should be 
>> allowed.
>> In this case it will work exactly as jsbonb_set

> It seems to me a violation of POLA to allow something called "insert" to 
> do a "replace" instead.

I agree with Andrew's point here.  If the target is an array it would
never replace an entry, so why would it do so for an object?

I think there is potentially some use-case for insert-only semantics
for an object target, if you want to be sure you're not overwriting
data.  So I think "throw an error on duplicate key" is marginally better
than "reject object target altogether".  But I could live with either.
        regards, tom lane

On 04/05/2016 03:08 PM, Tom Lane wrote:
>
> I think there is potentially some use-case for insert-only semantics
> for an object target, if you want to be sure you're not overwriting
> data.  So I think "throw an error on duplicate key" is marginally better
> than "reject object target altogether".  But I could live with either.



Yeah, keeping it but rejecting update of an existing key is my 
preference too.

cheers

andrew

On 06/04/16 06:13, Dmitry Dolgov wrote:
>
> On 6 April 2016 at 03:29, Andrew Dunstan <andrew@dunslane.net
> <mailto:andrew@dunslane.net>> wrote:
>
>
>     Yeah, keeping it but rejecting update of an existing key is my
>     preference too.
>
>     cheers
>
>     andrew
>
>
> Yes, it sounds quite reasonable. Here is a new version of patch (it will
> throw
> an error for an existing key). Is it better now?

This seems like reasonable compromise to me. I wonder if the errcode 
should be ERRCODE_INVALID_PARAMETER_VALUE but don't feel too strongly 
about that.

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

Thank you, pushed with Petr's notice

Dmitry Dolgov wrote:
>
> On 6 April 2016 at 03:29, Andrew Dunstan <andrew@dunslane.net
> <mailto:andrew@dunslane.net>> wrote:
>
>
>     Yeah, keeping it but rejecting update of an existing key is my preference too.
>
>     cheers
>
>     andrew
>
>
> Yes, it sounds quite reasonable. Here is a new version of patch (it will throw
> an error for an existing key). Is it better now?
>
>
>

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
  WWW: http://www.sigaev.ru/