Re: [HACKERS] PATCH: recursive json_populate_record()
On 30.05.2017 02:31, Tom Lane wrote: Nikita Glukhovwrites: 2. Also I've found a some kind of thinko in JsGetObjectSize() macro, but it seems that this obvious mistake can not lead to incorrect behavior. Hm, I think it actually was wrong for the case of jsonb_cont == NULL, wasn't it? Yes, JsObjectIsEmpty() returned false negative answer for jsonb_cont == NULL, but this could only lead to suboptimal behavior of populate_record() when the default record value was given. But maybe that case didn't arise for the sole call site. It also seems to me that populate_record() can't be called now with jsonb_cont == NULL from the existing call sites. -- Nikita Glukhov Postgres Professional:http://www.postgrespro.com The Russian Postgres Company -- 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] PATCH: recursive json_populate_record()
Noah Mischwrites: > On Mon, May 22, 2017 at 10:19:37PM +0300, Nikita Glukhov wrote: >> Attached two small fixes for the previous committed patch: > [Action required within three days. This is a generic notification.] > The above-described topic is currently a PostgreSQL 10 open item. Andrew, > since you committed the patch believed to have created it, you own this open > item. These fixes are committed in e45c5be99d08d7bb6708d7bb1dd0f5d99798c6aa and 68cff231e3a3d0ca9988cf1fde5a3be53235b3bb. 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] PATCH: recursive json_populate_record()
On Mon, May 22, 2017 at 10:19:37PM +0300, Nikita Glukhov wrote: > Attached two small fixes for the previous committed patch: > > 1. I've noticed a difference in behavior between json_populate_record() > and jsonb_populate_record() if we are trying to populate record from a > non-JSON-object: json function throws an error but jsonb function returns > a record with NULL fields. So I think it would be better to throw an error > in jsonb case too, but I'm not sure. > > 2. Also I've found a some kind of thinko in JsGetObjectSize() macro, but it > seems that this obvious mistake can not lead to incorrect behavior. [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Andrew, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com -- 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] PATCH: recursive json_populate_record()
Nikita Glukhovwrites: > Attached two small fixes for the previous committed patch: > 1. I've noticed a difference in behavior between json_populate_record() > and jsonb_populate_record() if we are trying to populate record from a > non-JSON-object: json function throws an error but jsonb function returns > a record with NULL fields. So I think it would be better to throw an error > in jsonb case too, but I'm not sure. Agreed on the error. I reformatted the code a bit. > 2. Also I've found a some kind of thinko in JsGetObjectSize() macro, but it > seems that this obvious mistake can not lead to incorrect behavior. Hm, I think it actually was wrong for the case of jsonb_cont == NULL, wasn't it? But maybe that case didn't arise for the sole call site. Anyway, agreed. Pushed both patches. 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] PATCH: recursive json_populate_record()
Attached two small fixes for the previous committed patch: 1. I've noticed a difference in behavior between json_populate_record() and jsonb_populate_record() if we are trying to populate record from a non-JSON-object: json function throws an error but jsonb function returns a record with NULL fields. So I think it would be better to throw an error in jsonb case too, but I'm not sure. 2. Also I've found a some kind of thinko in JsGetObjectSize() macro, but it seems that this obvious mistake can not lead to incorrect behavior. -- Nikita Glukhov Postgres Professional:http://www.postgrespro.com The Russian Postgres Company diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index ab9a745..a98742f 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -308,12 +308,11 @@ typedef struct JsObject ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \ : ((jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString)) -#define JsObjectSize(jso) \ +#define JsObjectIsEmpty(jso) \ ((jso)->is_json \ - ? hash_get_num_entries((jso)->val.json_hash) \ - : !(jso)->val.jsonb_cont || JsonContainerSize((jso)->val.jsonb_cont)) - -#define JsObjectIsEmpty(jso) (JsObjectSize(jso) == 0) + ? hash_get_num_entries((jso)->val.json_hash) == 0 \ + : (!(jso)->val.jsonb_cont || \ + JsonContainerSize((jso)->val.jsonb_cont) == 0)) #define JsObjectFree(jso) do { \ if ((jso)->is_json) \ diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index a98742f..083982c 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -2683,7 +2683,20 @@ JsValueToJsObject(JsValue *jsv, JsObject *jso) JsonContainerIsObject(jbv->val.binary.data)) jso->val.jsonb_cont = jbv->val.binary.data; else + { + bool is_scalar = IsAJsonbScalar(jbv) || +(jbv->type == jbvBinary && + JsonContainerIsScalar(jbv->val.binary.data)); + + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg(is_scalar + ? "cannot call %s on a scalar" + : "cannot call %s on an array", + "populate_composite"))); + jso->val.jsonb_cont = NULL; + } } } diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out index f199eb4..7954703 100644 --- a/src/test/regress/expected/jsonb.out +++ b/src/test/regress/expected/jsonb.out @@ -2276,17 +2276,9 @@ SELECT jsa FROM jsonb_populate_record(NULL::jsbrec, '{"jsa": ["aaa", null, [1, 2 (1 row) SELECT rec FROM jsonb_populate_record(NULL::jsbrec, '{"rec": 123}') q; - rec --- - (,,) -(1 row) - +ERROR: cannot call populate_composite on a scalar SELECT rec FROM jsonb_populate_record(NULL::jsbrec, '{"rec": [1, 2]}') q; - rec --- - (,,) -(1 row) - +ERROR: cannot call populate_composite on an array SELECT rec FROM jsonb_populate_record(NULL::jsbrec, '{"rec": {"a": "abc", "c": "01.02.2003", "x": 43.2}}') q; rec --- @@ -2303,11 +2295,7 @@ SELECT reca FROM jsonb_populate_record(NULL::jsbrec, '{"reca": 123}') q; ERROR: expected json array HINT: see the value of key "reca" SELECT reca FROM jsonb_populate_record(NULL::jsbrec, '{"reca": [1, 2]}') q; - reca -- - {"(,,)","(,,)"} -(1 row) - +ERROR: cannot call populate_composite on a scalar SELECT reca FROM jsonb_populate_record(NULL::jsbrec, '{"reca": [{"a": "abc", "b": 456}, null, {"c": "01.02.2003", "x": 43.2}]}') q; reca -- 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] PATCH: recursive json_populate_record()
On 04/05/2017 07:24 PM, Andrew Dunstan wrote: > > OK, I have been through this and I think it's basically committable. I > am currently doing some cleanup, such as putting all the typedefs in one > place, fixing redundant comments and adding more comments, declaring all > the static functions, and minor code changes, but nothing major. I > should be ready to commit tomorrow some time. > I have committed this. I will continue to work on adding comments. cheers andrew -- Andrew Dunstanhttps://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] PATCH: recursive json_populate_record()
On 04/04/2017 05:18 PM, Andrew Dunstan wrote: > > On 04/03/2017 05:17 PM, Andres Freund wrote: >> On 2017-03-21 14:31:08 -0400, Andrew Dunstan wrote: >>> On 03/21/2017 01:37 PM, David Steele wrote: On 3/16/17 11:54 AM, David Steele wrote: > On 2/1/17 12:53 AM, Michael Paquier wrote: >> On Thu, Jan 26, 2017 at 6:49 AM, Tom Lanewrote: >>> Nikita Glukhov writes: On 25.01.2017 23:58, Tom Lane wrote: > I think you need to take a second look at the code you're producing > and realize that it's not so clean either. This extract from > populate_record_field, for example, is pretty horrid: But what if we introduce some helper macros like this: #define JsValueIsNull(jsv) \ ((jsv)->is_json ? !(jsv)->val.json.str \ : !(jsv)->val.jsonb || (jsv)->val.jsonb->type == jbvNull) #define JsValueIsString(jsv) \ ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \ : (jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString) >>> Yeah, I was wondering about that too. I'm not sure that you can make >>> a reasonable set of helper macros that will fix this, but if you want >>> to try, go for it. >>> >>> BTW, just as a stylistic thing, I find "a?b:c||d" unreadable: I have >>> to go back to the manual every darn time to convince myself whether >>> that means (a?b:c)||d or a?b:(c||d). It's better not to rely on >>> the reader (... or the author) having memorized C's precedence rules >>> in quite that much detail. Extra parens help. >> Moved to CF 2017-03 as discussion is going on and more review is >> needed on the last set of patches. >> > The current patches do not apply cleanly at cccbdde: > > $ git apply ../other/0001-introduce-JsonContainerXxx-macros-v04.patch > error: patch failed: src/backend/utils/adt/jsonb_util.c:328 > error: src/backend/utils/adt/jsonb_util.c: patch does not apply > error: patch failed: src/backend/utils/adt/jsonfuncs.c:1266 > error: src/backend/utils/adt/jsonfuncs.c: patch does not apply > error: patch failed: src/include/utils/jsonb.h:218 > error: src/include/utils/jsonb.h: patch does not apply > > In addition, it appears a number of suggestions have been made by Tom > that warrant new patches. Marked "Waiting on Author". This thread has been idle for months since Tom's review. The submission has been marked "Returned with Feedback". Please feel free to resubmit to a future commitfest. >>> Please revive. I am planning to look at this later this week. >> Since that was 10 days ago - you're still planning to get this in before >> the freeze? >> > > Hoping to, other demands have intervened a bit, but I might be able to. > OK, I have been through this and I think it's basically committable. I am currently doing some cleanup, such as putting all the typedefs in one place, fixing redundant comments and adding more comments, declaring all the static functions, and minor code changes, but nothing major. I should be ready to commit tomorrow some time. cheers andrew -- Andrew Dunstanhttps://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] PATCH: recursive json_populate_record()
On 04/03/2017 05:17 PM, Andres Freund wrote: > On 2017-03-21 14:31:08 -0400, Andrew Dunstan wrote: >> >> On 03/21/2017 01:37 PM, David Steele wrote: >>> On 3/16/17 11:54 AM, David Steele wrote: On 2/1/17 12:53 AM, Michael Paquier wrote: > On Thu, Jan 26, 2017 at 6:49 AM, Tom Lanewrote: >> Nikita Glukhov writes: >>> On 25.01.2017 23:58, Tom Lane wrote: I think you need to take a second look at the code you're producing and realize that it's not so clean either. This extract from populate_record_field, for example, is pretty horrid: >>> But what if we introduce some helper macros like this: >>> #define JsValueIsNull(jsv) \ >>> ((jsv)->is_json ? !(jsv)->val.json.str \ >>> : !(jsv)->val.jsonb || (jsv)->val.jsonb->type == jbvNull) >>> #define JsValueIsString(jsv) \ >>> ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \ >>> : (jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString) >> Yeah, I was wondering about that too. I'm not sure that you can make >> a reasonable set of helper macros that will fix this, but if you want >> to try, go for it. >> >> BTW, just as a stylistic thing, I find "a?b:c||d" unreadable: I have >> to go back to the manual every darn time to convince myself whether >> that means (a?b:c)||d or a?b:(c||d). It's better not to rely on >> the reader (... or the author) having memorized C's precedence rules >> in quite that much detail. Extra parens help. > Moved to CF 2017-03 as discussion is going on and more review is > needed on the last set of patches. > The current patches do not apply cleanly at cccbdde: $ git apply ../other/0001-introduce-JsonContainerXxx-macros-v04.patch error: patch failed: src/backend/utils/adt/jsonb_util.c:328 error: src/backend/utils/adt/jsonb_util.c: patch does not apply error: patch failed: src/backend/utils/adt/jsonfuncs.c:1266 error: src/backend/utils/adt/jsonfuncs.c: patch does not apply error: patch failed: src/include/utils/jsonb.h:218 error: src/include/utils/jsonb.h: patch does not apply In addition, it appears a number of suggestions have been made by Tom that warrant new patches. Marked "Waiting on Author". >>> This thread has been idle for months since Tom's review. >>> >>> The submission has been marked "Returned with Feedback". Please feel >>> free to resubmit to a future commitfest. >>> >>> >> Please revive. I am planning to look at this later this week. > Since that was 10 days ago - you're still planning to get this in before > the freeze? > Hoping to, other demands have intervened a bit, but I might be able to. cheers andrew -- Andrew Dunstanhttps://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] PATCH: recursive json_populate_record()
On 2017-03-21 14:31:08 -0400, Andrew Dunstan wrote: > > > On 03/21/2017 01:37 PM, David Steele wrote: > > On 3/16/17 11:54 AM, David Steele wrote: > >> On 2/1/17 12:53 AM, Michael Paquier wrote: > >>> On Thu, Jan 26, 2017 at 6:49 AM, Tom Lanewrote: > Nikita Glukhov writes: > > On 25.01.2017 23:58, Tom Lane wrote: > >> I think you need to take a second look at the code you're producing > >> and realize that it's not so clean either. This extract from > >> populate_record_field, for example, is pretty horrid: > > > But what if we introduce some helper macros like this: > > > #define JsValueIsNull(jsv) \ > > ((jsv)->is_json ? !(jsv)->val.json.str \ > > : !(jsv)->val.jsonb || (jsv)->val.jsonb->type == jbvNull) > > > #define JsValueIsString(jsv) \ > > ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \ > > : (jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString) > > Yeah, I was wondering about that too. I'm not sure that you can make > a reasonable set of helper macros that will fix this, but if you want > to try, go for it. > > BTW, just as a stylistic thing, I find "a?b:c||d" unreadable: I have > to go back to the manual every darn time to convince myself whether > that means (a?b:c)||d or a?b:(c||d). It's better not to rely on > the reader (... or the author) having memorized C's precedence rules > in quite that much detail. Extra parens help. > >>> > >>> Moved to CF 2017-03 as discussion is going on and more review is > >>> needed on the last set of patches. > >>> > >> > >> The current patches do not apply cleanly at cccbdde: > >> > >> $ git apply ../other/0001-introduce-JsonContainerXxx-macros-v04.patch > >> error: patch failed: src/backend/utils/adt/jsonb_util.c:328 > >> error: src/backend/utils/adt/jsonb_util.c: patch does not apply > >> error: patch failed: src/backend/utils/adt/jsonfuncs.c:1266 > >> error: src/backend/utils/adt/jsonfuncs.c: patch does not apply > >> error: patch failed: src/include/utils/jsonb.h:218 > >> error: src/include/utils/jsonb.h: patch does not apply > >> > >> In addition, it appears a number of suggestions have been made by Tom > >> that warrant new patches. Marked "Waiting on Author". > > > > This thread has been idle for months since Tom's review. > > > > The submission has been marked "Returned with Feedback". Please feel > > free to resubmit to a future commitfest. > > > > > > Please revive. I am planning to look at this later this week. Since that was 10 days ago - you're still planning to get this in before the freeze? - Andres -- 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] PATCH: recursive json_populate_record()
On 22.03.2017 00:26, David Steele wrote: On 3/21/17 2:31 PM, Andrew Dunstan wrote: On 03/21/2017 01:37 PM, David Steele wrote: >> This thread has been idle for months since Tom's review. The submission has been marked "Returned with Feedback". Please feel free to resubmit to a future commitfest. Please revive. I am planning to look at this later this week. Revived in "Waiting on Author" state. Here is updated v05 version of this patch: * rebased to the latest master * one patch is completely removed because it is unnecessary now * added some macros for JsValue fields access * added new JsObject structure for passing json/jsonb objects * refactoring patch is not yet simplified (not broken into a step-by-step sequence) Also I must notice that json branch of this code is not as optimal as it might be: there could be repetitive parsing passes for nested json objects/arrays instead of a single parsing pass. -- Nikita Glukhov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index bf2c91f..da29dab 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -109,6 +109,7 @@ typedef struct JhashState HTAB *hash; char *saved_scalar; char *save_json_start; + JsonTokenType saved_token_type; } JHashState; /* hashtable element */ @@ -116,26 +117,49 @@ typedef struct JsonHashEntry { char fname[NAMEDATALEN]; /* hash key (MUST BE FIRST) */ char *val; - char *json; - bool isnull; + JsonTokenType type; } JsonHashEntry; -/* these two are stolen from hstore / record_out, used in populate_record* */ -typedef struct ColumnIOData +/* structure to cache type I/O metadata needed for populate_scalar() */ +typedef struct ScalarIOData { - Oid column_type; - Oid typiofunc; Oid typioparam; - FmgrInfo proc; -} ColumnIOData; + FmgrInfo typiofunc; +} ScalarIOData; + +typedef struct ColumnIOData ColumnIOData; +typedef struct RecordIOData RecordIOData; -typedef struct RecordIOData +/* structure to cache metadata needed for populate_composite() */ +typedef struct CompositeIOData +{ + /* + * We use pointer to a RecordIOData here because variable-length + * struct RecordIOData can't be used directly in ColumnIOData.io union + */ + RecordIOData *record_io; /* metadata cache for populate_record() */ + TupleDesc tupdesc; /* cached tuple descriptor */ +} CompositeIOData; + +/* these two are stolen from hstore / record_out, used in populate_record* */ + +/* structure to cache record metadata needed for populate_record_field() */ +struct ColumnIOData +{ + Oid typid; /* column type id */ + int32 typmod; /* column type modifier */ + ScalarIOData scalar_io; /* metadata cache for directi conversion + * through input function */ +}; + +/* structure to cache record metadata needed for populate_record() */ +struct RecordIOData { Oid record_type; int32 record_typmod; int ncolumns; ColumnIOData columns[FLEXIBLE_ARRAY_MEMBER]; -} RecordIOData; +}; /* state for populate_recordset */ typedef struct PopulateRecordsetState @@ -145,10 +169,11 @@ typedef struct PopulateRecordsetState HTAB *json_hash; char *saved_scalar; char *save_json_start; + JsonTokenType saved_token_type; Tuplestorestate *tuple_store; TupleDesc ret_tdesc; HeapTupleHeader rec; - RecordIOData *my_extra; + RecordIOData **my_extra; MemoryContext fn_mcxt; /* used to stash IO funcs */ } PopulateRecordsetState; @@ -160,6 +185,55 @@ typedef struct StripnullState bool skip_next_null; } StripnullState; +/* structure for generalized json/jsonb value passing */ +typedef struct JsValue +{ + bool is_json;/* json/jsonb */ + union + { + struct + { + char *str; /* json string */ + int len; /* json string length or -1 if null-terminated */ + JsonTokenType type; /* json type */ + } json; /* json value */ + + JsonbValue *jsonb; /* jsonb value */ + } val; +} JsValue; + +typedef struct JsObject +{ + bool is_json; /* json/jsonb */ + union + { + HTAB *json_hash; + JsonbContainer *jsonb_cont; + } val; +} JsObject; + +#define JsValueIsNull(jsv) \ + ((jsv)->is_json ? \ + (!(jsv)->val.json.str || (jsv)->val.json.type == JSON_TOKEN_NULL) : \ + (!(jsv)->val.jsonb || (jsv)->val.jsonb->type == jbvNull)) + +#define JsValueIsString(jsv) \ + ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \ + : ((jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString)) + +#define JsObjectSize(jso) \ + ((jso)->is_json \ + ? hash_get_num_entries((jso)->val.json_hash) \ + : !(jso)->val.jsonb_cont || JsonContainerSize((jso)->val.jsonb_cont)) + +#define JsObjectIsEmpty(jso) (JsObjectSize(jso) == 0) + +#define JsObjectFree(jso) do { \ + if ((jso)->is_json) \ + hash_destroy((jso)->val.json_hash); \ + } while (0) + + /* semantic action functions for json_object_keys */ static void okeys_object_field_start(void
Re: [HACKERS] PATCH: recursive json_populate_record()
On 3/21/17 2:31 PM, Andrew Dunstan wrote: On 03/21/2017 01:37 PM, David Steele wrote: >> This thread has been idle for months since Tom's review. The submission has been marked "Returned with Feedback". Please feel free to resubmit to a future commitfest. Please revive. I am planning to look at this later this week. Revived in "Waiting on Author" state. -- -David da...@pgmasters.net -- 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] PATCH: recursive json_populate_record()
On 03/21/2017 01:37 PM, David Steele wrote: > On 3/16/17 11:54 AM, David Steele wrote: >> On 2/1/17 12:53 AM, Michael Paquier wrote: >>> On Thu, Jan 26, 2017 at 6:49 AM, Tom Lanewrote: Nikita Glukhov writes: > On 25.01.2017 23:58, Tom Lane wrote: >> I think you need to take a second look at the code you're producing >> and realize that it's not so clean either. This extract from >> populate_record_field, for example, is pretty horrid: > But what if we introduce some helper macros like this: > #define JsValueIsNull(jsv) \ > ((jsv)->is_json ? !(jsv)->val.json.str \ > : !(jsv)->val.jsonb || (jsv)->val.jsonb->type == jbvNull) > #define JsValueIsString(jsv) \ > ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \ > : (jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString) Yeah, I was wondering about that too. I'm not sure that you can make a reasonable set of helper macros that will fix this, but if you want to try, go for it. BTW, just as a stylistic thing, I find "a?b:c||d" unreadable: I have to go back to the manual every darn time to convince myself whether that means (a?b:c)||d or a?b:(c||d). It's better not to rely on the reader (... or the author) having memorized C's precedence rules in quite that much detail. Extra parens help. >>> >>> Moved to CF 2017-03 as discussion is going on and more review is >>> needed on the last set of patches. >>> >> >> The current patches do not apply cleanly at cccbdde: >> >> $ git apply ../other/0001-introduce-JsonContainerXxx-macros-v04.patch >> error: patch failed: src/backend/utils/adt/jsonb_util.c:328 >> error: src/backend/utils/adt/jsonb_util.c: patch does not apply >> error: patch failed: src/backend/utils/adt/jsonfuncs.c:1266 >> error: src/backend/utils/adt/jsonfuncs.c: patch does not apply >> error: patch failed: src/include/utils/jsonb.h:218 >> error: src/include/utils/jsonb.h: patch does not apply >> >> In addition, it appears a number of suggestions have been made by Tom >> that warrant new patches. Marked "Waiting on Author". > > This thread has been idle for months since Tom's review. > > The submission has been marked "Returned with Feedback". Please feel > free to resubmit to a future commitfest. > > Please revive. I am planning to look at this later this week. cheers andrew -- Andrew Dunstanhttps://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] PATCH: recursive json_populate_record()
On 3/16/17 11:54 AM, David Steele wrote: On 2/1/17 12:53 AM, Michael Paquier wrote: On Thu, Jan 26, 2017 at 6:49 AM, Tom Lanewrote: Nikita Glukhov writes: On 25.01.2017 23:58, Tom Lane wrote: I think you need to take a second look at the code you're producing and realize that it's not so clean either. This extract from populate_record_field, for example, is pretty horrid: But what if we introduce some helper macros like this: #define JsValueIsNull(jsv) \ ((jsv)->is_json ? !(jsv)->val.json.str \ : !(jsv)->val.jsonb || (jsv)->val.jsonb->type == jbvNull) #define JsValueIsString(jsv) \ ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \ : (jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString) Yeah, I was wondering about that too. I'm not sure that you can make a reasonable set of helper macros that will fix this, but if you want to try, go for it. BTW, just as a stylistic thing, I find "a?b:c||d" unreadable: I have to go back to the manual every darn time to convince myself whether that means (a?b:c)||d or a?b:(c||d). It's better not to rely on the reader (... or the author) having memorized C's precedence rules in quite that much detail. Extra parens help. Moved to CF 2017-03 as discussion is going on and more review is needed on the last set of patches. The current patches do not apply cleanly at cccbdde: $ git apply ../other/0001-introduce-JsonContainerXxx-macros-v04.patch error: patch failed: src/backend/utils/adt/jsonb_util.c:328 error: src/backend/utils/adt/jsonb_util.c: patch does not apply error: patch failed: src/backend/utils/adt/jsonfuncs.c:1266 error: src/backend/utils/adt/jsonfuncs.c: patch does not apply error: patch failed: src/include/utils/jsonb.h:218 error: src/include/utils/jsonb.h: patch does not apply In addition, it appears a number of suggestions have been made by Tom that warrant new patches. Marked "Waiting on Author". This thread has been idle for months since Tom's review. The submission has been marked "Returned with Feedback". Please feel free to resubmit to a future commitfest. Thanks, -- -David da...@pgmasters.net -- 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] PATCH: recursive json_populate_record()
On 2/1/17 12:53 AM, Michael Paquier wrote: > On Thu, Jan 26, 2017 at 6:49 AM, Tom Lanewrote: >> Nikita Glukhov writes: >>> On 25.01.2017 23:58, Tom Lane wrote: I think you need to take a second look at the code you're producing and realize that it's not so clean either. This extract from populate_record_field, for example, is pretty horrid: >> >>> But what if we introduce some helper macros like this: >> >>> #define JsValueIsNull(jsv) \ >>> ((jsv)->is_json ? !(jsv)->val.json.str \ >>> : !(jsv)->val.jsonb || (jsv)->val.jsonb->type == jbvNull) >> >>> #define JsValueIsString(jsv) \ >>> ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \ >>> : (jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString) >> >> Yeah, I was wondering about that too. I'm not sure that you can make >> a reasonable set of helper macros that will fix this, but if you want >> to try, go for it. >> >> BTW, just as a stylistic thing, I find "a?b:c||d" unreadable: I have >> to go back to the manual every darn time to convince myself whether >> that means (a?b:c)||d or a?b:(c||d). It's better not to rely on >> the reader (... or the author) having memorized C's precedence rules >> in quite that much detail. Extra parens help. > > Moved to CF 2017-03 as discussion is going on and more review is > needed on the last set of patches. > The current patches do not apply cleanly at cccbdde: $ git apply ../other/0001-introduce-JsonContainerXxx-macros-v04.patch error: patch failed: src/backend/utils/adt/jsonb_util.c:328 error: src/backend/utils/adt/jsonb_util.c: patch does not apply error: patch failed: src/backend/utils/adt/jsonfuncs.c:1266 error: src/backend/utils/adt/jsonfuncs.c: patch does not apply error: patch failed: src/include/utils/jsonb.h:218 error: src/include/utils/jsonb.h: patch does not apply In addition, it appears a number of suggestions have been made by Tom that warrant new patches. Marked "Waiting on Author". -- -David da...@pgmasters.net -- 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] PATCH: recursive json_populate_record()
On Thu, Jan 26, 2017 at 6:49 AM, Tom Lanewrote: > Nikita Glukhov writes: >> On 25.01.2017 23:58, Tom Lane wrote: >>> I think you need to take a second look at the code you're producing >>> and realize that it's not so clean either. This extract from >>> populate_record_field, for example, is pretty horrid: > >> But what if we introduce some helper macros like this: > >> #define JsValueIsNull(jsv) \ >> ((jsv)->is_json ? !(jsv)->val.json.str \ >> : !(jsv)->val.jsonb || (jsv)->val.jsonb->type == jbvNull) > >> #define JsValueIsString(jsv) \ >> ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \ >> : (jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString) > > Yeah, I was wondering about that too. I'm not sure that you can make > a reasonable set of helper macros that will fix this, but if you want > to try, go for it. > > BTW, just as a stylistic thing, I find "a?b:c||d" unreadable: I have > to go back to the manual every darn time to convince myself whether > that means (a?b:c)||d or a?b:(c||d). It's better not to rely on > the reader (... or the author) having memorized C's precedence rules > in quite that much detail. Extra parens help. Moved to CF 2017-03 as discussion is going on and more review is needed on the last set of patches. -- Michael -- 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] PATCH: recursive json_populate_record()
Nikita Glukhovwrites: > On 25.01.2017 23:58, Tom Lane wrote: >> I think you need to take a second look at the code you're producing >> and realize that it's not so clean either. This extract from >> populate_record_field, for example, is pretty horrid: > But what if we introduce some helper macros like this: > #define JsValueIsNull(jsv) \ > ((jsv)->is_json ? !(jsv)->val.json.str \ > : !(jsv)->val.jsonb || (jsv)->val.jsonb->type == jbvNull) > #define JsValueIsString(jsv) \ > ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \ > : (jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString) Yeah, I was wondering about that too. I'm not sure that you can make a reasonable set of helper macros that will fix this, but if you want to try, go for it. BTW, just as a stylistic thing, I find "a?b:c||d" unreadable: I have to go back to the manual every darn time to convince myself whether that means (a?b:c)||d or a?b:(c||d). It's better not to rely on the reader (... or the author) having memorized C's precedence rules in quite that much detail. Extra parens help. 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] PATCH: recursive json_populate_record()
On 25.01.2017 23:58, Tom Lane wrote: I think you need to take a second look at the code you're producing and realize that it's not so clean either. This extract from populate_record_field, for example, is pretty horrid: But what if we introduce some helper macros like this: #define JsValueIsNull(jsv) \ ((jsv)->is_json ? !(jsv)->val.json.str \ : !(jsv)->val.jsonb || (jsv)->val.jsonb->type == jbvNull) #define JsValueIsString(jsv) \ ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \ : (jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString) /* prepare column metadata cache for the given type */ if (col->typid != typid || col->typmod != typmod) prepare_column_cache(col, typid, typmod, mcxt, jsv->is_json); *isnull = JsValueIsNull(jsv); typcat = col->typcat; /* try to convert json string to a non-scalar type through input function */ if (JsValueIsString(jsv) && (typcat == TYPECAT_ARRAY || typcat == TYPECAT_COMPOSITE)) typcat = TYPECAT_SCALAR; /* we must perform domain checks for NULLs */ if (*isnull && typcat != TYPECAT_DOMAIN) return (Datum) 0; -- Nikita Glukhov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- 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] PATCH: recursive json_populate_record()
Nikita Glukhovwrites: > On 22.01.2017 21:58, Tom Lane wrote: >> 5. The business with having some arguments that are only for json and >> others that are only for jsonb, eg in populate_scalar(), also makes me >> itch. > I've refactored json/jsonb passing using new struct JsValue which contains > union for json/jsonb values. I'm not too enamored of that fix. It doesn't do much for readability, and at least with my compiler (gcc 4.4.7), I sometimes get "variable might be used uninitialized" warnings, probably due to not all fields of the union getting set in every code path. >> I wonder whether this wouldn't all get a lot simpler and more >> readable if we gave up on trying to share code between the two cases. > Maybe two separate families of functions like this > ... > could slightly improve readability, but such code duplication (I believe it is > a duplicate code) would never be acceptable to me. I think you need to take a second look at the code you're producing and realize that it's not so clean either. This extract from populate_record_field, for example, is pretty horrid: /* prepare column metadata cache for the given type */ if (col->typid != typid || col->typmod != typmod) prepare_column_cache(col, typid, typmod, mcxt, jsv->is_json); *isnull = jsv->is_json ? jsv->val.json.str == NULL : jsv->val.jsonb == NULL || jsv->val.jsonb->type == jbvNull; typcat = col->typcat; /* try to convert json string to a non-scalar type through input function */ if ((jsv->is_json ? jsv->val.json.type == JSON_TOKEN_STRING : jsv->val.jsonb && jsv->val.jsonb->type == jbvString) && (typcat == TYPECAT_ARRAY || typcat == TYPECAT_COMPOSITE)) typcat = TYPECAT_SCALAR; /* we must perform domain checks for NULLs */ if (*isnull && typcat != TYPECAT_DOMAIN) return (Datum) 0; When every other line contains an is_json conditional, you are not making good readable code. It's also worth noting that this is going to become even less readable after pgindent gets done with it: /* prepare column metadata cache for the given type */ if (col->typid != typid || col->typmod != typmod) prepare_column_cache(col, typid, typmod, mcxt, jsv->is_json); *isnull = jsv->is_json ? jsv->val.json.str == NULL : jsv->val.jsonb == NULL || jsv->val.jsonb->type == jbvNull; typcat = col->typcat; /* try to convert json string to a non-scalar type through input function */ if ((jsv->is_json ? jsv->val.json.type == JSON_TOKEN_STRING : jsv->val.jsonb && jsv->val.jsonb->type == jbvString) && (typcat == TYPECAT_ARRAY || typcat == TYPECAT_COMPOSITE)) typcat = TYPECAT_SCALAR; /* we must perform domain checks for NULLs */ if (*isnull && typcat != TYPECAT_DOMAIN) return (Datum) 0; You could maybe improve that result with some different formatting choices, but I think it's basically a readability fail to be relying heavily on multiline x?y:z conditionals in the first place. And I still maintain that it's entirely silly to be structuring populate_scalar() this way. So I really think it'd be a good idea to explore separating the json and jsonb code paths. This direction isn't looking like a win. >> 7. More generally, the patch is hard to review because it whacks the >> existing code around so thoroughly that it's tough even to match up >> old and new code to get a handle on what you changed functionally. >> Maybe it would be good if you could separate it into a refactoring >> patch that just moves existing code around without functional changes, >> and then a patch on top of that that adds code and makes only localized >> changes in what's already there. > I've split this patch into two patches as you asked. That didn't really help :-( ... the 0002 patch is still nigh unreadable. Maybe it's trying to do too much in one step. 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] PATCH: recursive json_populate_record()
Nikita Glukhovwrites: > On 22.01.2017 21:58, Tom Lane wrote: >> If you want such macros I think it would be better to submit a separate >> cosmetic patch that tries to hide such bit-tests behind macros throughout >> the jsonb code. > I've attached that patch, but not all the bit-tests were hidden: some of them > in jsonb_util.c still remain valid after upcoming refactoring because they > don't belong to generic code (there might be better to use JBC_XXX() macros). Pushed this; grepping found a couple other places that could be replaced by the macros, so I did. I didn't include the JsonContainerIsEmpty macro, though. It wasn't used anywhere, and I'm not exactly convinced that "IsEmpty" is more readable than "Size == 0", anyhow. We can add it later if the use-case gets stronger. > Sorry for this obvious mistake. But macros JB_ROOT_IS_XXX() also contain the > same hazard. Good point, fixed. I'll look at the rest of this in a bit. 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] PATCH: recursive json_populate_record()
On 22.01.2017 21:58, Tom Lane wrote: In the meantime, we are *not* going to have attndims be semantically significant in just this one function. Therefore, please remove this patch's dependence on attndims. Ok, I've removed patch's dependence on attndims. But I still believe that someday PostgreSQL's type system will be fixed. I looked through the patch briefly and have some other comments: Thank you very much for your review. 1. It's pretty bizarre that you need dummy forward struct declarations for ColumnIOData and RecordIOData. The reason evidently is that somebody ignored project style and put a bunch of typedefs after the local function declarations in jsonfuncs.c. Project style of course is to put the typedefs first, precisely because they may be needed in the local function declarations. I will go move those declarations right now, as a single- purpose patch, so that you have something a bit cleaner to modify. These forward struct declarations were moved to the type declaration section. 2. The business with isstring, saved_scalar_is_string, etc makes me itch. I'm having a hard time explaining exactly why, but it just seems like a single-purpose kluge. Maybe it would seem less so if you saved the original JsonTokenType instead. Original JsonTokenType is saved now. Also "isnull" fields of several structures were replaced by it. But also I'm wondering why the ultimate consumers are concerned with string-ness as such. saved_scalar_is_string was necessary for the case when json string is converted to json/jsonb type. json lexer returns strings with stripped quotes and we must recover them before passing to json_in() or jsonb_in(). There were examples for this case in my first message: [master]=# select * from json_to_record('{"js": "a"}') as rec(js json); ERROR: invalid input syntax for type json DETAIL: Token "a" is invalid. CONTEXT: JSON data, line 1: a [master]=# select * from json_to_record('{"js": "true"}') as rec(js json); js -- true [patched]=# select * from json_to_record('{"js": "a"}') as rec(js json); js - "a" [patched]=# select * from json_to_record('{"js": "true"}') as rec(js json); js "true" It seems like mainly what they're trying to do is forbid converting a string into a non-scalar Postgres type, but (a) why, and (b) if you are going to restrict it, wouldn't it be more sensible to frame it as you can't convert any JSON scalar to a non-scalar type? As to (a), it seems like you're just introducing unnecessary compatibility breakage if you insist on that. As to (b), really it seems like an appropriate restriction of that sort would be like "if target type is composite, source must be a JSON object, and if target type is array, source must be a JSON array". Personally I think what you ought to do here is not change the semantics of cases that are allowed today, but only change the semantics in the cases of JSON object being assigned to PG composite and JSON array being assigned to PG array; both of those fail today, so there's nobody depending on the existing behavior. But if you reject string-to-composite cases then you will be breaking existing apps, and I doubt people will thank you for it. I've removed compatibility-breaking restrictions and now string-to-non-scalar conversions through the input function are allowed. Also I've added corresponding regression test cases. 3. I think you'd be better off declaring ColumnIOData.type_category as an actual enum type, not "char" with some specific values documented only by a comment. Also, could you fold the domain case in as a fourth type_category? I've introduced enum type TypeCat for type categories with domain category as its fourth member. (TypeCategory and JsonTypeCategory names conflict with existing names, you might offer a better name.) Also, why does ColumnIOData have both an ndims field and another one buried in io.array.ndims? Now there are no ndims fields at all, but earlier their values could differ: ColumnIOData.ndims was typically copied from attndims, but ArrayIOData.ndims could be copied from typndims for domain types. 4. populate_array_report_expected_array() violates our error message guidelines, first by using elog rather than ereport for a user-facing error, and second by assembling the message from pieces, which would make it untranslatable even if it were being reported through ereport. I'm not sure if this needs to be fixed though; will the function even still be needed once you remove the attndims dependency? Even if there are still callers, you wouldn't necessarily need such a function if you scale back your ambitions a bit as to the amount of detail required. I'm not sure you really need to report what you think the dimensions are when complaining that an array is nonrectangular. It was my mistake that I was not familiar message-error guidelines. Now ereport() is used and there is no message assembling, but I'm also not
Re: [HACKERS] PATCH: recursive json_populate_record()
Nikita Glukhovwrites: > On 01/08/2017 09:52 PM, Tom Lane wrote: >> ... attndims is really syntactic sugar only and doesn't affect anything >> meaningful semantically. > I have fixed the first patch: when the number of dimensionsis unknown > we determine it simply by the number of successive opening brackets at > the start of a json array. But I'm still using for verification non-zero > ndims values that can be get from composite type attribute (attndims) or > from domain array type (typndims) through specially added function > get_type_ndims(). Apparently I didn't make my point forcefully enough. attndims is not semantically significant in Postgres; the fact that we even store it is just a historical artifact, I think. There might be an argument to start enforcing it, but we'd have to do that across the board, and I think most likely any such proposal would fail because of backwards compatibility concerns. (There would also be a lot of discussion as to whether, say, "int foo[3]" shouldn't enforce that the array length is 3, not just that it be one-dimensional.) In the meantime, we are *not* going to have attndims be semantically significant in just this one function. That would not satisfy the principle of least astonishment. Therefore, please remove this patch's dependence on attndims. I looked through the patch briefly and have some other comments: 1. It's pretty bizarre that you need dummy forward struct declarations for ColumnIOData and RecordIOData. The reason evidently is that somebody ignored project style and put a bunch of typedefs after the local function declarations in jsonfuncs.c. Project style of course is to put the typedefs first, precisely because they may be needed in the local function declarations. I will go move those declarations right now, as a single- purpose patch, so that you have something a bit cleaner to modify. 2. The business with isstring, saved_scalar_is_string, etc makes me itch. I'm having a hard time explaining exactly why, but it just seems like a single-purpose kluge. Maybe it would seem less so if you saved the original JsonTokenType instead. But also I'm wondering why the ultimate consumers are concerned with string-ness as such. It seems like mainly what they're trying to do is forbid converting a string into a non-scalar Postgres type, but (a) why, and (b) if you are going to restrict it, wouldn't it be more sensible to frame it as you can't convert any JSON scalar to a non-scalar type? As to (a), it seems like you're just introducing unnecessary compatibility breakage if you insist on that. As to (b), really it seems like an appropriate restriction of that sort would be like "if target type is composite, source must be a JSON object, and if target type is array, source must be a JSON array". Personally I think what you ought to do here is not change the semantics of cases that are allowed today, but only change the semantics in the cases of JSON object being assigned to PG composite and JSON array being assigned to PG array; both of those fail today, so there's nobody depending on the existing behavior. But if you reject string-to-composite cases then you will be breaking existing apps, and I doubt people will thank you for it. 3. I think you'd be better off declaring ColumnIOData.type_category as an actual enum type, not "char" with some specific values documented only by a comment. Also, could you fold the domain case in as a fourth type_category? Also, why does ColumnIOData have both an ndims field and another one buried in io.array.ndims? 4. populate_array_report_expected_array() violates our error message guidelines, first by using elog rather than ereport for a user-facing error, and second by assembling the message from pieces, which would make it untranslatable even if it were being reported through ereport. I'm not sure if this needs to be fixed though; will the function even still be needed once you remove the attndims dependency? Even if there are still callers, you wouldn't necessarily need such a function if you scale back your ambitions a bit as to the amount of detail required. I'm not sure you really need to report what you think the dimensions are when complaining that an array is nonrectangular. 5. The business with having some arguments that are only for json and others that are only for jsonb, eg in populate_scalar(), also makes me itch. I wonder whether this wouldn't all get a lot simpler and more readable if we gave up on trying to share code between the two cases. In populate_scalar(), for instance, the amount of code actually shared between the two cases amounts to a whole two lines, which are dwarfed by the amount of crud needed to deal with trying to serve both cases in the one function. There are places where there's more sharable code than that, but it seems like a better design might be to refactor the sharable code out into subroutines called by separate functions for json and
Re: [HACKERS] PATCH: recursive json_populate_record()
On 01/08/2017 09:52 PM, Tom Lane wrote: The example you quoted at the start of the thread doesn't fail for me in HEAD, so I surmise that it's falling foul of some assertion you added in the 0001 patch, but if so I think that assertion is wrong. attndims is really syntactic sugar only and doesn't affect anything meaningful semantically. Here is an example showing why it shouldn't: regression=# create table foo (d0 _int4, d1 int[], d2 int[3][4]); CREATE TABLE regression=# select attname,atttypid,attndims from pg_attribute where attrelid = 'foo'::regclass and attnum > 0; attname | atttypid | attndims -+--+-- d0 | 1007 |0 d1 | 1007 |1 d2 | 1007 |2 (3 rows) Columns d0,d1,d2 are really all of the same type, and any code that treats d0 and d1 differently is certainly broken. Thank you for this example with raw _int4 type. I didn't expect that attndims can legally be zero. There was really wrong assertion "ndims > 0" that was necessary because I used attndims for verification of a number of dimensions of a populated json array. I have fixed the first patch: when the number of dimensionsis unknown we determine it simply by the number of successive opening brackets at the start of a json array. But I'm still using for verification non-zero ndims values that can be get from composite type attribute (attndims) or from domain array type (typndims) through specially added function get_type_ndims(). On 01/08/2017 09:52 PM, Tom Lane wrote: I do not see the point of the second one of these, and it adds no test case showing why it would be needed. I also have added special test cases for json_to_record() showing difference in behavior that the second patch brings in (see changes in json.out and jsonb.out). -- Nikita Glukhov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 10e3186..55cacfb 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -11249,12 +11249,12 @@ table2-mapping whose columns match the record type defined by base (see note below). - select * from json_populate_record(null::myrowtype, '{"a":1,"b":2}') + select * from json_populate_record(null::myrowtype, '{"a": 1, "b": ["2", "a b"], "c": {"d": 4, "e": "a b c"}}') - a | b +--- - 1 | 2 + a | b | c +---+---+- + 1 | {2,"a b"} | (4,"a b c") @@ -11343,12 +11343,12 @@ table2-mapping explicitly define the structure of the record with an AS clause. - select * from json_to_record('{"a":1,"b":[1,2,3],"c":"bar"}') as x(a int, b text, d text) + select * from json_to_record('{"a":1,"b":[1,2,3],"c":[1,2,3],"e":"bar","r": {"a": 123, "b": "a b c"}}') as x(a int, b text, c int[], d text, r myrowtype) - a |b| d +-+--- - 1 | [1,2,3] | + a |b|c| d | r +---+-+-+---+--- + 1 | [1,2,3] | {1,2,3} | | (123,"a b c") diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 17ee4e4..04959cb 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -93,7 +93,7 @@ static void elements_array_element_end(void *state, bool isnull); static void elements_scalar(void *state, char *token, JsonTokenType tokentype); /* turn a json object into a hash table */ -static HTAB *get_json_object_as_hash(text *json, const char *funcname); +static HTAB *get_json_object_as_hash(char *json, int len, const char *funcname); /* common worker for populate_record and to_record */ static Datum populate_record_worker(FunctionCallInfo fcinfo, const char *funcname, @@ -149,6 +149,33 @@ static void setPathArray(JsonbIterator **it, Datum *path_elems, int level, Jsonb *newval, uint32 nelems, int op_type); static void addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb); +/* helper functions for populate_record[set] */ +typedef struct ColumnIOData ColumnIOData; +typedef struct RecordIOData RecordIOData; + +static HeapTupleHeader +populate_record(TupleDesc tupdesc, +RecordIOData **record_info, +HeapTupleHeader template, +MemoryContext mcxt, +Oidjtype, +HTAB *json_hash, +JsonbContainer *cont); + +static Datum +populate_record_field(ColumnIOData *col, + Oid typid, + int32 typmod, + int32 ndims, + const char *colname, + MemoryContext mcxt, + Datum defaultval, + Oid jtype, + char *json, + bool json_is_string, + JsonbValue *jval, + bool *isnull); + /* state for json_object_keys */ typedef struct OkeysState { @@ -216,6 +243,7 @@ typedef struct JhashState HTAB *hash; char *saved_scalar; char *save_json_start; + bool saved_scalar_is_string; }
Re: [HACKERS] PATCH: recursive json_populate_record()
Nikita Glukhovwrites: > [ 0001_recursive_json_populate_record_v02.patch ] > [ 0002_assign_ndims_to_record_function_result_types_v02.patch ] I do not see the point of the second one of these, and it adds no test case showing why it would be needed. The example you quoted at the start of the thread doesn't fail for me in HEAD, so I surmise that it's falling foul of some assertion you added in the 0001 patch, but if so I think that assertion is wrong. attndims is really syntactic sugar only and doesn't affect anything meaningful semantically. Here is an example showing why it shouldn't: regression=# create table foo (d0 _int4, d1 int[], d2 int[3][4]); CREATE TABLE regression=# select attname,atttypid,attndims from pg_attribute where attrelid = 'foo'::regclass and attnum > 0; attname | atttypid | attndims -+--+-- d0 | 1007 |0 d1 | 1007 |1 d2 | 1007 |2 (3 rows) Columns d0,d1,d2 are really all of the same type, and any code that treats d0 and d1 differently is certainly broken. So there should be no need to track a particular attndims for an output column of a function result, and in most cases it's not clear to me where you would get an attndims value from anyway. 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] PATCH: recursive json_populate_record()
> I've noticed that this patch is on CF and needs a reviewer so I decided > to take a look. Code looks good to me in general, it's well covered by > tests and passes `make check-world`. Thanks for your review. > However it would be nice to have a little more comments. In my opinion > every procedure have to have at least a short description - what it > does, what arguments it receives and what it returns, even if it's a > static procedure. Same applies for structures and their fields. I have added some comments for functions and structures in the second version of this patch. > Another thing that bothers me is a FIXME comment: > > ``` > tupdesc = lookup_rowtype_tupdesc(type, typmod); /* FIXME cache */ > ``` > > I suggest remove it or implement a caching here as planned. I have implemented tuple descriptor caching here in populate_composite() and also in populate_record_worker() (by using populate_composite() instead of populate_record()). These improvements can speed up bulk jsonb conversion by 15-20% in the simple test with two nested records. -- Nikita Glukhov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 10e3186..55cacfb 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -11249,12 +11249,12 @@ table2-mapping whose columns match the record type defined by base (see note below). - select * from json_populate_record(null::myrowtype, '{"a":1,"b":2}') + select * from json_populate_record(null::myrowtype, '{"a": 1, "b": ["2", "a b"], "c": {"d": 4, "e": "a b c"}}') - a | b +--- - 1 | 2 + a | b | c +---+---+- + 1 | {2,"a b"} | (4,"a b c") @@ -11343,12 +11343,12 @@ table2-mapping explicitly define the structure of the record with an AS clause. - select * from json_to_record('{"a":1,"b":[1,2,3],"c":"bar"}') as x(a int, b text, d text) + select * from json_to_record('{"a":1,"b":[1,2,3],"c":[1,2,3],"e":"bar","r": {"a": 123, "b": "a b c"}}') as x(a int, b text, c int[], d text, r myrowtype) - a |b| d +-+--- - 1 | [1,2,3] | + a |b|c| d | r +---+-+-+---+--- + 1 | [1,2,3] | {1,2,3} | | (123,"a b c") diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 17ee4e4..bb72b8e 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -93,7 +93,7 @@ static void elements_array_element_end(void *state, bool isnull); static void elements_scalar(void *state, char *token, JsonTokenType tokentype); /* turn a json object into a hash table */ -static HTAB *get_json_object_as_hash(text *json, const char *funcname); +static HTAB *get_json_object_as_hash(char *json, int len, const char *funcname); /* common worker for populate_record and to_record */ static Datum populate_record_worker(FunctionCallInfo fcinfo, const char *funcname, @@ -149,6 +149,33 @@ static void setPathArray(JsonbIterator **it, Datum *path_elems, int level, Jsonb *newval, uint32 nelems, int op_type); static void addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb); +/* helper functions for populate_record[set] */ +typedef struct ColumnIOData ColumnIOData; +typedef struct RecordIOData RecordIOData; + +static HeapTupleHeader +populate_record(TupleDesc tupdesc, +RecordIOData **record_info, +HeapTupleHeader template, +MemoryContext mcxt, +Oidjtype, +HTAB *json_hash, +JsonbContainer *cont); + +static Datum +populate_record_field(ColumnIOData *col, + Oid typid, + int32 typmod, + int32 ndims, + const char *colname, + MemoryContext mcxt, + Datum defaultval, + Oid jtype, + char *json, + bool json_is_string, + JsonbValue *jval, + bool *isnull); + /* state for json_object_keys */ typedef struct OkeysState { @@ -216,6 +243,7 @@ typedef struct JhashState HTAB *hash; char *saved_scalar; char *save_json_start; + bool saved_scalar_is_string; } JHashState; /* hashtable element */ @@ -223,26 +251,67 @@ typedef struct JsonHashEntry { char fname[NAMEDATALEN]; /* hash key (MUST BE FIRST) */ char *val; - char *json; bool isnull; + bool isstring; } JsonHashEntry; -/* these two are stolen from hstore / record_out, used in populate_record* */ -typedef struct ColumnIOData +/* structure to cache type I/O metadata needed for populate_scalar() */ +typedef struct ScalarIOData { - Oid column_type; - Oid typiofunc; Oid typioparam; - FmgrInfo proc; -} ColumnIOData; + FmgrInfo typiofunc; +} ScalarIOData; -typedef struct RecordIOData +/* structure to cache metadata needed for populate_array() */ +typedef struct ArrayIOData +{ + Oidelement_type; /* array element
Re: [HACKERS] PATCH: recursive json_populate_record()
Hello. I've noticed that this patch is on CF and needs a reviewer so I decided to take a look. Code looks good to me in general, it's well covered by tests and passes `make check-world`. However it would be nice to have a little more comments. In my opinion every procedure have to have at least a short description - what it does, what arguments it receives and what it returns, even if it's a static procedure. Same applies for structures and their fields. Another thing that bothers me is a FIXME comment: ``` tupdesc = lookup_rowtype_tupdesc(type, typmod); /* FIXME cache */ ``` I suggest remove it or implement a caching here as planned. On Tue, Dec 13, 2016 at 10:07:46AM +0900, Michael Paquier wrote: > On Tue, Dec 13, 2016 at 9:38 AM, Nikita Glukhov> wrote: > > It also fixes the following errors/inconsistencies caused by lost quoting of > > string json values: > > > > [master]=# select * from json_to_record('{"js": "a"}') as rec(js json); > > ERROR: invalid input syntax for type json > > DETAIL: Token "a" is invalid. > > CONTEXT: JSON data, line 1: a > > The docs mention that this is based on a best effort now. It may be > interesting to improve that. > > > [master]=# select * from json_to_record('{"js": "true"}') as rec(js json); > > js > > -- > > true > > > > [patched]=# select * from json_to_record('{"js": "a"}') as rec(js json); > > js > > - > > "a" > > That's indeed valid JSON. > > > [patched]=# select * from json_to_record('{"js": "true"}') as rec(js json); > >js > > > > "true" > > And so is that. > > > The second patch adds assignment of correct ndims to array fields of RECORD > > function result types. > > Without this patch, attndims in tuple descriptors of RECORD types is always > > 0 and the corresponding assertion fails in the next test: > > > > [patched]=# select json_to_record('{"a": [1, 2, 3]}') as rec(a int[]); > > > > > > Should I submit these patches to commitfest? > > It seems to me that it would be a good idea to add them. > -- > Michael > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Best regards, Aleksander Alekseev signature.asc Description: PGP signature
Re: [HACKERS] PATCH: recursive json_populate_record()
On Tue, Dec 13, 2016 at 9:38 AM, Nikita Glukhovwrote: > It also fixes the following errors/inconsistencies caused by lost quoting of > string json values: > > [master]=# select * from json_to_record('{"js": "a"}') as rec(js json); > ERROR: invalid input syntax for type json > DETAIL: Token "a" is invalid. > CONTEXT: JSON data, line 1: a The docs mention that this is based on a best effort now. It may be interesting to improve that. > [master]=# select * from json_to_record('{"js": "true"}') as rec(js json); > js > -- > true > > [patched]=# select * from json_to_record('{"js": "a"}') as rec(js json); > js > - > "a" That's indeed valid JSON. > [patched]=# select * from json_to_record('{"js": "true"}') as rec(js json); >js > > "true" And so is that. > The second patch adds assignment of correct ndims to array fields of RECORD > function result types. > Without this patch, attndims in tuple descriptors of RECORD types is always > 0 and the corresponding assertion fails in the next test: > > [patched]=# select json_to_record('{"a": [1, 2, 3]}') as rec(a int[]); > > > Should I submit these patches to commitfest? It seems to me that it would be a good idea to add them. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PATCH: recursive json_populate_record()
Hi. The first attached patch implements recursive processing of nested objects and arrays in json[b]_populate_record[set](), json[b]_to_record[set](). See regression tests for examples. It also fixes the following errors/inconsistencies caused by lost quoting of string json values: [master]=# select * from json_to_record('{"js": "a"}') as rec(js json); ERROR: invalid input syntax for type json DETAIL: Token "a" is invalid. CONTEXT: JSON data, line 1: a [master]=# select * from json_to_record('{"js": "true"}') as rec(js json); js -- true [patched]=# select * from json_to_record('{"js": "a"}') as rec(js json); js - "a" [patched]=# select * from json_to_record('{"js": "true"}') as rec(js json); js "true" The second patch adds assignment of correct ndims to array fields of RECORD function result types. Without this patch, attndims in tuple descriptors of RECORD types is always 0 and the corresponding assertion fails in the next test: [patched]=# select json_to_record('{"a": [1, 2, 3]}') as rec(a int[]); Should I submit these patches to commitfest? -- Nikita Glukhov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index eca98df..12049a4 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -11249,12 +11249,12 @@ table2-mapping whose columns match the record type defined by base (see note below). - select * from json_populate_record(null::myrowtype, '{"a":1,"b":2}') + select * from json_populate_record(null::myrowtype, '{"a": 1, "b": ["2", "a b"], "c": {"d": 4, "e": "a b c"}}') - a | b +--- - 1 | 2 + a | b | c +---+---+- + 1 | {2,"a b"} | (4,"a b c") @@ -11343,12 +11343,12 @@ table2-mapping explicitly define the structure of the record with an AS clause. - select * from json_to_record('{"a":1,"b":[1,2,3],"c":"bar"}') as x(a int, b text, d text) + select * from json_to_record('{"a":1,"b":[1,2,3],"c":[1,2,3],"e":"bar","r": {"a": 123, "b": "a b c"}}') as x(a int, b text, c int[], d text, r myrowtype) - a |b| d +-+--- - 1 | [1,2,3] | + a |b|c| d | r +---+-+-+---+--- + 1 | [1,2,3] | {1,2,3} | | (123,"a b c") diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 17ee4e4..729826c 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -93,7 +93,7 @@ static void elements_array_element_end(void *state, bool isnull); static void elements_scalar(void *state, char *token, JsonTokenType tokentype); /* turn a json object into a hash table */ -static HTAB *get_json_object_as_hash(text *json, const char *funcname); +static HTAB *get_json_object_as_hash(char *json, int len, const char *funcname); /* common worker for populate_record and to_record */ static Datum populate_record_worker(FunctionCallInfo fcinfo, const char *funcname, @@ -149,6 +149,33 @@ static void setPathArray(JsonbIterator **it, Datum *path_elems, int level, Jsonb *newval, uint32 nelems, int op_type); static void addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb); +/* helper functions for populate_record[set] */ +typedef struct ColumnIOData ColumnIOData; +typedef struct RecordIOData RecordIOData; + +static HeapTupleHeader +populate_record(TupleDesc tupdesc, +RecordIOData **record_info, +HeapTupleHeader template, +MemoryContext mcxt, +Oidjtype, +HTAB *json_hash, +JsonbContainer *cont); + +static Datum +populate_record_field(ColumnIOData *col, + Oid type, + int32 typmod, + int32 ndims, + const char *colname, + MemoryContext mcxt, + Datum defaultval, + Oid jtype, + char *json, + bool json_is_string, + JsonbValue *jval, + bool *isnull); + /* state for json_object_keys */ typedef struct OkeysState { @@ -216,6 +243,7 @@ typedef struct JhashState HTAB *hash; char *saved_scalar; char *save_json_start; + bool saved_scalar_is_string; } JHashState; /* hashtable element */ @@ -223,26 +251,55 @@ typedef struct JsonHashEntry { char fname[NAMEDATALEN]; /* hash key (MUST BE FIRST) */ char *val; - char *json; bool isnull; + bool isstring; } JsonHashEntry; -/* these two are stolen from hstore / record_out, used in populate_record* */ -typedef struct ColumnIOData +typedef struct ScalarIOData { - Oid column_type; Oid typiofunc; Oid typioparam; FmgrInfo proc; -} ColumnIOData; +} ScalarIOData; -typedef struct RecordIOData +typedef struct ArrayIOData +{ + Oidelement_type; + int32 element_typmod; + ColumnIOData *element_info; + intndims; +} ArrayIOData; + +/* + * CompositeIOData is a pointer to a