Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
Thank you, pushed with Petr's notice Dmitry Dolgov wrote: On 6 April 2016 at 03:29, Andrew Dunstan> 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: teo...@sigaev.ru WWW: http://www.sigaev.ru/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
On 06/04/16 06:13, Dmitry Dolgov wrote: On 6 April 2016 at 03:29, Andrew Dunstan> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
On 6 April 2016 at 03:29, Andrew Dunstanwrote: > > 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? diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 1bc9fbc..0d4b3a1 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -10903,6 +10903,9 @@ table2-mapping jsonb_set + jsonb_insert + + jsonb_pretty @@ -11184,6 +11187,39 @@ table2-mapping + + + jsonb_insert(target jsonb, path text[], new_value jsonb, insert_after boolean) + + + jsonb + + Returns target with + new_value inserted. If + target section designated by + path is in a JSONB array, + new_value will be inserted before target or + after if insert_after is true (default is + false). If target section + designated by path is in JSONB object, + new_value will be inserted only if + target does not exist. As with the path + orientated operators, negative integers that appear in + path count from the end of JSON arrays. + + + + jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"') + + + jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"', true) + + + {"a": [0, "new_value", 1, 2]} + {"a": [0, 1, "new_value", 2]} + + + jsonb_pretty(from_json jsonb) text @@ -11235,10 +11271,11 @@ table2-mapping All the items of the path parameter of jsonb_set - must be present in the target, unless - create_missing is true, in which case all but the last item - must be present. If these conditions are not met the target - is returned unchanged. + as well as jsonb_insert except the last item must be present + in the target. If create_missing is false, all + items of the path parameter of jsonb_set must be + present. If these conditions are not met the target is + returned unchanged. If the last path item is an object key, it will be created if it diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 9ae1ef4..a6e661c 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -997,3 +997,11 @@ RETURNS text[] LANGUAGE INTERNAL STRICT IMMUTABLE AS 'parse_ident'; + +CREATE OR REPLACE FUNCTION + jsonb_insert(jsonb_in jsonb, path text[] , replacement jsonb, +insert_after boolean DEFAULT false) +RETURNS jsonb +LANGUAGE INTERNAL +STRICT IMMUTABLE +AS 'jsonb_insert'; diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 97e0e8e..c1b8041 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -33,6 +33,15 @@ #include "utils/memutils.h" #include "utils/typcache.h" +/* Operations available for setPath */ +#define JB_PATH_NOOP 0x +#define JB_PATH_CREATE 0x0001 +#define JB_PATH_DELETE 0x0002 +#define JB_PATH_INSERT_BEFORE 0x0004 +#define JB_PATH_INSERT_AFTER 0x0008 +#define JB_PATH_CREATE_OR_INSERT (JB_PATH_INSERT_BEFORE | JB_PATH_INSERT_AFTER \ + | JB_PATH_CREATE) + /* semantic action functions for json_object_keys */ static void okeys_object_field_start(void *state, char *fname, bool isnull); static void okeys_array_start(void *state); @@ -130,14 +139,14 @@ static JsonbValue *IteratorConcat(JsonbIterator **it1, JsonbIterator **it2, static JsonbValue *setPath(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, int level, Jsonb *newval, - bool create); + int op_type); static void setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, int level, - Jsonb *newval, uint32 npairs, bool create); + Jsonb *newval, uint32 npairs, int op_type); static void setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, - int level, Jsonb *newval, uint32 nelems, bool create); + int level, Jsonb *newval, uint32 nelems, int op_type); static void addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb); /* state for json_object_keys */ @@ -3544,7 +3553,7 @@ jsonb_set(PG_FUNCTION_ARGS) it = JsonbIteratorInit(>root); res = setPath(, path_elems, path_nulls, path_len, , - 0, newval, create); + 0, newval, create ? JB_PATH_CREATE : JB_PATH_NOOP); Assert(res != NULL); @@ -3588,7 +3597,52 @@ jsonb_delete_path(PG_FUNCTION_ARGS) it = JsonbIteratorInit(>root); - res = setPath(, path_elems, path_nulls, path_len, , 0, NULL, false); + res = setPath(,
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
Andrew Dunstanwrites: > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
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: teo...@sigaev.ru WWW: http://www.sigaev.ru/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
On 03/31/2016 09:00 AM, Dmitry Dolgov wrote: On 31 March 2016 at 17:31, Vitaly Burovoy> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
On 31 March 2016 at 17:31, Vitaly Burovoywrote: > 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). diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 1bc9fbc..8fb2624 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -10903,6 +10903,9 @@ table2-mapping jsonb_set + jsonb_insert + + jsonb_pretty @@ -11184,6 +11187,40 @@ table2-mapping + + + jsonb_insert(target jsonb, path text[], new_value jsonb, insert_after boolean) + + + jsonb + + Returns target with + new_value inserted. + If target section designated by + path is in a JSONB array, + new_value will be inserted before target or + after if insert_after is true (default is + false). If target section + designated by path is in JSONB object, + jsonb_insert behaves as + jsonb_set function where + create_missing is true. As with the + path orientated operators, negative integers that appear in + path count from the end of JSON arrays. + + + + jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"') + + + jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"', true) + + + {"a": [0, "new_value", 1, 2]} + {"a": [0, 1, "new_value", 2]} + + + jsonb_pretty(from_json jsonb) text @@ -11235,10 +11272,11 @@ table2-mapping All the items of the path parameter of jsonb_set - must be present in the target, unless - create_missing is true, in which case all but the last item - must be present. If these conditions are not met the target - is returned unchanged. + as well as jsonb_insert except the last item must be present + in the target. If create_missing is false, all + items of the path parameter of jsonb_set must be + present. If these conditions are not met the target is + returned unchanged. If the last path item is an object key, it will be created if it diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 9ae1ef4..a6e661c 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -997,3 +997,11 @@ RETURNS text[] LANGUAGE INTERNAL STRICT IMMUTABLE AS 'parse_ident'; + +CREATE OR REPLACE FUNCTION + jsonb_insert(jsonb_in jsonb, path text[] , replacement jsonb, +insert_after boolean DEFAULT false) +RETURNS jsonb +LANGUAGE INTERNAL +STRICT IMMUTABLE +AS 'jsonb_insert'; diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 97e0e8e..97bb08e 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -33,6 +33,15 @@ #include "utils/memutils.h" #include "utils/typcache.h" +/* Operations available for setPath */ +#define JB_PATH_NOOP 0x +#define JB_PATH_CREATE 0x0001 +#define JB_PATH_DELETE 0x0002 +#define JB_PATH_INSERT_BEFORE 0x0004 +#define JB_PATH_INSERT_AFTER 0x0008 +#define JB_PATH_CREATE_OR_INSERT (JB_PATH_INSERT_BEFORE | JB_PATH_INSERT_AFTER \ + | JB_PATH_CREATE) + /* semantic action functions for json_object_keys */ static void okeys_object_field_start(void *state, char *fname, bool isnull); static void okeys_array_start(void *state); @@ -130,14 +139,14 @@ static JsonbValue *IteratorConcat(JsonbIterator **it1, JsonbIterator **it2, static JsonbValue *setPath(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, int level, Jsonb *newval, - bool create); + int op_type); static void setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, int level, - Jsonb *newval, uint32 npairs, bool create); + Jsonb *newval, uint32 npairs, int op_type); static void setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, - int level, Jsonb *newval, uint32 nelems, bool create); + int level, Jsonb *newval, uint32 nelems, int op_type); static void addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb); /* state for json_object_keys */ @@ -3544,7 +3553,7 @@ jsonb_set(PG_FUNCTION_ARGS) it = JsonbIteratorInit(>root); res = setPath(, path_elems, path_nulls, path_len, , -
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
On 3/30/16, Dmitry Dolgov <9erthali...@gmail.com> wrote: > On 31 March 2016 at 05:04, Vitaly Burovoywrote: >> 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_AFTER in 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 be removed. * * If any bit mentioned in JB_PATH_CREATE_OR_INSERT is set in op_type, * we create the new value if the key or array index does not exist. * * Bits JB_PATH_INSERT_BEFORE and JB_PATH_INSERT_AFTER in op_type * behave as JB_PATH_CREATE if new value is inserted in JsonbObject. * * All path elements before the last must already exist * whatever bits in op_type are 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
On 31 March 2016 at 05:04, Vitaly Burovoywrote: > The documentation changes still has to be fixed. > Thanks for help. Looks like I'm not so good at text formulation. Fixed. > Moreover it seems the logic in the code is correct No - I see now, that I made the same mistake in two places (e.g. in case of `setPathArray` a current item was pushed to parse state after `JB_PATH_INSERT_BEFORE`, not a new one), so they were annihilated. Fixed. diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index ae93e69..3c3ff7a 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -10904,6 +10904,9 @@ table2-mapping jsonb_set + jsonb_insert + + jsonb_pretty @@ -11185,6 +11188,40 @@ table2-mapping + + + jsonb_insert(target jsonb, path text[], new_value jsonb, insert_after boolean) + + + jsonb + + Returns target with + new_value inserted. + If target section designated by + path is in a JSONB array, + new_value will be inserted before target or + after if insert_after is true (default is + false). If target section + designated by path is in JSONB object, + jsonb_insert behaves as + jsonb_set function where + create_missing is true. As with the + path orientated operators, negative integers that appear in + path count from the end of JSON arrays. + + + + jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"') + + + jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"', true) + + + {"a": [0, "new_value", 1, 2]} + {"a": [0, 1, "new_value", 2]} + + + jsonb_pretty(from_json jsonb) text @@ -11236,10 +11273,11 @@ table2-mapping All the items of the path parameter of jsonb_set - must be present in the target, unless - create_missing is true, in which case all but the last item - must be present. If these conditions are not met the target - is returned unchanged. + as well as jsonb_insert except the last item must be present + in the target. If create_missing is false, all + items of the path parameter of jsonb_set must be + present. If these conditions are not met the target is + returned unchanged. If the last path item is an object key, it will be created if it diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 9ae1ef4..a6e661c 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -997,3 +997,11 @@ RETURNS text[] LANGUAGE INTERNAL STRICT IMMUTABLE AS 'parse_ident'; + +CREATE OR REPLACE FUNCTION + jsonb_insert(jsonb_in jsonb, path text[] , replacement jsonb, +insert_after boolean DEFAULT false) +RETURNS jsonb +LANGUAGE INTERNAL +STRICT IMMUTABLE +AS 'jsonb_insert'; diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 97e0e8e..df9fa5d 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -33,6 +33,15 @@ #include "utils/memutils.h" #include "utils/typcache.h" +/* Operations available for setPath */ +#define JB_PATH_NOOP 0x +#define JB_PATH_CREATE 0x0001 +#define JB_PATH_DELETE 0x0002 +#define JB_PATH_INSERT_BEFORE 0x0004 +#define JB_PATH_INSERT_AFTER 0x0008 +#define JB_PATH_CREATE_OR_INSERT (JB_PATH_INSERT_BEFORE | JB_PATH_INSERT_AFTER \ + | JB_PATH_CREATE) + /* semantic action functions for json_object_keys */ static void okeys_object_field_start(void *state, char *fname, bool isnull); static void okeys_array_start(void *state); @@ -130,14 +139,14 @@ static JsonbValue *IteratorConcat(JsonbIterator **it1, JsonbIterator **it2, static JsonbValue *setPath(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, int level, Jsonb *newval, - bool create); + int op_type); static void setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, int level, - Jsonb *newval, uint32 npairs, bool create); + Jsonb *newval, uint32 npairs, int op_type); static void setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, - int level, Jsonb *newval, uint32 nelems, bool create); + int level, Jsonb *newval, uint32 nelems, int op_type); static void addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb); /* state for json_object_keys */ @@ -3544,7 +3553,7 @@ jsonb_set(PG_FUNCTION_ARGS) it = JsonbIteratorInit(>root); res = setPath(, path_elems, path_nulls, path_len, , - 0, newval, create); + 0, newval, create ? JB_PATH_CREATE : JB_PATH_NOOP); Assert(res != NULL);
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
On 3/25/16, Dmitry Dolgov <9erthali...@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 "0x" 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
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. > 5. > > if (op_type != JB_PATH_DELETE) Yes, I just missed that in previous patch. > 7. > Please, return the "skip" comment. Well, I'm still not so sure about that, but if you're so confident then ok =) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index ae93e69..dfed589 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -10904,6 +10904,9 @@ table2-mapping jsonb_set + jsonb_insert + + jsonb_pretty @@ -11185,6 +11188,40 @@ table2-mapping + + + jsonb_insert(target jsonb, path text[], new_value jsonb, after boolean) + + + jsonb + + Returns target with + new_value inserted. + If target section designated by + path is a JSONB array, + new_value will be inserted before target or + after if after is true (default is + false). If target section + designated by path is a JSONB object, + jsonb_insert will act like + jsonb_set function where + create_missing is true. As with the + path orientated operators, negative integers that appear in + path count from the end of JSON arrays. + + + + jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"') + + + jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"', true) + + + {"a": [0, "new_value", 1, 2]} + {"a": [0, 1, "new_value", 2]} + + + jsonb_pretty(from_json jsonb) text @@ -11236,10 +11273,11 @@ table2-mapping All the items of the path parameter of jsonb_set - must be present in the target, unless - create_missing is true, in which case all but the last item - must be present. If these conditions are not met the target - is returned unchanged. + as well as jsonb_insert except the last item must be present + in the target. If create_missing is false, all + items of the path parameter of jsonb_set must be + present. If these conditions are not met the target is + returned unchanged. If the last path item is an object key, it will be created if it diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 9ae1ef4..58ed3c3 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -997,3 +997,11 @@ RETURNS text[] LANGUAGE INTERNAL STRICT IMMUTABLE AS 'parse_ident'; + +CREATE OR REPLACE FUNCTION + jsonb_insert(jsonb_in jsonb, path text[] , replacement jsonb, +insert_before_after boolean DEFAULT false) +RETURNS jsonb +LANGUAGE INTERNAL +STRICT IMMUTABLE +AS 'jsonb_insert'; diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 97e0e8e..5bd2165 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -33,6 +33,15 @@ #include "utils/memutils.h" #include "utils/typcache.h" +/* Operations available for setPath */ +#define JB_PATH_NOOP 0x0 +#define JB_PATH_CREATE 0x0001 +#define JB_PATH_DELETE 0x0002 +#define JB_PATH_INSERT_BEFORE 0x0004 +#define JB_PATH_INSERT_AFTER 0x0008 +#define JB_PATH_CREATE_OR_INSERT (JB_PATH_INSERT_BEFORE | JB_PATH_INSERT_AFTER \ + | JB_PATH_CREATE) + /* semantic action functions for json_object_keys */ static void okeys_object_field_start(void *state, char *fname, bool isnull); static void okeys_array_start(void *state); @@ -130,14 +139,14 @@ static JsonbValue *IteratorConcat(JsonbIterator **it1, JsonbIterator **it2, static JsonbValue *setPath(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, int level, Jsonb *newval, - bool create); + int op_type); static void setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, int level, - Jsonb *newval, uint32 npairs, bool create); + Jsonb *newval, uint32 npairs, int op_type); static void setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, - int level, Jsonb *newval, uint32 nelems, bool create); + int level, Jsonb *newval, uint32 nelems, int op_type); static void addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb); /* state for json_object_keys */ @@ -3544,7 +3553,7 @@ jsonb_set(PG_FUNCTION_ARGS) it = JsonbIteratorInit(>root); res = setPath(, path_elems, path_nulls, path_len, , - 0, newval, create); + 0, newval, create ? JB_PATH_CREATE : JB_PATH_NOOP); Assert(res != NULL); @@
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
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(, , 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: > Iftarget 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
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
> 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. I'm thinking about this following way - `jsonb_insert` is just a function to insert a new value, which has specific options to enforce arbitrary position in 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 available only for arrays.
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
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, 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. 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(, , true); ``` So since in this patch it's not a simple skipping for setPathArray, I removed that commentary. Am I wrong? > 9. And finally... it does not work as expected in case of: Yes, good catch, thanks. diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index c0b94bc..158e7fb 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -10791,6 +10791,9 @@ table2-mapping jsonb_set + jsonb_insert + + jsonb_pretty @@ -11072,6 +11075,39 @@ table2-mapping + + + jsonb_insert(target jsonb, path text[], new_value jsonb, after boolean) + + + jsonb + + Returns target with + new_value inserted. + Iftarget section designated by + path is a JSONB array, + new_value will be inserted before it, or + after if after is true (defailt is + false). If target section + designated by path is a JSONB object, + new_value will be added just like a regular + key. As with the path orientated operators, negative integers that + appear in path count from the end of JSON + arrays. + + + + jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"') + + + jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"', true) + + + {"a": [0, "new_value", 1, 2]} + {"a": [0, 1, "new_value", 2]} + + + jsonb_pretty(from_json jsonb) text diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index abf9a70..b1281e7 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -971,3 +971,11 @@ RETURNS jsonb LANGUAGE INTERNAL STRICT IMMUTABLE AS 'jsonb_set'; + +CREATE OR REPLACE FUNCTION + jsonb_insert(jsonb_in jsonb, path text[] , replacement jsonb, +insert_before_after boolean DEFAULT false) +RETURNS jsonb +LANGUAGE INTERNAL +STRICT IMMUTABLE +AS 'jsonb_insert'; diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 88225aa..1c1da7c 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -33,6 +33,13 @@ #include "utils/memutils.h" #include "utils/typcache.h" +/* Operations available for setPath */ +#define JB_PATH_NOOP 0x0 +#define JB_PATH_CREATE 0x0001 +#define JB_PATH_DELETE 0x0002 +#define JB_PATH_INSERT_BEFORE 0x0004 +#define JB_PATH_INSERT_AFTER 0x0008 + /* semantic action functions for json_object_keys */ static void okeys_object_field_start(void *state, char *fname, bool isnull); static void okeys_array_start(void *state); @@ -130,14 +137,14 @@ static JsonbValue *IteratorConcat(JsonbIterator **it1, JsonbIterator **it2, static JsonbValue *setPath(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, int level, Jsonb *newval, - bool create); + int op_type); static void setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, int level, - Jsonb *newval, uint32 npairs, bool create); + Jsonb *newval, uint32 npairs, int op_type); static void setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls, int path_len, JsonbParseState **st, - int level, Jsonb *newval, uint32 nelems, bool create); + int level, Jsonb *newval, uint32 nelems, int op_type); static void addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb); /* state for json_object_keys */ @@ -3544,7 +3551,7 @@ jsonb_set(PG_FUNCTION_ARGS) it = JsonbIteratorInit(>root); res = setPath(, path_elems, path_nulls, path_len, , - 0, newval, create); + 0, newval, create ? JB_PATH_CREATE : JB_PATH_NOOP); Assert(res != NULL); @@ -3588,7 +3595,52 @@ jsonb_delete_path(PG_FUNCTION_ARGS) it = JsonbIteratorInit(>root); - res = setPath(, path_elems, path_nulls, path_len, , 0, NULL, false); + res = setPath(, path_elems, path_nulls, path_len, , + 0, NULL, JB_PATH_DELETE); + + Assert(res != NULL); + + PG_RETURN_JSONB(JsonbValueToJsonb(res)); +} + +/* + * SQL function jsonb_insert(jsonb, text[], jsonb, boolean) + * + */ +Datum +jsonb_insert(PG_FUNCTION_ARGS) +{ +
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
On 2016-02-29 17:19+00, Dmitry Dolgov <9erthali...@gmail.com> wrote: On 2016-02-24 19:37+00, Petr Jelinekwrote: >> 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(, path_elems, path_nulls, path_len, , - 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, , true);/* skip */ + r = JsonbIteratorNext(it, , 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Jsonb, insert a new value into an array at arbitrary position
> 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. > Also this patch needs documentation. I've added new version in attachments, thanks. jsonb_insert_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers