Re: Jsonb transform for pl/python
On 27/09/2018 16:58, Nikita Glukhov wrote: > Working on the new lazy transform for jsonb I found another memory leak in > PLyObject_ToJsonbValue(): palloc() for output boolean JsonbValue is > unnecessary, > 'out' variable is already initialized. > > Fix is attached. Committed, thanks. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Jsonb transform for pl/python
Working on the new lazy transform for jsonb I found another memory leak in PLyObject_ToJsonbValue(): palloc() for output boolean JsonbValue is unnecessary, 'out' variable is already initialized. Fix is attached. On 15.06.2018 14:42, Alexander Korotkov wrote: Hi! On Fri, Jun 15, 2018 at 2:11 PM Nikita Glukhov wrote: I found a memory leak in PLySequence_ToJsonbValue(): PyObject returned from PySequence_GetItem() is not released. A bug can be easily reproduced using function roudtrip() from regression test: SELECT roundtrip('[1,2,3]'::jsonb) FROM generate_series(1, 100); Similar code in PLyMapping_ToJsonbValue() seems to be correct because PyList_GetItem() and PyTuple_GetItem() return a borrowed reference. Patch with fix is attached. I'm going to check and commit this if everything is OK. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Nikita Glukhov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company >From 06e22b5ff12486917259270c8f2a95b1a5d7cee2 Mon Sep 17 00:00:00 2001 From: Nikita Glukhov Date: Tue, 7 Aug 2018 13:37:10 +0300 Subject: [PATCH] Fix memory leak in PLyObject_ToJsonbValue() --- contrib/jsonb_plpython/jsonb_plpython.c | 1 - 1 file changed, 1 deletion(-) diff --git a/contrib/jsonb_plpython/jsonb_plpython.c b/contrib/jsonb_plpython/jsonb_plpython.c index d6d6eeb..f44d364 100644 --- a/contrib/jsonb_plpython/jsonb_plpython.c +++ b/contrib/jsonb_plpython/jsonb_plpython.c @@ -398,7 +398,6 @@ PLyObject_ToJsonbValue(PyObject *obj, JsonbParseState **jsonb_state, bool is_ele */ else if (PyBool_Check(obj)) { - out = palloc(sizeof(JsonbValue)); out->type = jbvBool; out->val.boolean = (obj == Py_True); } -- 2.7.4
Re: Jsonb transform for pl/python
On Wed, Jul 11, 2018 at 12:30 AM Tom Lane wrote: > Peter Eisentraut writes: > > On 6/23/18 01:44, Nikita Glukhov wrote: > >> We are simply trying first to convert numeric to int64 if is does not have > >> digits after the decimal point, and then construct Python Int instead of > >> Decimal. Standard Python json.loads() does the same for exact integers. > > > We just had a very similar but not identical discussion in the thread > > about the PL/Perl transforms, where we rejected the proposal. Other > > comments on this one? > > I can sympathize with the speed concern, but the proposed patch produces > a functional change (ie, you get a different kind of Python object, with > different behaviors, in some cases). That seems to me to be a good reason > to reject it. Nevertheless, as Nikita pointed, json.loads() performs the same in Python. See an example for python 2.7.10. >>> import json >>> type(json.loads('1')) >>> type(json.loads('1.0')) So, from postgres point of view this behavior is wrong. But on another hand why don't let python transform to behave like a python? -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Jsonb transform for pl/python
Peter Eisentraut writes: > On 6/23/18 01:44, Nikita Glukhov wrote: >> We are simply trying first to convert numeric to int64 if is does not have >> digits after the decimal point, and then construct Python Int instead of >> Decimal. Standard Python json.loads() does the same for exact integers. > We just had a very similar but not identical discussion in the thread > about the PL/Perl transforms, where we rejected the proposal. Other > comments on this one? I can sympathize with the speed concern, but the proposed patch produces a functional change (ie, you get a different kind of Python object, with different behaviors, in some cases). That seems to me to be a good reason to reject it. The fact that it makes the behavior vary depending on the local width of "long" is also a problem, although I think that could be fixed. More generally, I'd be happier with a patch that sped things up for non-integers too. I don't know whether Python exposes enough internals of type Decimal to make that practical, though. One idea for doing it at arm's length is to compute the Decimal value using numeric-digit-at-a-time arithmetic, roughly x := 0; foreach(digit, numeric) x.fma(1, digit); x.scaleb(appropriate-exponent); In principle that's probably faster than string conversion, but not sure by how much. regards, tom lane
Re: Jsonb transform for pl/python
On 6/23/18 01:44, Nikita Glukhov wrote: > We are simply trying first to convert numeric to int64 if is does not have > digits after the decimal point, and then construct Python Int instead of > Decimal. Standard Python json.loads() does the same for exact integers. We just had a very similar but not identical discussion in the thread about the PL/Perl transforms, where we rejected the proposal. Other comments on this one? In any case, this might well be a consideration for PG12, not a bug in the existing implementation. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Jsonb transform for pl/python
Hi! On Fri, Jun 15, 2018 at 2:11 PM Nikita Glukhov wrote: > > On 28.03.2018 15:43, Peter Eisentraut wrote: > > > On 3/21/18 18:50, Peter Eisentraut wrote: > >> On 3/12/18 11:26, Nikita Glukhov wrote: > >>> I have reviewed this patch. Attached new 6th version of the patch with > >>> v5-v6 delta-patch. > >> Thanks for the update. I'm working on committing this. > > Committed. > > > > Everything seemed to work well. I just did a bit of cosmetic cleanups. > > I did a fair amount of tweaking on the naming of functions, the > > extensions and library names, etc., to make it look like the existing > > plpython transforms. I also changed it so that the transform would > > support mappings and sequences other than dict and list. I removed the > > non-ASCII test and the test with the huge numbers. > > > I found a memory leak in PLySequence_ToJsonbValue(): > PyObject returned from PySequence_GetItem() is not released. > > A bug can be easily reproduced using function roudtrip() from regression test: > SELECT roundtrip('[1,2,3]'::jsonb) FROM generate_series(1, 100); > > Similar code in PLyMapping_ToJsonbValue() seems to be correct because > PyList_GetItem() and PyTuple_GetItem() return a borrowed reference. > > Patch with fix is attached. I'm going to check and commit this if everything is OK. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Jsonb transform for pl/python
On 28.03.2018 15:43, Peter Eisentraut wrote: On 3/21/18 18:50, Peter Eisentraut wrote: On 3/12/18 11:26, Nikita Glukhov wrote: I have reviewed this patch. Attached new 6th version of the patch with v5-v6 delta-patch. Thanks for the update. I'm working on committing this. Committed. Everything seemed to work well. I just did a bit of cosmetic cleanups. I did a fair amount of tweaking on the naming of functions, the extensions and library names, etc., to make it look like the existing plpython transforms. I also changed it so that the transform would support mappings and sequences other than dict and list. I removed the non-ASCII test and the test with the huge numbers. I found a memory leak in PLySequence_ToJsonbValue(): PyObject returned from PySequence_GetItem() is not released. A bug can be easily reproduced using function roudtrip() from regression test: SELECT roundtrip('[1,2,3]'::jsonb) FROM generate_series(1, 100); Similar code in PLyMapping_ToJsonbValue() seems to be correct because PyList_GetItem() and PyTuple_GetItem() return a borrowed reference. Patch with fix is attached. -- Nikita Glukhov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company >From 53ab2aa2f345d7c4f5924b2e327b2d94c6f2c765 Mon Sep 17 00:00:00 2001 From: Nikita Glukhov Date: Fri, 15 Jun 2018 02:58:40 +0300 Subject: [PATCH] Fix memory leak in contrib/jsonb_plpython --- contrib/jsonb_plpython/jsonb_plpython.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contrib/jsonb_plpython/jsonb_plpython.c b/contrib/jsonb_plpython/jsonb_plpython.c index f752d6c..d6d6eeb 100644 --- a/contrib/jsonb_plpython/jsonb_plpython.c +++ b/contrib/jsonb_plpython/jsonb_plpython.c @@ -308,6 +308,8 @@ PLySequence_ToJsonbValue(PyObject *obj, JsonbParseState **jsonb_state) PyObject *value = PySequence_GetItem(obj, i); (void) PLyObject_ToJsonbValue(value, jsonb_state, true); + + Py_XDECREF(value); } return pushJsonbValue(jsonb_state, WJB_END_ARRAY, NULL); -- 2.7.4
Re: Jsonb transform for pl/python
On 3/21/18 18:50, Peter Eisentraut wrote: > On 3/12/18 11:26, Nikita Glukhov wrote: >> I have reviewed this patch. Attached new 6th version of the patch with >> v5-v6 delta-patch. > > Thanks for the update. I'm working on committing this. Committed. Everything seemed to work well. I just did a bit of cosmetic cleanups. I did a fair amount of tweaking on the naming of functions, the extensions and library names, etc., to make it look like the existing plpython transforms. I also changed it so that the transform would support mappings and sequences other than dict and list. I removed the non-ASCII test and the test with the huge numbers. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Jsonb transform for pl/python
On 3/12/18 11:26, Nikita Glukhov wrote: > I have reviewed this patch. Attached new 6th version of the patch with > v5-v6 delta-patch. Thanks for the update. I'm working on committing this. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Jsonb transform for pl/python
On 01.02.2018 19:06, Peter Eisentraut wrote: On 1/12/18 10:43, Aleksander Alekseev wrote: The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed LGTM. The new status of this patch is: Ready for Committer I've been working on polishing this a bit. I'll keep working on it. It should be ready to commit soon. Hi. I have reviewed this patch. Attached new 6th version of the patch with v5-v6 delta-patch. * Added out of memory checks after the following function calls: - PyList_New() - PyDict_New() - PyString_FromStringAndSize() (added PyString_FromJsonbValue()) * Added Py_XDECREF() for key-value pairs and list elements after theirs appending because PyDict_SetItem() and PyList_Append() do not steal references (see also hstore_plpython code). * Removed unnecessary JsonbValue heap-allocations in PyObject_ToJsonbValue(). * Added iterating to the end of iterator in PyObject_FromJsonb() for correct freeing of JsonbIterators. * Passed JsonbParseState ** to PyXxx_ToJsonbValue() functions. * Added transformation of Python tuples into JSON arrays because standard Python JSONEncoder encoder does the same. (See https://docs.python.org/2/library/json.html#py-to-json-table) -- Nikita Glukhov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company 0001-jsonb_plpython-extension-v6.patch.gz Description: application/gzip diff --git a/contrib/jsonb_plpython/expected/jsonb_plpython2u.out b/contrib/jsonb_plpython/expected/jsonb_plpython2u.out index 7073d52..7d29589 100644 --- a/contrib/jsonb_plpython/expected/jsonb_plpython2u.out +++ b/contrib/jsonb_plpython/expected/jsonb_plpython2u.out @@ -341,8 +341,22 @@ SELECT testDecimal(); 255 (1 row) +-- tuple -> jsonb +CREATE FUNCTION testTuple() RETURNS jsonb +LANGUAGE plpython2u +TRANSFORM FOR TYPE jsonb +AS $$ +x = (1, 'String', None) +return x +$$; +SELECT testTuple(); + testtuple +- + [1, "String", null] +(1 row) + DROP EXTENSION plpython2u CASCADE; -NOTICE: drop cascades to 17 other objects +NOTICE: drop cascades to 18 other objects DETAIL: drop cascades to extension jsonb_plpython2u drop cascades to function test1(jsonb) drop cascades to function test1complex(jsonb) @@ -360,6 +374,7 @@ drop cascades to function test1set() drop cascades to function testcomplexnumbers() drop cascades to function testrange() drop cascades to function testdecimal() +drop cascades to function testtuple() -- test jsonb int -> python int CREATE EXTENSION jsonb_plpythonu CASCADE; NOTICE: installing required extension "plpythonu" diff --git a/contrib/jsonb_plpython/expected/jsonb_plpython3u.out b/contrib/jsonb_plpython/expected/jsonb_plpython3u.out index 5acd45c..2492858 100644 --- a/contrib/jsonb_plpython/expected/jsonb_plpython3u.out +++ b/contrib/jsonb_plpython/expected/jsonb_plpython3u.out @@ -340,8 +340,22 @@ SELECT testDecimal(); 255 (1 row) +-- tuple -> jsonb +CREATE FUNCTION testTuple() RETURNS jsonb +LANGUAGE plpython2u +TRANSFORM FOR TYPE jsonb +AS $$ +x = (1, 'String', None) +return x +$$; +SELECT testTuple(); + testtuple +- + [1, "String", null] +(1 row) + DROP EXTENSION plpython3u CASCADE; -NOTICE: drop cascades to 17 other objects +NOTICE: drop cascades to 18 other objects DETAIL: drop cascades to extension jsonb_plpython3u drop cascades to function test1(jsonb) drop cascades to function test1complex(jsonb) @@ -359,3 +373,4 @@ drop cascades to function test1set() drop cascades to function testcomplexnumbers() drop cascades to function testrange() drop cascades to function testdecimal() +drop cascades to function testtuple() diff --git a/contrib/jsonb_plpython/jsonb_plpython.c b/contrib/jsonb_plpython/jsonb_plpython.c index 28f7a3f..705e82f 100644 --- a/contrib/jsonb_plpython/jsonb_plpython.c +++ b/contrib/jsonb_plpython/jsonb_plpython.c @@ -21,7 +21,7 @@ static PyObject *decimal_constructor; static PyObject *PyObject_FromJsonb(JsonbContainer *jsonb); static JsonbValue *PyObject_ToJsonbValue(PyObject *obj, - JsonbParseState *jsonb_state); + JsonbParseState **jsonb_state, bool is_elem); #if PY_MAJOR_VERSION >= 3 typedef PyObject *(*PLyUnicode_FromStringAndSize_t) @@ -53,6 +53,43 @@ _PG_init(void) #define PLyUnicode_FromStringAndSize (PLyUnicode_FromStringAndSize_p) /* + * PyString_FromJsonbValue + * + * Transform string JsonbValue into python string + */ +static PyObject * +PyString_FromJsonbValue(JsonbValue *jbv) +{ + PyObject *str; + + Assert(jbv->type == jbvString); + + str = PyString_FromStringAndSize(jbv->val.string.val, jbv->val.string.len); + + if (!str) + ereport(ERROR, +(errcode(ERRCODE_OUT_OF_MEMORY), + errmsg("out of memory"))); + + return str; +} + +/* + * PyString_ToJsonbValue + * + * Transform
Re: Jsonb transform for pl/python
On 1/12/18 10:43, Aleksander Alekseev wrote: > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: tested, passed > Documentation:tested, passed > > LGTM. > > The new status of this patch is: Ready for Committer I've been working on polishing this a bit. I'll keep working on it. It should be ready to commit soon. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Jsonb transform for pl/python
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed LGTM. The new status of this patch is: Ready for Committer
Re: Jsonb transform for pl/python
On Thu, Dec 7, 2017 at 12:40 AM, Anthony Bykovwrote: > Hello, > fixed the issues: > 1. Rising errors when invalid object being transformed. > 2. We don't rise the exception when transforming range(3) only in > python 2. In third one it is an error. > > Please, find the 4-th version of the patch in attachments to this > message. -- Hi Anthony, FYI make docs fails: json.sgml:584: parser error : Opening and ending tag mismatch: xref line 581 and para ^ json.sgml:585: parser error : Opening and ending tag mismatch: para line 575 and sect2 ^ json.sgml:588: parser error : Opening and ending tag mismatch: sect2 line 572 and sect1 ^ json.sgml:589: parser error : Premature end of data in tag sect1 line 3 json.sgml:589: parser error : chunk is not well balanced datatype.sgml:4354: parser error : Failure to process entity json ^ datatype.sgml:4354: parser error : Entity 'json' not defined ^ datatype.sgml:4955: parser error : chunk is not well balanced postgres.sgml:104: parser error : Failure to process entity datatype ^ postgres.sgml:104: parser error : Entity 'datatype' not defined ^ -- Thomas Munro http://www.enterprisedb.com
Re: Jsonb transform for pl/python
On 12/11/17 03:22, Anthony Bykov wrote: >> Why not make this part of the plpythonu extension? It doesn't have to >> be a separate contrib module. >> > Hello, > I thought about that, but the problem is that there will be no > possibilities to create custom transform if we create this extension by > default. OK, could it be a separate extension, but part of the same code directory? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Jsonb transform for pl/python
On Sat, 9 Dec 2017 16:57:05 -0500 Peter Eisentrautwrote: > On 12/6/17 06:40, Anthony Bykov wrote: > > Hello, > > fixed the issues: > > 1. Rising errors when invalid object being transformed. > > 2. We don't rise the exception when transforming range(3) only in > > python 2. In third one it is an error. > > > > Please, find the 4-th version of the patch in attachments to this > > message. > > Why not make this part of the plpythonu extension? It doesn't have to > be a separate contrib module. > Hello, I thought about that, but the problem is that there will be no possibilities to create custom transform if we create this extension by default. For example, it is easy to check if we install this extension and try to create new transform: # create extension jsonb_plperl cascade; NOTICE: installing required extension "plperl" CREATE EXTENSION # CREATE TRANSFORM FOR jsonb LANGUAGE plperl ( # FROM SQL WITH FUNCTION jsonb_to_plperl(internal), # TO SQL WITH FUNCTION plperl_to_jsonb(internal) # ); 2017-12-11 10:23:07.507 MSK [19149] ERROR: transform for type jsonb language "plperl" already exists 2017-12-11 10:23:07.507 MSK [19149] STATEMENT: CREATE TRANSFORM FOR jsonb LANGUAGE plperl ( FROM SQL WITH FUNCTION jsonb_to_plperl(internal), TO SQL WITH FUNCTION plperl_to_jsonb(internal) ); ERROR: transform for type jsonb language "plperl" already exists Other types of transforms may be interesting for people when they want to transform the jsonb to certain structures. For example, what if the user wants the parameter to be always array inside the function, while this extension can return integers or strings in some cases. -- Anthony Bykov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Jsonb transform for pl/python
On 12/6/17 06:40, Anthony Bykov wrote: > Hello, > fixed the issues: > 1. Rising errors when invalid object being transformed. > 2. We don't rise the exception when transforming range(3) only in > python 2. In third one it is an error. > > Please, find the 4-th version of the patch in attachments to this > message. Why not make this part of the plpythonu extension? It doesn't have to be a separate contrib module. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Jsonb transform for pl/python
On Mon, Nov 20, 2017 at 10:48 PM, Aleksander Alekseevwrote: > Well frankly I very much doubt that this: > [snip] > I'm afraid that tests fail on Python 3: So this still needs more work.. I am marking it as returned with feedback as there has been no updates for more than 1 week. -- Michael
Re: Jsonb transform for pl/python
Hi Anthony, > thank you for your review. I took your comments into account in the > third version of the patch. In the new version, I've added all the > tests you asked for. The interesting thing is that: > 1. set or any other non-jsonb-transformable object is transformed into > string and added to jsonb as a string. Well frankly I very much doubt that this: ``` +-- set -> jsonb +CREATE FUNCTION test1set() RETURNS jsonb +LANGUAGE plpython2u +TRANSFORM FOR TYPE jsonb +AS $$ +x = set() +x.add(1) +x.add("string") +x.add(None) +return x +$$; +SELECT test1set(); + test1set + + "set([1, 'string', None])" +(1 row) ``` ... is an expected and valid behavior. If user tries to transform a set() to JSONB this is most likely a mistake since there is no standard representation of a set() in JSONB. I believe we should rise an error in this case instead of generating a string. Besides user can expect that such string can be transformed back to set() which doesn't sound like a good idea either. If necessary, user can just transform a set() to a list(): ``` >>> x = set([1,2,3,4]) >>> x {1, 2, 3, 4} >>> list(x) [1, 2, 3, 4] ``` BTW I just recalled that Python supports complex numbers out-of-the box and that range and xrange are a separate types too: ``` >>> 1 + 2j (1+2j) >>> range(3) range(0, 3) >>> type(range(3)) ``` I think we should add all this to the tests as well. Naturally complex numbers can't be represented in JSON so we should rise an error if user tries to transform a complex number to JSON. I'm not that sure regarding ranges though. Probably the best solution will be to rise and error in this case as well just to keep things consistent. > +ERROR: jsonb doesn't support inf type yet I would say this error message is too informal. How about something more like "Infinity can't be represented in JSONB"? > 2. couldn't find a solution of working with "inf": this extension > troughs exception if python generates a number called "inf" and returns > it, but if we pass a very large integer into a function, it works fine > with the whole number. This situation can be seen in tests. > > I've added tests of large numerics which weights quite heavy. So, > please find it in compressed format in attachments. I'm afraid that tests fail on Python 3: ``` SELECT test1set(); test1set --- ! "{None, 1, 'string'}" (1 row) DROP EXTENSION plpython3u CASCADE; --- 296,302 SELECT test1set(); test1set --- ! "{1, None, 'string'}" (1 row) DROP EXTENSION plpython3u CASCADE ``` -- Best regards, Aleksander Alekseev signature.asc Description: PGP signature
Re: Jsonb transform for pl/python
On Mon, 13 Nov 2017 15:08:16 + Aleksander Alekseevwrote: > The following review has been posted through the commitfest > application: make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: tested, passed > Documentation:tested, passed > > Hi Anthony, > > Thank you for the new version of the patch! Here is my code review. > > 1. In jsonb_plpython2.out: > > +CREATE FUNCTION back(val jsonb) RETURNS jsonb > +LANGUAGE plpython2u > +TRANSFORM FOR TYPE jsonb > +as $$ > +return val > +$$; > +SELECT back('null'::jsonb); > + back > + > + [null] > +(1 row) > + > +SELECT back('1'::jsonb); > + back > +-- > + [1] > +(1 row) > + > +SELECT back('true'::jsonb); > + back > + > + [true] > +(1 row) > > Maybe I'm missing something, but why exactly all JSONB values turn > into arrays? > > 2. Could you please also add tests for some negative and real > numbers? Also could you check that your code handles numbers like > MAX_INT, MIN_INT, +/- infinity and NaN properly in both (Python <-> > JSONB) directions? > > 3. Handling unicode strings properly is another thing that is worth > checking. > > 4. I think we also need some tests that check the behavior of Python > -> JSONB conversion when the object contains data that is not > representable in JSON format, e.g. set(), some custom objects, etc. > > 5. PyObject_FromJsonbValue - I realize it's unlikely that the new > jsonbValue->type will be introduced any time soon. Still I believe > it's a good practice to add "it should never happen" default case > that just does elog(ERROR, ...) in case it happens nevertheless. > Otherwise in this scenario instead of reporting the error the code > will silently do the wrong thing. > > 6. Well, you decided to make the extension non-relocatable. Could you > at least describe what prevents it to be relocatable or why it's > meaningless is a comment in .control file? Please note that almost > all contrib/ extensions are relocatable. I believe your extension > should be relocatable as well unless there is a good reason why it > can't. > > The new status of this patch is: Waiting on Author Hi, thank you for your review. I took your comments into account in the third version of the patch. In the new version, I've added all the tests you asked for. The interesting thing is that: 1. set or any other non-jsonb-transformable object is transformed into string and added to jsonb as a string. 2. couldn't find a solution of working with "inf": this extension troughs exception if python generates a number called "inf" and returns it, but if we pass a very large integer into a function, it works fine with the whole number. This situation can be seen in tests. I've added tests of large numerics which weights quite heavy. So, please find it in compressed format in attachments. -- Anthony Bykov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company 0001-jsonb_plpython-extension-v3.patch.gz Description: application/gzip
Re: Jsonb transform for pl/python
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed Hi Anthony, Thank you for the new version of the patch! Here is my code review. 1. In jsonb_plpython2.out: +CREATE FUNCTION back(val jsonb) RETURNS jsonb +LANGUAGE plpython2u +TRANSFORM FOR TYPE jsonb +as $$ +return val +$$; +SELECT back('null'::jsonb); + back + + [null] +(1 row) + +SELECT back('1'::jsonb); + back +-- + [1] +(1 row) + +SELECT back('true'::jsonb); + back + + [true] +(1 row) Maybe I'm missing something, but why exactly all JSONB values turn into arrays? 2. Could you please also add tests for some negative and real numbers? Also could you check that your code handles numbers like MAX_INT, MIN_INT, +/- infinity and NaN properly in both (Python <-> JSONB) directions? 3. Handling unicode strings properly is another thing that is worth checking. 4. I think we also need some tests that check the behavior of Python -> JSONB conversion when the object contains data that is not representable in JSON format, e.g. set(), some custom objects, etc. 5. PyObject_FromJsonbValue - I realize it's unlikely that the new jsonbValue->type will be introduced any time soon. Still I believe it's a good practice to add "it should never happen" default case that just does elog(ERROR, ...) in case it happens nevertheless. Otherwise in this scenario instead of reporting the error the code will silently do the wrong thing. 6. Well, you decided to make the extension non-relocatable. Could you at least describe what prevents it to be relocatable or why it's meaningless is a comment in .control file? Please note that almost all contrib/ extensions are relocatable. I believe your extension should be relocatable as well unless there is a good reason why it can't. The new status of this patch is: Waiting on Author