Transform for pl/perl

Hello.
Please, check out jsonb transform
(https://www.postgresql.org/docs/9.5/static/sql-createtransform.html)
for pl/perl language I've implemented.

There are some moments I should mention:
1. {"1":1}::jsonb is transformed into HV {"1"=>"1"}, while
["1","2"]::jsonb is transformed into AV ["1", "2"]

2. If there is a numeric value appear in jsonb, it will be transformed
to SVnv through string (Numeric->String->SV->SVnv). Not the best
solution, but as far as I understand this is usual practise in
postgresql to serialize Numerics and de-serialize them.

3. SVnv is transformed into jsonb through string
(SVnv->String->Numeric).

An example may also be helpful to understand extension. So, as an
example, function "test" transforms incoming jsonb into perl,
transforms it back into jsonb and returns it.

create extension jsonb_plperl cascade;

create or replace function test(val jsonb)
returns jsonb
transform for type jsonb
language plperl
as $$
return $_[0];
$$;

select test('{"1":1,"example": null}'::jsonb);

On Tue, Oct 24, 2017 at 1:01 PM, anthony <a(dot)bykov(at)postgrespro(dot)ru> wrote:
> Hello.
> Please, check out jsonb transform
> (https://www.postgresql.org/docs/9.5/static/sql-createtransform.html)
> for pl/perl language I've implemented.

Neat.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi

2017-10-24 14:27 GMT+02:00 Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru>:

> There are some moments I should mention:
> 1. {"1":1}::jsonb is transformed into HV {"1"=>"1"}, while
> ["1","2"]::jsonb is transformed into AV ["1", "2"]
>
> 2. If there is a numeric value appear in jsonb, it will be transformed
> to SVnv through string (Numeric->String->SV->SVnv). Not the best
> solution, but as far as I understand this is usual practise in
> postgresql to serialize Numerics and de-serialize them.
>
> 3. SVnv is transformed into jsonb through string
> (SVnv->String->Numeric).
>
> An example may also be helpful to understand extension. So, as an
> example, function "test" transforms incoming jsonb into perl,
> transforms it back into jsonb and returns it.
>
> create extension jsonb_plperl cascade;
>
> create or replace function test(val jsonb)
> returns jsonb
> transform for type jsonb
> language plperl
> as $$
> return $_[0];
> $$;
>
> select test('{"1":1,"example": null}'::jsonb);
>
>
I am looking to this patch:

1. the patch contains some artefacts - look the word "hstore"

2. I got lot of warnings

make[1]: Vstupuje se do adresáře
„/home/pavel/src/postgresql/contrib/jsonb_plperl“
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-g -ggdb -Og -g3 -fno-omit-frame-pointer -fPIC -I../../src/pl/plperl -I.
-I. -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2
-I/usr/lib64/perl5/CORE -c -o jsonb_plperl.o jsonb_plperl.c
jsonb_plperl.c: In function ‘SV_FromJsonbValue’:
jsonb_plperl.c:83:9: warning: ‘result’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
return (result);
^
jsonb_plperl.c: In function ‘SV_FromJsonb’:
jsonb_plperl.c:95:10: warning: ‘object’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
HV *object;
^~~~~~
In file included from /usr/lib64/perl5/CORE/perl.h:5644:0,
from ../../src/pl/plperl/plperl.h:52,
from jsonb_plperl.c:17:
/usr/lib64/perl5/CORE/embed.h:404:19: warning: ‘value’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
#define newRV(a) Perl_newRV(aTHX_ a)
^~~~~~~~~~
jsonb_plperl.c:101:10: note: ‘value’ was declared here
SV *value;
^~~~~
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-g -ggdb -Og -g3 -fno-omit-frame-pointer -fPIC -shared -o jsonb_plperl.so
jsonb_plperl.o -L../../src/port -L../../src/common -Wl,--as-needed
-Wl,-rpath,'/usr/lib64/perl5/CORE',--enable-new-dtags -Wl,-z,relro
-specs=/usr/lib/rpm/redhat/redhat-hardened-ld -fstack-protector-strong
-L/usr/local/lib -L/usr/lib64/perl5/CORE -lperl -lpthread -lresolv -lnsl
-ldl -lm -lcrypt -lutil -lc
make[1]: Opouští se adresář
„/home/pavel/src/postgresql/contrib/jsonb_plperl“

[pavel(at)nemesis contrib]$ gcc --version
gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2)
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

3. regress tests passed

4. There are not any documentation - probably it should be part of PLPerl

5. The regress tests doesn't coverage other datatypes than numbers. I miss
boolean, binary, object, ... Maybe using data::dumper or some similar can
be interesting

Note - it is great extension, I am pleasured so transformations are used.

Regards

Pavel

>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On Fri, 10 Nov 2017 14:40:21 +0100
Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:

> Hi
>
> 2017-10-24 14:27 GMT+02:00 Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru>:
>
> > There are some moments I should mention:
> > 1. {"1":1}::jsonb is transformed into HV {"1"=>"1"}, while
> > ["1","2"]::jsonb is transformed into AV ["1", "2"]
> >
> > 2. If there is a numeric value appear in jsonb, it will be
> > transformed to SVnv through string (Numeric->String->SV->SVnv). Not
> > the best solution, but as far as I understand this is usual
> > practise in postgresql to serialize Numerics and de-serialize them.
> >
> > 3. SVnv is transformed into jsonb through string
> > (SVnv->String->Numeric).
> >
> > An example may also be helpful to understand extension. So, as an
> > example, function "test" transforms incoming jsonb into perl,
> > transforms it back into jsonb and returns it.
> >
> > create extension jsonb_plperl cascade;
> >
> > create or replace function test(val jsonb)
> > returns jsonb
> > transform for type jsonb
> > language plperl
> > as $$
> > return $_[0];
> > $$;
> >
> > select test('{"1":1,"example": null}'::jsonb);
> >
> >
> I am looking to this patch:
>
> 1. the patch contains some artefacts - look the word "hstore"
>
> 2. I got lot of warnings
>
>
> make[1]: Vstupuje se do adresáře
> „/home/pavel/src/postgresql/contrib/jsonb_plperl“
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3
> -fno-omit-frame-pointer -fPIC -I../../src/pl/plperl -I. -I.
> -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2
> -I/usr/lib64/perl5/CORE -c -o jsonb_plperl.o jsonb_plperl.c
> jsonb_plperl.c: In function ‘SV_FromJsonbValue’: jsonb_plperl.c:83:9:
> warning: ‘result’ may be used uninitialized in this function
> [-Wmaybe-uninitialized] return (result);
> ^
> jsonb_plperl.c: In function ‘SV_FromJsonb’:
> jsonb_plperl.c:95:10: warning: ‘object’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
> HV *object;
> ^~~~~~
> In file included from /usr/lib64/perl5/CORE/perl.h:5644:0,
> from ../../src/pl/plperl/plperl.h:52,
> from jsonb_plperl.c:17:
> /usr/lib64/perl5/CORE/embed.h:404:19: warning: ‘value’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> #define newRV(a) Perl_newRV(aTHX_ a)
> ^~~~~~~~~~
> jsonb_plperl.c:101:10: note: ‘value’ was declared here
> SV *value;
> ^~~~~
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3
> -fno-omit-frame-pointer -fPIC -shared -o jsonb_plperl.so
> jsonb_plperl.o -L../../src/port -L../../src/common -Wl,--as-needed
> -Wl,-rpath,'/usr/lib64/perl5/CORE',--enable-new-dtags -Wl,-z,relro
> -specs=/usr/lib/rpm/redhat/redhat-hardened-ld
> -fstack-protector-strong -L/usr/local/lib -L/usr/lib64/perl5/CORE
> -lperl -lpthread -lresolv -lnsl -ldl -lm -lcrypt -lutil -lc make[1]:
> Opouští se adresář „/home/pavel/src/postgresql/contrib/jsonb_plperl“
>
> [pavel(at)nemesis contrib]$ gcc --version
> gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2)
> Copyright (C) 2017 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There
> is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
> PARTICULAR PURPOSE.
>
> 3. regress tests passed
>
> 4. There are not any documentation - probably it should be part of
> PLPerl
>
> 5. The regress tests doesn't coverage other datatypes than numbers. I
> miss boolean, binary, object, ... Maybe using data::dumper or some
> similar can be interesting
>
> Note - it is great extension, I am pleasured so transformations are
> used.
>
> Regards
>
> Pavel
> >
> > --
> > Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
> >

Hello,
Thank you for your review. I have fixed most of your comments, except
for the 5-th part, about data::dumper - I just couldn't understand
your point, but I've added more tests with more complex objects if this
helps.

Please, take a look at new patch. You can find it in attachments to
this message (it is called "0001-jsonb_plperl-extension-v2.patch")
--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi

> Hello,
> Thank you for your review. I have fixed most of your comments, except
> for the 5-th part, about data::dumper - I just couldn't understand
> your point, but I've added more tests with more complex objects if this
> helps.
>
> Please, take a look at new patch. You can find it in attachments to
> this message (it is called "0001-jsonb_plperl-extension-v2.patch")
>

I changed few lines in regress tests.

Now, I am think so this patch is ready for commiters.

1. all tests passed
2. there are some basic documentation

I have not any other objections

Regards

Pavel

> --
> Anthony Bykov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

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

Hello Anthony,

Great patch! Everything is OK and I almost agree with Pavel.

The only thing that I would like to suggest is to add a little more tests for
various corner cases. For instance:

1. Converting in both directions (Perl <-> JSONB) +/- infinity, NaN, MAX_INT,
MIN_INT.

2. Converting in both directions strings that contain non-ASCII (Russian /
Japanese / etc) characters and special characters like \n, \t, \.

3. Make sure that converting Perl objects that are not representable in JSONB
(blessed hashes, file descriptors, regular expressions, ...) doesn't crash
everything and shows a reasonable error message.

The new status of this patch is: Waiting on Author

On 14 Nov 2017 11:35, "Anthony Bykov" <a(dot)bykov(at)postgrespro(dot)ru> wrote:

On Fri, 10 Nov 2017 14:40:21 +0100
Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:

> Hi
>
> 2017-10-24 14:27 GMT+02:00 Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru>:
>
> > There are some moments I should mention:
> > 1. {"1":1}::jsonb is transformed into HV {"1"=>"1"}, while
> > ["1","2"]::jsonb is transformed into AV ["1", "2"]
> >
> > 2. If there is a numeric value appear in jsonb, it will be
> > transformed to SVnv through string (Numeric->String->SV->SVnv). Not
> > the best solution, but as far as I understand this is usual
> > practise in postgresql to serialize Numerics and de-serialize them.
> >
> > 3. SVnv is transformed into jsonb through string
> > (SVnv->String->Numeric).
> >
> > An example may also be helpful to understand extension. So, as an
> > example, function "test" transforms incoming jsonb into perl,
> > transforms it back into jsonb and returns it.
> >
> > create extension jsonb_plperl cascade;
> >
> > create or replace function test(val jsonb)
> > returns jsonb
> > transform for type jsonb
> > language plperl
> > as $$
> > return $_[0];
> > $$;
> >
> > select test('{"1":1,"example": null}'::jsonb);
> >
> >
> I am looking to this patch:
>
> 1. the patch contains some artefacts - look the word "hstore"
>
> 2. I got lot of warnings
>
>
> make[1]: Vstupuje se do adresáře
> „/home/pavel/src/postgresql/contrib/jsonb_plperl“
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3
> -fno-omit-frame-pointer -fPIC -I../../src/pl/plperl -I. -I.
> -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2
> -I/usr/lib64/perl5/CORE -c -o jsonb_plperl.o jsonb_plperl.c
> jsonb_plperl.c: In function ‘SV_FromJsonbValue’: jsonb_plperl.c:83:9:
> warning: ‘result’ may be used uninitialized in this function
> [-Wmaybe-uninitialized] return (result);
> ^
> jsonb_plperl.c: In function ‘SV_FromJsonb’:
> jsonb_plperl.c:95:10: warning: ‘object’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
> HV *object;
> ^~~~~~
> In file included from /usr/lib64/perl5/CORE/perl.h:5644:0,
> from ../../src/pl/plperl/plperl.h:52,
> from jsonb_plperl.c:17:
> /usr/lib64/perl5/CORE/embed.h:404:19: warning: ‘value’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> #define newRV(a) Perl_newRV(aTHX_ a)
> ^~~~~~~~~~
> jsonb_plperl.c:101:10: note: ‘value’ was declared here
> SV *value;
> ^~~~~
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3
> -fno-omit-frame-pointer -fPIC -shared -o jsonb_plperl.so
> jsonb_plperl.o -L../../src/port -L../../src/common -Wl,--as-needed
> -Wl,-rpath,'/usr/lib64/perl5/CORE',--enable-new-dtags -Wl,-z,relro
> -specs=/usr/lib/rpm/redhat/redhat-hardened-ld
> -fstack-protector-strong -L/usr/local/lib -L/usr/lib64/perl5/CORE
> -lperl -lpthread -lresolv -lnsl -ldl -lm -lcrypt -lutil -lc make[1]:
> Opouští se adresář „/home/pavel/src/postgresql/contrib/jsonb_plperl“
>
> [pavel(at)nemesis contrib]$ gcc --version
> gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2)
> Copyright (C) 2017 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There
> is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
> PARTICULAR PURPOSE.
>
> 3. regress tests passed
>
> 4. There are not any documentation - probably it should be part of
> PLPerl
>
> 5. The regress tests doesn't coverage other datatypes than numbers. I
> miss boolean, binary, object, ... Maybe using data::dumper or some
> similar can be interesting
>
> Note - it is great extension, I am pleasured so transformations are
> used.
>
> Regards
>
> Pavel
> >
> > --
> > Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
> >

Hello,
Thank you for your review. I have fixed most of your comments, except
for the 5-th part, about data::dumper - I just couldn't understand
your point, but I've added more tests with more complex objects if this
helps.

Please, take a look at new patch. You can find it in attachments to
this message (it is called "0001-jsonb_plperl-extension-v2.patch")

I'm curious, how much benefit we could get from this ? There are several
publicly available json datasets, which can be used to measure performance
gaining. I have bookmarks and review datasets available from
http://www.sai.msu.su/~megera/postgres/files/, look at js.dump.gz and
jr.dump.gz

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

2017-11-15 10:24 GMT+01:00 Oleg Bartunov <obartunov(at)gmail(dot)com>:

>
>
> On 14 Nov 2017 11:35, "Anthony Bykov" <a(dot)bykov(at)postgrespro(dot)ru> wrote:
>
> On Fri, 10 Nov 2017 14:40:21 +0100
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
> > Hi
> >
> > 2017-10-24 14:27 GMT+02:00 Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru>:
> >
> > > There are some moments I should mention:
> > > 1. {"1":1}::jsonb is transformed into HV {"1"=>"1"}, while
> > > ["1","2"]::jsonb is transformed into AV ["1", "2"]
> > >
> > > 2. If there is a numeric value appear in jsonb, it will be
> > > transformed to SVnv through string (Numeric->String->SV->SVnv). Not
> > > the best solution, but as far as I understand this is usual
> > > practise in postgresql to serialize Numerics and de-serialize them.
> > >
> > > 3. SVnv is transformed into jsonb through string
> > > (SVnv->String->Numeric).
> > >
> > > An example may also be helpful to understand extension. So, as an
> > > example, function "test" transforms incoming jsonb into perl,
> > > transforms it back into jsonb and returns it.
> > >
> > > create extension jsonb_plperl cascade;
> > >
> > > create or replace function test(val jsonb)
> > > returns jsonb
> > > transform for type jsonb
> > > language plperl
> > > as $$
> > > return $_[0];
> > > $$;
> > >
> > > select test('{"1":1,"example": null}'::jsonb);
> > >
> > >
> > I am looking to this patch:
> >
> > 1. the patch contains some artefacts - look the word "hstore"
> >
> > 2. I got lot of warnings
> >
> >
> > make[1]: Vstupuje se do adresáře
> > „/home/pavel/src/postgresql/contrib/jsonb_plperl“
> > gcc -Wall -Wmissing-prototypes -Wpointer-arith
> > -Wdeclaration-after-statement -Wendif-labels
> > -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> > -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3
> > -fno-omit-frame-pointer -fPIC -I../../src/pl/plperl -I. -I.
> > -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2
> > -I/usr/lib64/perl5/CORE -c -o jsonb_plperl.o jsonb_plperl.c
> > jsonb_plperl.c: In function ‘SV_FromJsonbValue’: jsonb_plperl.c:83:9:
> > warning: ‘result’ may be used uninitialized in this function
> > [-Wmaybe-uninitialized] return (result);
> > ^
> > jsonb_plperl.c: In function ‘SV_FromJsonb’:
> > jsonb_plperl.c:95:10: warning: ‘object’ may be used uninitialized in
> > this function [-Wmaybe-uninitialized]
> > HV *object;
> > ^~~~~~
> > In file included from /usr/lib64/perl5/CORE/perl.h:5644:0,
> > from ../../src/pl/plperl/plperl.h:52,
> > from jsonb_plperl.c:17:
> > /usr/lib64/perl5/CORE/embed.h:404:19: warning: ‘value’ may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
> > #define newRV(a) Perl_newRV(aTHX_ a)
> > ^~~~~~~~~~
> > jsonb_plperl.c:101:10: note: ‘value’ was declared here
> > SV *value;
> > ^~~~~
> > gcc -Wall -Wmissing-prototypes -Wpointer-arith
> > -Wdeclaration-after-statement -Wendif-labels
> > -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> > -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3
> > -fno-omit-frame-pointer -fPIC -shared -o jsonb_plperl.so
> > jsonb_plperl.o -L../../src/port -L../../src/common -Wl,--as-needed
> > -Wl,-rpath,'/usr/lib64/perl5/CORE',--enable-new-dtags -Wl,-z,relro
> > -specs=/usr/lib/rpm/redhat/redhat-hardened-ld
> > -fstack-protector-strong -L/usr/local/lib -L/usr/lib64/perl5/CORE
> > -lperl -lpthread -lresolv -lnsl -ldl -lm -lcrypt -lutil -lc make[1]:
> > Opouští se adresář „/home/pavel/src/postgresql/contrib/jsonb_plperl“
> >
> > [pavel(at)nemesis contrib]$ gcc --version
> > gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2)
> > Copyright (C) 2017 Free Software Foundation, Inc.
> > This is free software; see the source for copying conditions. There
> > is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
> > PARTICULAR PURPOSE.
> >
> > 3. regress tests passed
> >
> > 4. There are not any documentation - probably it should be part of
> > PLPerl
> >
> > 5. The regress tests doesn't coverage other datatypes than numbers. I
> > miss boolean, binary, object, ... Maybe using data::dumper or some
> > similar can be interesting
> >
> > Note - it is great extension, I am pleasured so transformations are
> > used.
> >
> > Regards
> >
> > Pavel
> > >
> > > --
> > > Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> > > To make changes to your subscription:
> > > http://www.postgresql.org/mailpref/pgsql-hackers
> > >
>
> Hello,
> Thank you for your review. I have fixed most of your comments, except
> for the 5-th part, about data::dumper - I just couldn't understand
> your point, but I've added more tests with more complex objects if this
> helps.
>
> Please, take a look at new patch. You can find it in attachments to
> this message (it is called "0001-jsonb_plperl-extension-v2.patch")
>
>
> I'm curious, how much benefit we could get from this ? There are several
> publicly available json datasets, which can be used to measure performance
> gaining. I have bookmarks and review datasets available from
> http://www.sai.msu.su/~megera/postgres/files/, look at js.dump.gz and
> jr.dump.gz
>

I don't expect significant performance effect - it remove some
transformations - perl object -> json | json -> jsonb - but on modern cpu
these transformations should be fast. For me - main benefit is user comfort
- it does direct transformation from perl object -> jsonb

But some performance check can be interesting

Regards

Pavel

>
>
> --
> Anthony Bykov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>
>

Hello, hackers.

> > I'm curious, how much benefit we could get from this ? There are several
> > publicly available json datasets, which can be used to measure performance
> > gaining. I have bookmarks and review datasets available from
> > http://www.sai.msu.su/~megera/postgres/files/, look at js.dump.gz and
> > jr.dump.gz
> >
>
> I don't expect significant performance effect - it remove some
> transformations - perl object -> json | json -> jsonb - but on modern cpu
> these transformations should be fast. For me - main benefit is user comfort
> - it does direct transformation from perl object -> jsonb

I completely agree that currently the main benefit of this feature is
user comfort. So correctness is the priority. When we make sure that the
implementation is correct we can start worry about the performance.
Probably in a separate patch.

Thanks for the datasets though!

--
Best regards,
Aleksander Alekseev

On Wed, 15 Nov 2017 08:58:54 +0000
Aleksander Alekseev <a(dot)alekseev(at)postgrespro(dot)ru> 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
>
> Hello Anthony,
>
> Great patch! Everything is OK and I almost agree with Pavel.
>
> The only thing that I would like to suggest is to add a little more
> tests for various corner cases. For instance:
>
> 1. Converting in both directions (Perl <-> JSONB) +/- infinity, NaN,
> MAX_INT, MIN_INT.
>
> 2. Converting in both directions strings that contain non-ASCII
> (Russian / Japanese / etc) characters and special characters like \n,
> \t, \.
>
> 3. Make sure that converting Perl objects that are not representable
> in JSONB (blessed hashes, file descriptors, regular expressions, ...)
> doesn't crash everything and shows a reasonable error message.
>
> The new status of this patch is: Waiting on Author

Hello Aleksander,
Thank you for your review.

I've added more tests and I had to change behavior of transform when
working with not-representable-in-JSONB format objects - now it will
through an exception. You can find an example in tests.

Please, find the 4-th version of patch in attachments.

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

Looks good to me.

The new status of this patch is: Ready for Committer

On Tue, Nov 28, 2017 at 5:14 PM, Aleksander Alekseev
<a(dot)alekseev(at)postgrespro(dot)ru> wrote:
> The new status of this patch is: Ready for Committer

Patch moved to CF 2018-01. Perhaps a committer will look at it at some point.
--
Michael

On Fri, Dec 1, 2017 at 12:30 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Nov 28, 2017 at 5:14 PM, Aleksander Alekseev
> <a(dot)alekseev(at)postgrespro(dot)ru> wrote:
>> The new status of this patch is: Ready for Committer
>
> Patch moved to CF 2018-01. Perhaps a committer will look at it at some point.

FWIW, although I like the idea, I'm not going to work on the patch. I
like Perl, but I neither know its internals nor use plperl. I hope
someone else will be interested.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Dec 1, 2017 at 12:30 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> Patch moved to CF 2018-01. Perhaps a committer will look at it at some point.

> FWIW, although I like the idea, I'm not going to work on the patch. I
> like Perl, but I neither know its internals nor use plperl. I hope
> someone else will be interested.

I've been assuming (and I imagine other committers think likewise) that
Peter will pick this up at some point, since the whole transform feature
was his work to begin with. If he doesn't want to touch it, maybe he
should say so explicitly so that other people will feel free to take it.

regards, tom lane

A few very minor comments while quickly paging through:

1. non-ASCII tests are going to cause problems in one platform or
another. Please don't include that one.

2. error messages
a) please use ereport() not elog()
b) conform to style guidelines: errmsg() start with lowercase, others
are complete phrases (start with uppercase, end with period)
c) replace
"The type you was trying to transform can't be represented in JSONB"
maybe with
errmsg("could not transform to type \"%s\"", "jsonb"),
errdetail("The type you are trying to transform can't be represented in JSON")
d) same errmsg() for the other error; figure out suitable errdetail.

3. whitespace: don't add newlines to while, DirectFunctionCallN, pnstrdup.

4. the "relocatability" test seems pointless to me.

5. "#undef _" looks bogus. Don't do it.

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

On 12/01/2017 11:37 AM, Robert Haas wrote:
> On Fri, Dec 1, 2017 at 12:30 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Tue, Nov 28, 2017 at 5:14 PM, Aleksander Alekseev
>> <a(dot)alekseev(at)postgrespro(dot)ru> wrote:
>>> The new status of this patch is: Ready for Committer
>> Patch moved to CF 2018-01. Perhaps a committer will look at it at some point.
> FWIW, although I like the idea, I'm not going to work on the patch. I
> like Perl, but I neither know its internals nor use plperl. I hope
> someone else will be interested.
>

I will probably pick it up fairly shortly.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, 1 Dec 2017 15:49:21 -0300
Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:

> A few very minor comments while quickly paging through:
>
> 1. non-ASCII tests are going to cause problems in one platform or
> another. Please don't include that one.
>
> 2. error messages
> a) please use ereport() not elog()
> b) conform to style guidelines: errmsg() start with lowercase,
> others are complete phrases (start with uppercase, end with period)
> c) replace
> "The type you was trying to transform can't be represented in
> JSONB" maybe with
> errmsg("could not transform to type \"%s\"", "jsonb"),
> errdetail("The type you are trying to transform can't be
> represented in JSON") d) same errmsg() for the other error; figure
> out suitable errdetail.
>
> 3. whitespace: don't add newlines to while, DirectFunctionCallN,
> pnstrdup.
>
> 4. the "relocatability" test seems pointless to me.
>
> 5. "#undef _" looks bogus. Don't do it.
>

Hello,
thank you for your time.

1. I really think that it might be a good practice to test non ASCII
symbols on platforms where it is possible. Maybe locale-dependent
Makefile? I've already spent pretty much time trying to find possible
solutions and I have no results. So, I've deleted this tests. Maybe
there is a better solution I don't know about?

2. Thank you for this one. Writing those errors were really pain for
me. I've changed those things in new patch.

3. I've fixed all the whitespaces you was talking about in new version
of the patch.

4. The relocatibility test is needed in order to check if patch is
still relocatable. With this test I've tried to prove the code
"relocatable=true" in *.control files. So, I've decided to leave them
in next version of the patch.

5. If I delete #undef _, I will get the warning:
/usr/lib/x86_64-linux-gnu/perl/5.22/CORE/config.h:3094:0:
warning: "_" redefined #define _(args) args

In file included from ../../src/include/postgres.h:47:0,
from jsonb_plperl.c:12:
../../src/include/c.h:971:0: note: this is the location of the
previous definition #define _(x) gettext(x)
This #undef was meant to fix the warning. I hope a new comment next
to #undef contains all the explanations needed.

Please, find a new version of the patch in attachments to this message.

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, 7 Dec 2017 12:54:55 +0300
Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru> wrote:

> On Fri, 1 Dec 2017 15:49:21 -0300
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> > A few very minor comments while quickly paging through:
> >
> > 1. non-ASCII tests are going to cause problems in one platform or
> > another. Please don't include that one.
> >
> > 2. error messages
> > a) please use ereport() not elog()
> > b) conform to style guidelines: errmsg() start with lowercase,
> > others are complete phrases (start with uppercase, end with period)
> > c) replace
> > "The type you was trying to transform can't be represented in
> > JSONB" maybe with
> > errmsg("could not transform to type \"%s\"", "jsonb"),
> > errdetail("The type you are trying to transform can't be
> > represented in JSON") d) same errmsg() for the other error; figure
> > out suitable errdetail.
> >
> > 3. whitespace: don't add newlines to while, DirectFunctionCallN,
> > pnstrdup.
> >
> > 4. the "relocatability" test seems pointless to me.
> >
> > 5. "#undef _" looks bogus. Don't do it.
> >
>
> Hello,
> thank you for your time.
>
> 1. I really think that it might be a good practice to test non ASCII
> symbols on platforms where it is possible. Maybe locale-dependent
> Makefile? I've already spent pretty much time trying to find
> possible solutions and I have no results. So, I've deleted this
> tests. Maybe there is a better solution I don't know about?
>
> 2. Thank you for this one. Writing those errors were really pain for
> me. I've changed those things in new patch.
>
> 3. I've fixed all the whitespaces you was talking about in new version
> of the patch.
>
> 4. The relocatibility test is needed in order to check if patch is
> still relocatable. With this test I've tried to prove the code
> "relocatable=true" in *.control files. So, I've decided to leave
> them in next version of the patch.
>
> 5. If I delete #undef _, I will get the warning:
> /usr/lib/x86_64-linux-gnu/perl/5.22/CORE/config.h:3094:0:
> warning: "_" redefined #define _(args) args
>
> In file included from ../../src/include/postgres.h:47:0,
> from jsonb_plperl.c:12:
> ../../src/include/c.h:971:0: note: this is the location of the
> previous definition #define _(x) gettext(x)
> This #undef was meant to fix the warning. I hope a new comment next
> to #undef contains all the explanations needed.
>
> Please, find a new version of the patch in attachments to this
> message.
>
>
> --
> Anthony Bykov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company

Forgot the patch.

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 12/1/17 13:15, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Dec 1, 2017 at 12:30 AM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> Patch moved to CF 2018-01. Perhaps a committer will look at it at some point.
>
>> FWIW, although I like the idea, I'm not going to work on the patch. I
>> like Perl, but I neither know its internals nor use plperl. I hope
>> someone else will be interested.
>
> I've been assuming (and I imagine other committers think likewise) that
> Peter will pick this up at some point, since the whole transform feature
> was his work to begin with. If he doesn't want to touch it, maybe he
> should say so explicitly so that other people will feel free to take it.

I'll take a look.

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

On Thu, Dec 7, 2017 at 10:56 PM, Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru> wrote:
>> Please, find a new version of the patch in attachments to this
>> message.

Hi again Anthony,

I wonder why make check passes for me on my Mac, but when Travis CI
(Ubuntu Trusty on amd64) runs it, it fails like this:

test jsonb_plperl ... FAILED
test jsonb_plperl_relocatability ... ok
test jsonb_plperlu ... FAILED
test jsonb_plperlu_relocatability ... ok

========= Contents of ./contrib/jsonb_plperl/regression.diffs
*** /home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/expected/jsonb_plperl.out
2018-01-11 21:46:35.867584467 +0000
--- /home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/results/jsonb_plperl.out
2018-01-11 21:55:08.564204175 +0000
***************
*** 89,96 ****
(1 row)

SELECT testSVToJsonb2('1E+131071');
! ERROR: could not transform to type "jsonb"
! DETAIL: The type you are trying to transform can't be transformed to jsonb
CONTEXT: PL/Perl function "testsvtojsonb2"
SELECT testSVToJsonb2('-1');
testsvtojsonb2
--- 89,95 ----
(1 row)

SELECT testSVToJsonb2('1E+131071');
! ERROR: invalid input syntax for type numeric: "inf"
CONTEXT: PL/Perl function "testsvtojsonb2"
SELECT testSVToJsonb2('-1');
testsvtojsonb2
======================================================================
*** /home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/expected/jsonb_plperlu.out
2018-01-11 21:46:35.867584467 +0000
--- /home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/results/jsonb_plperlu.out
2018-01-11 21:55:08.704204228 +0000
***************
*** 89,96 ****
(1 row)

SELECT testSVToJsonb2('1E+131071');
! ERROR: could not transform to type "jsonb"
! DETAIL: The type you are trying to transform can't be transformed to jsonb
CONTEXT: PL/Perl function "testsvtojsonb2"
SELECT testSVToJsonb2('-1');
testsvtojsonb2
--- 89,95 ----
(1 row)

SELECT testSVToJsonb2('1E+131071');
! ERROR: invalid input syntax for type numeric: "inf"
CONTEXT: PL/Perl function "testsvtojsonb2"
SELECT testSVToJsonb2('-1');
testsvtojsonb2
======================================================================

--
Thomas Munro
http://www.enterprisedb.com

On Fri, 12 Jan 2018 15:19:26 +1300
Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> wrote:

> On Thu, Dec 7, 2017 at 10:56 PM, Anthony Bykov
> <a(dot)bykov(at)postgrespro(dot)ru> wrote:
> >> Please, find a new version of the patch in attachments to this
> >> message.
>
> Hi again Anthony,
>
> I wonder why make check passes for me on my Mac, but when Travis CI
> (Ubuntu Trusty on amd64) runs it, it fails like this:
>
> test jsonb_plperl ... FAILED
> test jsonb_plperl_relocatability ... ok
> test jsonb_plperlu ... FAILED
> test jsonb_plperlu_relocatability ... ok
>
> ========= Contents of ./contrib/jsonb_plperl/regression.diffs
> *** /home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/expected/jsonb_plperl.out
> 2018-01-11 21:46:35.867584467 +0000
> --- /home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/results/jsonb_plperl.out
> 2018-01-11 21:55:08.564204175 +0000
> ***************
> *** 89,96 ****
> (1 row)
>
> SELECT testSVToJsonb2('1E+131071');
> ! ERROR: could not transform to type "jsonb"
> ! DETAIL: The type you are trying to transform can't be transformed
> to jsonb CONTEXT: PL/Perl function "testsvtojsonb2"
> SELECT testSVToJsonb2('-1');
> testsvtojsonb2
> --- 89,95 ----
> (1 row)
>
> SELECT testSVToJsonb2('1E+131071');
> ! ERROR: invalid input syntax for type numeric: "inf"
> CONTEXT: PL/Perl function "testsvtojsonb2"
> SELECT testSVToJsonb2('-1');
> testsvtojsonb2
> ======================================================================
> *** /home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/expected/jsonb_plperlu.out
> 2018-01-11 21:46:35.867584467 +0000
> --- /home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/results/jsonb_plperlu.out
> 2018-01-11 21:55:08.704204228 +0000
> ***************
> *** 89,96 ****
> (1 row)
>
> SELECT testSVToJsonb2('1E+131071');
> ! ERROR: could not transform to type "jsonb"
> ! DETAIL: The type you are trying to transform can't be transformed
> to jsonb CONTEXT: PL/Perl function "testsvtojsonb2"
> SELECT testSVToJsonb2('-1');
> testsvtojsonb2
> --- 89,95 ----
> (1 row)
>
> SELECT testSVToJsonb2('1E+131071');
> ! ERROR: invalid input syntax for type numeric: "inf"
> CONTEXT: PL/Perl function "testsvtojsonb2"
> SELECT testSVToJsonb2('-1');
> testsvtojsonb2
> ======================================================================
>

Hello, thank you for your message.
The problem was that different perl compilers uses different infinity
representations. Some of them use "Inf" others - use "inf". So, in
attachments there is a new version of the patch.

Thank you again.

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 01/12/2018 03:47 AM, Anthony Bykov wrote:
>
> The problem was that different perl compilers uses different infinity
> representations. Some of them use "Inf" others - use "inf". So, in
> attachments there is a new version of the patch.
>

There's a bit of an impedance mismatch and inconsistency here. I think
we need to deal with json scalars (particularly numerics) the same way
we do for plain scalar arguments. We don't convert a numeric argument to
and SvNV. We just do this in plperl_call_perl_func():

                tmp = OutputFunctionCall(&(desc->arg_out_func[i]),
                                         fcinfo->arg[i]);
                sv = cstr2sv(tmp);
                pfree(tmp)
[...]

            PUSHs(sv_2mortal(sv));

Large numerics won't work as SvNV values, which have to fit in a
standard double. So I think we should treat them the same way we do for
plain scalar arguments.

(This also suggests that the tests are a bit deficient in not testing
jsonb with large numeric values.)

I'm going to set this back to waiting on author pending discussion.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jan 12, 2018 at 9:47 PM, Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru> wrote:
> On Fri, 12 Jan 2018 15:19:26 +1300
> Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> Hello, thank you for your message.
> The problem was that different perl compilers uses different infinity
> representations. Some of them use "Inf" others - use "inf". So, in
> attachments there is a new version of the patch.

BTW PostgreSQL is written in C89 (though it uses some C99 library features if
present, just not language features), so you can't do this:

jsonb_plperl.c: In function ‘SV_ToJsonbValue’:
jsonb_plperl.c:238:6: error: ‘for’ loop initial declarations are only
allowed in C99 mode
for (int i=0;str[i];i++)
^

--
Thomas Munro
http://www.enterprisedb.com

Hello,

On Fri, Jan 12, 2018 at 11:47:39AM +0300, Anthony Bykov wrote:
> Hello, thank you for your message.
> The problem was that different perl compilers uses different infinity
> representations. Some of them use "Inf" others - use "inf". So, in
> attachments there is a new version of the patch.

I've noticed a possible bug:

> + /* json key in v */
> + key = pstrdup(v.val.string.val);
> + keyLength = v.val.string.len;
> + JsonbIteratorNext(&it, &v, true);

I think it is worth to use pnstrdup() here, because v.val.string.val is
not necessarily null-terminated as the comment says:

> struct JsonbValue
> ...
> struct
> {
> int len;
> char *val; /* Not necessarily null-terminated */
> } string; /* String primitive type */

Consider an example:

=# CREATE FUNCTION testSVToJsonb3(val jsonb) RETURNS jsonb
LANGUAGE plperl
TRANSFORM FOR TYPE jsonb
AS $$
return $_->{"1"};
$$;

=# SELECT testSVToJsonb3('{"1":{"2":[3,4,5]},"2":3}');
testsvtojsonb3
----------------
(null)

But my perl isn't good, so the example maybe isn't good too.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On Sat, 13 Jan 2018 09:29:46 -0500
Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:

> There's a bit of an impedance mismatch and inconsistency here. I think
> we need to deal with json scalars (particularly numerics) the same way
> we do for plain scalar arguments. We don't convert a numeric argument
> to and SvNV. We just do this in plperl_call_perl_func():
>
>                 tmp = OutputFunctionCall(&(desc->arg_out_func[i]),
>                                          fcinfo->arg[i]);
>                 sv = cstr2sv(tmp);
>                 pfree(tmp)
> [...]
>
>             PUSHs(sv_2mortal(sv));
>
> Large numerics won't work as SvNV values, which have to fit in a
> standard double. So I think we should treat them the same way we do
> for plain scalar arguments.
>
> (This also suggests that the tests are a bit deficient in not testing
> jsonb with large numeric values.)
>
> I'm going to set this back to waiting on author pending discussion.
>
>
> cheers
>
> andrew
>

Hello,
thank you for your attention.

I'm sorry, but I couldn't understand what types of numerics you was
talking about. Large numerics are just transformed into "inf" (or
"Inf") and the patch contains such test. But there were no tests with
numerics close to "inf" but not "inf" yet. So, I've added such test.

Also I've fixed the thing Thomas Munro was talking about.

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Wed, 31 Jan 2018 13:36:22 +0300
Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> wrote:

> I've noticed a possible bug:
>
> > + /* json key in v */
> > + key =
> > pstrdup(v.val.string.val);
> > + keyLength =
> > v.val.string.len;
> > + JsonbIteratorNext(&it, &v,
> > true);
>
> I think it is worth to use pnstrdup() here, because v.val.string.val
> is not necessarily null-terminated as the comment says:
>
> > struct JsonbValue
> > ...
> > struct
> > {
> > int len;
> > char *val; /* Not
> > necessarily null-terminated */ }
> > string; /* String primitive type */
>
> Consider an example:
>
> =# CREATE FUNCTION testSVToJsonb3(val jsonb) RETURNS jsonb
> LANGUAGE plperl
> TRANSFORM FOR TYPE jsonb
> AS $$
> return $_->{"1"};
> $$;
>
> =# SELECT testSVToJsonb3('{"1":{"2":[3,4,5]},"2":3}');
> testsvtojsonb3
> ----------------
> (null)
>
> But my perl isn't good, so the example maybe isn't good too.
>

Hello.
Glad you've noticed this. Thank you.

I've fixed this possible bug in the new patch, but your example
can't check that.

The problem is that $_ - is a pointer to an array of incoming
parameters. So, if you return $_[0]->{"1"} instead of $_->{"1"}, the
test will return exactly the expected output: {"2":[3,4,5]}

I've tried to test "chop" and even "=~ s/\0$//", but that didn't check
the problem.

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi

I am looking on this patch. I found few issues:

1. compile warning

I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2
-I/usr/lib64/perl5/CORE -c -o jsonb_plperl.o jsonb_plperl.c
jsonb_plperl.c: In function ‘SV_FromJsonbValue’:
jsonb_plperl.c:69:9: warning: ‘result’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
return result;
^~~~~~
jsonb_plperl.c: In function ‘SV_FromJsonb’:
jsonb_plperl.c:142:9: warning: ‘result’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
return result;
^~~~~~

2. bad comment

/*
* SV_ToJsonbValue
*
* Transform Jsonb into SV --- propably reverse direction
*/

/*
* HV_ToJsonbValue
*
* Transform Jsonb into SV
*/

/*
* plperl_to_jsonb(SV *in)
*
* Transform Jsonb into SV
*/

3. Do we need two identical tests fro PLPerl and PLPerlu? Why?

Regards

Pavel

On Mon, 5 Mar 2018 14:03:37 +0100
Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:

> Hi
>
> I am looking on this patch. I found few issues:
>
> 1. compile warning
>
> I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2
> -I/usr/lib64/perl5/CORE -c -o jsonb_plperl.o jsonb_plperl.c
> jsonb_plperl.c: In function ‘SV_FromJsonbValue’:
> jsonb_plperl.c:69:9: warning: ‘result’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
> return result;
> ^~~~~~
> jsonb_plperl.c: In function ‘SV_FromJsonb’:
> jsonb_plperl.c:142:9: warning: ‘result’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
> return result;
> ^~~~~~
>
> 2. bad comment
>
> /*
> * SV_ToJsonbValue
> *
> * Transform Jsonb into SV --- propably reverse direction
> */
>
>
> /*
> * HV_ToJsonbValue
> *
> * Transform Jsonb into SV
> */
>
> /*
> * plperl_to_jsonb(SV *in)
> *
> * Transform Jsonb into SV
> */
>
> 3. Do we need two identical tests fro PLPerl and PLPerlu? Why?
>
> Regards
>
> Pavel

Hello, thanks for reviewing my patch! I really appreciate it.

That warnings are created on purpose - I was trying to prevent the case
when new types are added into pl/perl, but new transform logic was not.
Maybe there is a better way to do it, but right now I'll just add the
"default: pg_unreachable" logic.

About the 3 point - I thought that plperlu and plperl uses different
interpreters. And if they act identically on same examples - there
is no need in identical tests for them indeed.

Point 2 is fixed in this version of the patch.

Please, find in attachments a new version of the patch.

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

2018-03-12 9:08 GMT+01:00 Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru>:

> On Mon, 5 Mar 2018 14:03:37 +0100
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
> > Hi
> >
> > I am looking on this patch. I found few issues:
> >
> > 1. compile warning
> >
> > I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2
> > -I/usr/lib64/perl5/CORE -c -o jsonb_plperl.o jsonb_plperl.c
> > jsonb_plperl.c: In function ‘SV_FromJsonbValue’:
> > jsonb_plperl.c:69:9: warning: ‘result’ may be used uninitialized in
> > this function [-Wmaybe-uninitialized]
> > return result;
> > ^~~~~~
> > jsonb_plperl.c: In function ‘SV_FromJsonb’:
> > jsonb_plperl.c:142:9: warning: ‘result’ may be used uninitialized in
> > this function [-Wmaybe-uninitialized]
> > return result;
> > ^~~~~~
> >
> > 2. bad comment
> >
> > /*
> > * SV_ToJsonbValue
> > *
> > * Transform Jsonb into SV --- propably reverse direction
> > */
> >
> >
> > /*
> > * HV_ToJsonbValue
> > *
> > * Transform Jsonb into SV
> > */
> >
> > /*
> > * plperl_to_jsonb(SV *in)
> > *
> > * Transform Jsonb into SV
> > */
> >
> > 3. Do we need two identical tests fro PLPerl and PLPerlu? Why?
> >
> > Regards
> >
> > Pavel
>
> Hello, thanks for reviewing my patch! I really appreciate it.
>
> That warnings are created on purpose - I was trying to prevent the case
> when new types are added into pl/perl, but new transform logic was not.
> Maybe there is a better way to do it, but right now I'll just add the
> "default: pg_unreachable" logic.
>
> About the 3 point - I thought that plperlu and plperl uses different
> interpreters. And if they act identically on same examples - there
> is no need in identical tests for them indeed.
>

plperlu and plperl uses same interprets - so the duplicate tests has not
any sense. But in last versions there are duplicate tests again

The naming convention of functions is not consistent

almost are are src_to_dest

This is different and it is little bit messy

+static SV *
+SV_FromJsonb(JsonbContainer *jsonb)

This comment is broken

+/*
+ * plperl_to_jsonb(SV *in)
+ *
+ * Transform Jsonb into SV ---< should be SV to Jsonb
+ */
+PG_FUNCTION_INFO_V1(plperl_to_jsonb);
+Datum

Regards

Pavel

>
> Point 2 is fixed in this version of the patch.
>
> Please, find in attachments a new version of the patch.
>
> --
> Anthony Bykov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Hi.

I have reviewed this patch too. Attached new version with v8-v9 delta-patch.

Here is my changes:

* HV_ToJsonbValue():
- addded missing hv_iterinit()
- used hv_iternextsv() instead of hv_iternext(), HeSVKEY_force(), HeVAL()

* SV_ToJsonbValue():
- added recursive dereferencing for all SV types
- removed unnecessary JsonbValue heap-allocations

* Jsonb_ToSV():
- added iteration to the end of iterator needed for correct freeing of
JsonbIterators

* passed JsonbParseState ** to XX_ToJsonbValue() functions.

* fixed warnings (see below)

* fixed comments (see below)

Also I am not sure if we need to use newRV() for returning SVs in
Jsonb_ToSV() and JsonbValue_ToSV().

On 12.03.2018 18:06, Pavel Stehule wrote:

> 2018-03-12 9:08 GMT+01:00 Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru
> <mailto:a(dot)bykov(at)postgrespro(dot)ru>>:
>
> On Mon, 5 Mar 2018 14:03:37 +0100
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com
> <mailto:pavel(dot)stehule(at)gmail(dot)com>> wrote:
>
> > Hi
> >
> > I am looking on this patch. I found few issues:
> >
> > 1. compile warning
> >
> > I../../src/include  -D_GNU_SOURCE -I/usr/include/libxml2
> > -I/usr/lib64/perl5/CORE  -c -o jsonb_plperl.o jsonb_plperl.c
> > jsonb_plperl.c: In function ‘SV_FromJsonbValue’:
> > jsonb_plperl.c:69:9: warning: ‘result’ may be used uninitialized in
> > this function [-Wmaybe-uninitialized]
> >   return result;
> >          ^~~~~~
> > jsonb_plperl.c: In function ‘SV_FromJsonb’:
> > jsonb_plperl.c:142:9: warning: ‘result’ may be used uninitialized in
> > this function [-Wmaybe-uninitialized]
> >   return result;
> >          ^~~~~~
>
> Hello, thanks for reviewing my patch! I really appreciate it.
>
> That warnings are created on purpose - I was trying to prevent the
> case
> when new types are added into pl/perl, but new transform logic was
> not.
> Maybe there is a better way to do it, but right now I'll just add the
> "default: pg_unreachable" logic.
>
pg_unreachable() is replaced with elog(ERROR) for reporting impossible
JsonbValue types and JsonbIteratorTokens.

> > 3. Do we need two identical tests fro PLPerl and PLPerlu? Why?
> >
> > Regards
> >
> > Pavel
>
> About the 3 point - I thought that plperlu and plperl uses different
> interpreters. And if they act identically on same examples - there
> is no need in identical tests for them indeed.
>
>
> plperlu and plperl uses same interprets - so the duplicate tests has
> not any sense. But in last versions there are duplicate tests again
I have not removed duplicate test yet, because I am not sure that this
test does not make sense at all.

> The naming convention of functions is not consistent
>
> almost are are src_to_dest
>
> This is different and it is little bit messy
>
> +static SV  *
> +SV_FromJsonb(JsonbContainer *jsonb)
>
Renamed to Jsonb_ToSV() and JsonbValue_ToSV().

> This comment is broken
>
> +/*
> + * plperl_to_jsonb(SV *in)
> + *
> + * Transform Jsonb into SV  ---< should be SV to Jsonb
> + */
> +PG_FUNCTION_INFO_V1(plperl_to_jsonb);
> +Datum
Fixed.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi

2018-03-13 15:50 GMT+01:00 Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>:

> Hi.
>
> I have reviewed this patch too. Attached new version with v8-v9 delta-patch.
>
> Here is my changes:
>
> * HV_ToJsonbValue():
> - addded missing hv_iterinit()
> - used hv_iternextsv() instead of hv_iternext(), HeSVKEY_force(), HeVAL()
>
> * SV_ToJsonbValue():
> - added recursive dereferencing for all SV types
> - removed unnecessary JsonbValue heap-allocations
>
> * Jsonb_ToSV():
> - added iteration to the end of iterator needed for correct freeing of
> JsonbIterators
>
> * passed JsonbParseState ** to XX_ToJsonbValue() functions.
>
> * fixed warnings (see below)
>
> * fixed comments (see below)
>
>
> Also I am not sure if we need to use newRV() for returning SVs in
> Jsonb_ToSV() and JsonbValue_ToSV().
>
>
> On 12.03.2018 18:06, Pavel Stehule wrote:
>
> 2018-03-12 9:08 GMT+01:00 Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru>:
>
>> On Mon, 5 Mar 2018 14:03:37 +0100
>> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>
>> > Hi
>> >
>> > I am looking on this patch. I found few issues:
>> >
>> > 1. compile warning
>> >
>> > I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2
>> > -I/usr/lib64/perl5/CORE -c -o jsonb_plperl.o jsonb_plperl.c
>> > jsonb_plperl.c: In function ‘SV_FromJsonbValue’:
>> > jsonb_plperl.c:69:9: warning: ‘result’ may be used uninitialized in
>> > this function [-Wmaybe-uninitialized]
>> > return result;
>> > ^~~~~~
>> > jsonb_plperl.c: In function ‘SV_FromJsonb’:
>> > jsonb_plperl.c:142:9: warning: ‘result’ may be used uninitialized in
>> > this function [-Wmaybe-uninitialized]
>> > return result;
>> > ^~~~~~
>>
>> Hello, thanks for reviewing my patch! I really appreciate it.
>>
>> That warnings are created on purpose - I was trying to prevent the case
>> when new types are added into pl/perl, but new transform logic was not.
>> Maybe there is a better way to do it, but right now I'll just add the
>> "default: pg_unreachable" logic.
>>
>> pg_unreachable() is replaced with elog(ERROR) for reporting impossible
> JsonbValue types and JsonbIteratorTokens.
>
> > 3. Do we need two identical tests fro PLPerl and PLPerlu? Why?
>> >
>> > Regards
>> >
>> > Pavel
>>
>> About the 3 point - I thought that plperlu and plperl uses different
>> interpreters. And if they act identically on same examples - there
>> is no need in identical tests for them indeed.
>>
>
> plperlu and plperl uses same interprets - so the duplicate tests has not
> any sense. But in last versions there are duplicate tests again
>
> I have not removed duplicate test yet, because I am not sure that this
> test does not make sense at all.
>

ok .. the commiter can decide it

>
> The naming convention of functions is not consistent
>
> almost are are src_to_dest
>
> This is different and it is little bit messy
>
> +static SV *
> +SV_FromJsonb(JsonbContainer *jsonb)
>
> Renamed to Jsonb_ToSV() and JsonbValue_ToSV().
>
> This comment is broken
>
> +/*
> + * plperl_to_jsonb(SV *in)
> + *
> + * Transform Jsonb into SV ---< should be SV to Jsonb
> + */
> +PG_FUNCTION_INFO_V1(plperl_to_jsonb);
> +Datum
>
> Fixed.
>

It looks well

the patch has tests and doc,
there are not any warnings or compilation issues
all tests passed

I'll mark this patch as ready for commiter

Regards

Pavel

>
>
> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

On 3/15/18 03:46, Pavel Stehule wrote:
> It looks well
>
> the patch has tests and doc,
> there are not any warnings or compilation issues
> all tests passed
>
> I'll mark this patch as ready for commiter

committed

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

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:

> On 3/15/18 03:46, Pavel Stehule wrote:
>> It looks well
>>
>> the patch has tests and doc,
>> there are not any warnings or compilation issues
>> all tests passed
>>
>> I'll mark this patch as ready for commiter
>
> committed

I played around with this a bit, and noticed that the number handling
doesn't cope with Perl UVs (unsigned integers) in the (IV_MAX, UV_MAX]
range:

ilmari(at)[local]:5432 ~=# CREATE FUNCTION testUVToJsonb() RETURNS jsonb
ilmari(at)[local]:5432 ~-# LANGUAGE plperl TRANSFORM FOR TYPE jsonb
ilmari(at)[local]:5432 ~-# as $$
ilmari(at)[local]:5432 ~$# $val = ~0;
ilmari(at)[local]:5432 ~$# elog(NOTICE, "value is $val");
ilmari(at)[local]:5432 ~$# return $val;
ilmari(at)[local]:5432 ~$# $$;
CREATE FUNCTION
Time: 6.795 ms
ilmari(at)[local]:5432 ~=# select testUVToJsonb();
NOTICE: value is 18446744073709551615
┌───────────────┐
│ testuvtojsonb │
├───────────────┤
│ -1 │
└───────────────┘
(1 row)

I tried fixing this by adding an 'if (SvUV(in))' clause to
SV_to_JsonbValue, but I couldn't find a function to create a numeric
value from an uint64. If it's not possible, should we error on UVs
greater than PG_INT64_MAX?

- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law

ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> I tried fixing this by adding an 'if (SvUV(in))' clause to
> SV_to_JsonbValue, but I couldn't find a function to create a numeric
> value from an uint64. If it's not possible, should we error on UVs
> greater than PG_INT64_MAX?

I think you'd have to convert to text and back. That's kind of icky,
but it beats failing.

Or we could add a not-visible-to-SQL uint8-to-numeric function in
numeric.c. Not sure if this is enough use-case to justify that
though.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> I tried fixing this by adding an 'if (SvUV(in))' clause to
>> SV_to_JsonbValue, but I couldn't find a function to create a numeric
>> value from an uint64. If it's not possible, should we error on UVs
>> greater than PG_INT64_MAX?
>
> I think you'd have to convert to text and back. That's kind of icky,
> but it beats failing.

I had a look, and that's what the PL/Python transform does. Attached is
a patch that does that for PL/Perl too, but only if the value is
actually > PG_INT64_MAX.

The secondary output files are for Perls with 32bit IV/UV types, but I
haven't been able to test them, since Debian's Perl uses 64bit integers
even on 32bit platforms.

> Or we could add a not-visible-to-SQL uint8-to-numeric function in
> numeric.c. Not sure if this is enough use-case to justify that
> though.

I don't think this one use-case is enough, but it's worth keeping in
mind if it keeps cropping up.

- ilmari
--
"I use RMS as a guide in the same way that a boat captain would use
a lighthouse. It's good to know where it is, but you generally
don't want to find yourself in the same spot." - Tollef Fog Heen

ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> I think you'd have to convert to text and back. That's kind of icky,
>> but it beats failing.

> I had a look, and that's what the PL/Python transform does. Attached is
> a patch that does that for PL/Perl too, but only if the value is
> actually > PG_INT64_MAX.

> The secondary output files are for Perls with 32bit IV/UV types, but I
> haven't been able to test them, since Debian's Perl uses 64bit integers
> even on 32bit platforms.

Ugh. I really don't want to maintain a separate expected-file for this,
especially not if it's going to be hard to test. Can we choose another
way of exercising the code path?

Another issue with this code as written is that on 32-bit-UV platforms,
at least some vompilers will give warnings about the constant-false
predicate. Not sure about a good solution for that. Maybe it's a
sufficient reason to invent uint8_numeric so we don't need a range check.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>>> I think you'd have to convert to text and back. That's kind of icky,
>>> but it beats failing.
>
>> I had a look, and that's what the PL/Python transform does. Attached is
>> a patch that does that for PL/Perl too, but only if the value is
>> actually > PG_INT64_MAX.
>
>> The secondary output files are for Perls with 32bit IV/UV types, but I
>> haven't been able to test them, since Debian's Perl uses 64bit integers
>> even on 32bit platforms.
>
> Ugh. I really don't want to maintain a separate expected-file for this,
> especially not if it's going to be hard to test. Can we choose another
> way of exercising the code path?

How about a plperl function that returns ~0 as numeric, and doing

select testuvtojsonb()::numeric = plperlu_maxuint();

in the test?

> Another issue with this code as written is that on 32-bit-UV platforms,
> at least some vompilers will give warnings about the constant-false
> predicate. Not sure about a good solution for that. Maybe it's a
> sufficient reason to invent uint8_numeric so we don't need a range check.

Yes, that does push the needle towards it being worth doing.

While playing around some more with the extension, I discoverered a few
more issues:

1) both the jsonb_plperl and jsonb_plperlu extensions contain the SQL
functions jsonb_to_plperl and plperl_to_jsonb, so can't be installed
simultaneously

2) jsonb scalar values are passed to the plperl function wrapped in not
one, but _two_ layers of references

3) jsonb numeric values are passed as perl's NV (floating point) type,
losing precision if they're integers that would fit in an IV or UV.

4) SV_to_JsonbValue() throws an error for infinite NVs, but not NaNs

Attached is a patch for the first issue. I'll look at fixing the rest
later this week.

- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law

ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> While playing around some more with the extension, I discoverered a few
> more issues:
> ...
> 4) SV_to_JsonbValue() throws an error for infinite NVs, but not NaNs

The others sound like bugs, but that one's intentional, since type
numeric does have a concept of NaN. If you're arguing that we should
disallow that value in the context of jsonb, maybe so, but it'd likely
take changes in quite a few more places than here.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> While playing around some more with the extension, I discoverered a few
>> more issues:
>> ...
>> 4) SV_to_JsonbValue() throws an error for infinite NVs, but not NaNs
>
> The others sound like bugs, but that one's intentional, since type
> numeric does have a concept of NaN. If you're arguing that we should
> disallow that value in the context of jsonb, maybe so, but it'd likely
> take changes in quite a few more places than here.

The numeric type that's used internally to represent numbers in jsonb
might have the concept of NaN, but JSON itself does not:

Numeric values that cannot be represented in the grammar below (such
as Infinity and NaN) are not permitted.

- https://tools.ietf.org/html/rfc7159#section-6

And it cannot be cast to json:

=# create or replace function jsonbnan() returns jsonb immutable language plperlu transform for type jsonb as '0+"NaN"';
CREATE FUNCTION
=# select jsonbnan();
┌──────────┐
│ jsonbnan │
├──────────┤
│ NaN │
└──────────┘

=# select jsonb_typeof(jsonbnan());
┌──────────────┐
│ jsonb_typeof │
├──────────────┤
│ number │
└──────────────┘

=# select jsonbnan()::json;
ERROR: invalid input syntax for type json
DETAIL: Token "NaN" is invalid.
CONTEXT: JSON data, line 1: NaN

- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law

ilmari(at)ilmari(dot)org (Dagfinn Ilmari Mannsåker) writes:

> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>
>> ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>>> While playing around some more with the extension, I discoverered a few
>>> more issues:
>>> ...
>>> 4) SV_to_JsonbValue() throws an error for infinite NVs, but not NaNs
>>
>> The others sound like bugs, but that one's intentional, since type
>> numeric does have a concept of NaN. If you're arguing that we should
>> disallow that value in the context of jsonb, maybe so, but it'd likely
>> take changes in quite a few more places than here.
>
> The numeric type that's used internally to represent numbers in jsonb
> might have the concept of NaN, but JSON itself does not:
>
> Numeric values that cannot be represented in the grammar below (such
> as Infinity and NaN) are not permitted.
>
> - https://tools.ietf.org/html/rfc7159#section-6
[…]
> =# create or replace function jsonbnan() returns jsonb immutable language plperlu transform for type jsonb as '0+"NaN"';
> CREATE FUNCTION
[…]
> =# select jsonbnan()::json;
> ERROR: invalid input syntax for type json
> DETAIL: Token "NaN" is invalid.
> CONTEXT: JSON data, line 1: NaN

Also, it doesn't parse back in as jsonb either:

=# select jsonbnan()::text::json;
ERROR: invalid input syntax for type json
DETAIL: Token "NaN" is invalid.
CONTEXT: JSON data, line 1: NaN

And it's inconsistent with to_jsonb():

=# select to_jsonb('nan'::numeric);
┌──────────┐
│ to_jsonb │
├──────────┤
│ "NaN" │
└──────────┘

It would be highly weird if PL transforms (jsonb_plpython does the same
thing) let you create spec-violating jsonb values that don't round-trip
via jsonb_out/in.

- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law

On 4/10/18 07:33, Dagfinn Ilmari Mannsåker wrote:
> 1) both the jsonb_plperl and jsonb_plperlu extensions contain the SQL
> functions jsonb_to_plperl and plperl_to_jsonb, so can't be installed
> simultaneously
>
> 2) jsonb scalar values are passed to the plperl function wrapped in not
> one, but _two_ layers of references
>
> 3) jsonb numeric values are passed as perl's NV (floating point) type,
> losing precision if they're integers that would fit in an IV or UV.
>
> 4) SV_to_JsonbValue() throws an error for infinite NVs, but not NaNs
>
> Attached is a patch for the first issue. I'll look at fixing the rest
> later this week.

Committed #1. Please keep more patches coming. :)

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

On 4/10/18 10:31, Dagfinn Ilmari Mannsåker wrote:
> Also, it doesn't parse back in as jsonb either:
>
> =# select jsonbnan()::text::json;
> ERROR: invalid input syntax for type json
> DETAIL: Token "NaN" is invalid.
> CONTEXT: JSON data, line 1: NaN
>
> And it's inconsistent with to_jsonb():
>
> =# select to_jsonb('nan'::numeric);
> ┌──────────┐
> │ to_jsonb │
> ├──────────┤
> │ "NaN" │
> └──────────┘
>
> It would be highly weird if PL transforms (jsonb_plpython does the same
> thing) let you create spec-violating jsonb values that don't round-trip
> via jsonb_out/in.

Yeah this is not good. Is there a way to do this in a centralized way?
Is there a function to check an internal jsonb value for consistency.
Should at least the jsonb output function check and not print invalid
values?

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

On 04/24/2018 12:17 PM, Peter Eisentraut wrote:
> On 4/10/18 10:31, Dagfinn Ilmari Mannsåker wrote:
>> Also, it doesn't parse back in as jsonb either:
>>
>> =# select jsonbnan()::text::json;
>> ERROR: invalid input syntax for type json
>> DETAIL: Token "NaN" is invalid.
>> CONTEXT: JSON data, line 1: NaN
>>
>> And it's inconsistent with to_jsonb():
>>
>> =# select to_jsonb('nan'::numeric);
>> ┌──────────┐
>> │ to_jsonb │
>> ├──────────┤
>> │ "NaN" │
>> └──────────┘
>>
>> It would be highly weird if PL transforms (jsonb_plpython does the same
>> thing) let you create spec-violating jsonb values that don't round-trip
>> via jsonb_out/in.
> Yeah this is not good. Is there a way to do this in a centralized way?
> Is there a function to check an internal jsonb value for consistency.
> Should at least the jsonb output function check and not print invalid
> values?
>

The output function fairly reasonably assumes that the jsonb is in a
form that would be parsed in by the input function. In particular, it
assumes that anything like a NaN will be stored as text and not as a
jsonb numeric. I don't think the transform should be doing anything
different from the input function.

There is the routine IsValidJsonNumber that helps - see among others
hstore_io.c for an example use.

cheers

andrew

--

Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 4/24/18 14:31, Andrew Dunstan wrote:
> There is the routine IsValidJsonNumber that helps - see among others
> hstore_io.c for an example use.

I would need something like that taking a double/float8 input. But I
think there is no such shortcut available, so I just wrote out the tests
for isinf and isnan explicitly. Attached patch should fix it.
jsonb_plpython will need a similar fix.

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

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> On 4/24/18 14:31, Andrew Dunstan wrote:
>> There is the routine IsValidJsonNumber that helps - see among others
>> hstore_io.c for an example use.

> I would need something like that taking a double/float8 input. But I
> think there is no such shortcut available, so I just wrote out the tests
> for isinf and isnan explicitly. Attached patch should fix it.
> jsonb_plpython will need a similar fix.

I looked this over, it looks fine to me. I first questioned the use
of ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE for rejecting NaN, but I see
that we are doing that in lots of comparable places (e.g., dtoi4())
so I'm on board with it.

regards, tom lane

On 4/29/18 14:28, Tom Lane wrote:
> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>> On 4/24/18 14:31, Andrew Dunstan wrote:
>>> There is the routine IsValidJsonNumber that helps - see among others
>>> hstore_io.c for an example use.
>
>> I would need something like that taking a double/float8 input. But I
>> think there is no such shortcut available, so I just wrote out the tests
>> for isinf and isnan explicitly. Attached patch should fix it.
>> jsonb_plpython will need a similar fix.
>
> I looked this over, it looks fine to me. I first questioned the use
> of ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE for rejecting NaN, but I see
> that we are doing that in lots of comparable places (e.g., dtoi4())
> so I'm on board with it.

Yeah, that was the idea.

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

These two items are now outstanding:

On 4/10/18 07:33, Dagfinn Ilmari Mannsåker wrote:
> 2) jsonb scalar values are passed to the plperl function wrapped in not
> one, but _two_ layers of references

I don't understand this one, or why it's a problem, or what to do about it.

> 3) jsonb numeric values are passed as perl's NV (floating point) type,
> losing precision if they're integers that would fit in an IV or UV.

This seems fixable, but perhaps we need to think through whether this
will result in other strange behaviors.

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

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:

> These two items are now outstanding:
>
> On 4/10/18 07:33, Dagfinn Ilmari Mannsåker wrote:
>> 2) jsonb scalar values are passed to the plperl function wrapped in not
>> one, but _two_ layers of references
>
> I don't understand this one, or why it's a problem, or what to do about it.

It means that if you call a jsonb-transforming pl/perl function like

select somefunc(jsonb '42');

it receives not the scalar 42, but reference to a reference to the
scalar (**int instead of an int, in C terms). This is not caught by the
current round-trip tests because the output transform automatically
dereferences any number of references on the way out again.

The fix is to reshuffle the newRV() calls in Jsonb_to_SV() and
jsonb_to_plperl(). I am working on a patch (and improved tests) for
this, but have not have had time to finish it yet. I hope be able to in
the next week or so.

>> 3) jsonb numeric values are passed as perl's NV (floating point) type,
>> losing precision if they're integers that would fit in an IV or UV.
>
> This seems fixable, but perhaps we need to think through whether this
> will result in other strange behaviors.

Nubers > 2⁵³ are not "interoperable" in the sense of the JSON spec,
because JavaScript only has doubles, but it seems desirable to preserve
whatever precision one reasonably can, and I can't think of any
downsides. We already support the full numeric range when processing
JSONB in SQL, it's just in the PL/Perl transform (and possibly
PL/Python, I didn't look) we're losing precision.

Perl can also be configured to use long double or __float128 (via
libquadmath) for its NV type, but I think preserving 64bit integers when
building against a Perl with a 64bit integer type would be sufficient.

- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law

Hello

This is still listed as an open item, though the patch proposed by Peter
upthread has been committed. If I understand correctly, ilmari was
going to propose another patch. Or is the right course of action to set
the open item as resolved?

On 2018-May-02, Dagfinn Ilmari Mannsåker wrote:

> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>
> > These two items are now outstanding:
> >
> > On 4/10/18 07:33, Dagfinn Ilmari Mannsåker wrote:
> >> 2) jsonb scalar values are passed to the plperl function wrapped in not
> >> one, but _two_ layers of references
> >
> > I don't understand this one, or why it's a problem, or what to do about it.
>
> It means that if you call a jsonb-transforming pl/perl function like
>
> select somefunc(jsonb '42');
>
> it receives not the scalar 42, but reference to a reference to the
> scalar (**int instead of an int, in C terms). This is not caught by the
> current round-trip tests because the output transform automatically
> dereferences any number of references on the way out again.
>
> The fix is to reshuffle the newRV() calls in Jsonb_to_SV() and
> jsonb_to_plperl(). I am working on a patch (and improved tests) for
> this, but have not have had time to finish it yet. I hope be able to in
> the next week or so.
>
> >> 3) jsonb numeric values are passed as perl's NV (floating point) type,
> >> losing precision if they're integers that would fit in an IV or UV.
> >
> > This seems fixable, but perhaps we need to think through whether this
> > will result in other strange behaviors.
>
> Nubers > 2⁵³ are not "interoperable" in the sense of the JSON spec,
> because JavaScript only has doubles, but it seems desirable to preserve
> whatever precision one reasonably can, and I can't think of any
> downsides. We already support the full numeric range when processing
> JSONB in SQL, it's just in the PL/Perl transform (and possibly
> PL/Python, I didn't look) we're losing precision.
>
> Perl can also be configured to use long double or __float128 (via
> libquadmath) for its NV type, but I think preserving 64bit integers when
> building against a Perl with a 64bit integer type would be sufficient.
>
> - ilmari
> --
> "The surreality of the universe tends towards a maximum" -- Skud's Law
> "Never formulate a law or axiom that you're not prepared to live with
> the consequences of." -- Skud's Meta-Law
>

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> This is still listed as an open item, though the patch proposed by Peter
> upthread has been committed. If I understand correctly, ilmari was
> going to propose another patch. Or is the right course of action to set
> the open item as resolved?

AIUI, ilmari complained about several things only some of which have been
resolved, so that this is still an open item. But I think the ball is in
his court to propose a patch.

regards, tom lane

On 5/17/18 17:11, Alvaro Herrera wrote:
> This is still listed as an open item, though the patch proposed by Peter
> upthread has been committed. If I understand correctly, ilmari was
> going to propose another patch. Or is the right course of action to set
> the open item as resolved?

The items that are still open from the original email are:

2) jsonb scalar values are passed to the plperl function wrapped in not
one, but _two_ layers of references

3) jsonb numeric values are passed as perl's NV (floating point) type,
losing precision if they're integers that would fit in an IV or UV.

#2 appears to be a quality of implementation issue without any
user-visible effects.

#3 is an opportunity for future improvement, but works as intended right
now.

I think patches for these issues could still be considered during beta,
but they are not release blockers IMO.

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

On Wed, 02 May 2018 17:41:38 +0100
ilmari(at)ilmari(dot)org (Dagfinn Ilmari Mannsåker) wrote:

> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>
> > These two items are now outstanding:
> >
> > On 4/10/18 07:33, Dagfinn Ilmari Mannsåker wrote:
> >> 2) jsonb scalar values are passed to the plperl function wrapped
> >> in not one, but _two_ layers of references
> >
> > I don't understand this one, or why it's a problem, or what to do
> > about it.
>
> It means that if you call a jsonb-transforming pl/perl function like
>
> select somefunc(jsonb '42');
>
> it receives not the scalar 42, but reference to a reference to the
> scalar (**int instead of an int, in C terms). This is not caught by
> the current round-trip tests because the output transform
> automatically dereferences any number of references on the way out
> again.
>
> The fix is to reshuffle the newRV() calls in Jsonb_to_SV() and
> jsonb_to_plperl(). I am working on a patch (and improved tests) for
> this, but have not have had time to finish it yet. I hope be able to
> in the next week or so.
>
> >> 3) jsonb numeric values are passed as perl's NV (floating point)
> >> type, losing precision if they're integers that would fit in an IV
> >> or UV.
> >
> > This seems fixable, but perhaps we need to think through whether
> > this will result in other strange behaviors.
>
> Nubers > 2⁵³ are not "interoperable" in the sense of the JSON spec,
> because JavaScript only has doubles, but it seems desirable to
> preserve whatever precision one reasonably can, and I can't think of
> any downsides. We already support the full numeric range when
> processing JSONB in SQL, it's just in the PL/Perl transform (and
> possibly PL/Python, I didn't look) we're losing precision.
>
> Perl can also be configured to use long double or __float128 (via
> libquadmath) for its NV type, but I think preserving 64bit integers
> when building against a Perl with a 64bit integer type would be
> sufficient.
>
> - ilmari

Hello,
need any help with the patch?

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2018-May-17, Peter Eisentraut wrote:

> The items that are still open from the original email are:
>
> 2) jsonb scalar values are passed to the plperl function wrapped in not
> one, but _two_ layers of references
>
> 3) jsonb numeric values are passed as perl's NV (floating point) type,
> losing precision if they're integers that would fit in an IV or UV.
>
> #2 appears to be a quality of implementation issue without any
> user-visible effects.
>
> #3 is an opportunity for future improvement, but works as intended right
> now.
>
> I think patches for these issues could still be considered during beta,
> but they are not release blockers IMO.

It appears to me that item #2 definitely would need to be fixed before
release, so that it doesn't become a backwards-incompatibility later on.

I'm not sure I agree that #3 is just a future feature. If you have
functions working with jsonb numeric giving exact results, and later
enable transforms for plperl, then your function starts giving inexact
results? Maybe I misunderstand the issue but this doesn't sound great.

Anyway, please let's move this forward. Peter, you own this item.
Anthony and ilmari, if you can help by providing a patch, I'm sure
that'll be appreciated.

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

On 6/6/18 12:14, Alvaro Herrera wrote:
> On 2018-May-17, Peter Eisentraut wrote:
>
>> The items that are still open from the original email are:
>>
>> 2) jsonb scalar values are passed to the plperl function wrapped in not
>> one, but _two_ layers of references
>>
>> 3) jsonb numeric values are passed as perl's NV (floating point) type,
>> losing precision if they're integers that would fit in an IV or UV.
>>
>> #2 appears to be a quality of implementation issue without any
>> user-visible effects.
>>
>> #3 is an opportunity for future improvement, but works as intended right
>> now.
>>
>> I think patches for these issues could still be considered during beta,
>> but they are not release blockers IMO.
>
> It appears to me that item #2 definitely would need to be fixed before
> release, so that it doesn't become a backwards-incompatibility later on.

The way I understand it, it's only how things are passed around
internally. Nothing is noticeable externally, and so there is no
backward compatibility issue.

At least that's how I understand it. So far this is only a claim by one
person. I haven't seen anything conclusive about whether there is an
actual issue.

> I'm not sure I agree that #3 is just a future feature. If you have
> functions working with jsonb numeric giving exact results, and later
> enable transforms for plperl, then your function starts giving inexact
> results? Maybe I misunderstand the issue but this doesn't sound great.

It would be the other way around. Right now, a transform from jsonb to
Perl would produce a float value in Perl. The argument is that it could
be an integer value in Perl if the original value fits. That's a change
worth considering, but the current behavior is consistent and works as
designed.

I took a brief look at this, and it seems there are some APIs needed to
be exposed from numeric.c to know whether a numeric is an integer.

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

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:

> On 6/6/18 12:14, Alvaro Herrera wrote:
>> On 2018-May-17, Peter Eisentraut wrote:
>>
>>> The items that are still open from the original email are:
>>>
>>> 2) jsonb scalar values are passed to the plperl function wrapped in not
>>> one, but _two_ layers of references
>>>
>>> 3) jsonb numeric values are passed as perl's NV (floating point) type,
>>> losing precision if they're integers that would fit in an IV or UV.
>>>
>>> #2 appears to be a quality of implementation issue without any
>>> user-visible effects.
>>>
>>> #3 is an opportunity for future improvement, but works as intended right
>>> now.
>>>
>>> I think patches for these issues could still be considered during beta,
>>> but they are not release blockers IMO.
>>
>> It appears to me that item #2 definitely would need to be fixed before
>> release, so that it doesn't become a backwards-incompatibility later on.
>
> The way I understand it, it's only how things are passed around
> internally. Nothing is noticeable externally, and so there is no
> backward compatibility issue.
>
> At least that's how I understand it. So far this is only a claim by one
> person. I haven't seen anything conclusive about whether there is an
> actual issue.

It's not just how things are passed internally, but how they are passed
to pl/perl functions with a jsonb transform: JSON scalar values at the
top level (strings, numbers, booleans, null) get passed as a reference
to a reference to the value, e.g. \\42 instead of 42. The reason the
current tests don't pick this up is that they don't check the value
inside the pl/perl functions, only that they roundtrip back to jsonb,
and the plperl to jsonb transform recursively dereferences references.

Another side effect of the recursive dereferencing is that returning
undef from the perl function returns an SQL NULL while returning a
reference to undef (\undef) returns a jsonb null.

The attached patch fixes the excess enreferencing, but does not touch
the dereferencing part.

- ilmari
--
"I use RMS as a guide in the same way that a boat captain would use
a lighthouse. It's good to know where it is, but you generally
don't want to find yourself in the same spot." - Tollef Fog Heen

ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>> The way I understand it, it's only how things are passed around
>> internally. Nothing is noticeable externally, and so there is no
>> backward compatibility issue.
>>
>> At least that's how I understand it. So far this is only a claim by one
>> person. I haven't seen anything conclusive about whether there is an
>> actual issue.

> It's not just how things are passed internally, but how they are passed
> to pl/perl functions with a jsonb transform: JSON scalar values at the
> top level (strings, numbers, booleans, null) get passed as a reference
> to a reference to the value, e.g. \\42 instead of 42. The reason the
> current tests don't pick this up is that they don't check the value
> inside the pl/perl functions, only that they roundtrip back to jsonb,
> and the plperl to jsonb transform recursively dereferences references.

Yeah, the reason this is important is that it affects what the plperl
function body sees.

> Another side effect of the recursive dereferencing is that returning
> undef from the perl function returns an SQL NULL while returning a
> reference to undef (\undef) returns a jsonb null.

Hm, I think you're blaming the wrong moving part there. The way the
transform logic is set up (e.g., in plperl_sv_to_datum()), it's
impossible for a transform function to return a SQL null; the decision by
plperl_sv_to_datum as to whether or not the output will be a SQL null is
final. (Perhaps that was a mistake, but changing the transform function
API seems like a rather Big Deal.) So if we think that \undef ought to
produce a SQL null, the thing to do is move the dereferencing loop to
the beginning of plperl_sv_to_datum, and then \undef would produce NULL
whether this transform is installed or not. I don't have a well-informed
opinion on whether that's a good idea, but I tend to the view that it is.
Right now the case produces an error, and not even a very sane one:

regression=# create function foo() returns int language plperlu
regression-# as '\undef';
CREATE FUNCTION
regression=# select foo();
ERROR: PL/Perl function must return reference to hash or array
CONTEXT: PL/Perl function "foo"

so there's not really a compatibility break if we redefine it.

regards, tom lane

I wrote:
> ... So if we think that \undef ought to
> produce a SQL null, the thing to do is move the dereferencing loop to
> the beginning of plperl_sv_to_datum, and then \undef would produce NULL
> whether this transform is installed or not. I don't have a well-informed
> opinion on whether that's a good idea, but I tend to the view that it is.
> Right now the case produces an error, and not even a very sane one:

> regression=# create function foo() returns int language plperlu
> regression-# as '\undef';
> CREATE FUNCTION
> regression=# select foo();
> ERROR: PL/Perl function must return reference to hash or array
> CONTEXT: PL/Perl function "foo"

> so there's not really a compatibility break if we redefine it.

After further thought, the only argument I can think of for preserving
this existing behavior is if we wanted to reserve returning a reference-
to-scalar for some future purpose, ie make it do something different
from returning the referenced value. I can't think of any likely use
of that kind, but maybe I'm just insufficiently creative today.

However, if one makes that argument, then it is clearly a bad idea for
jsonb_plperl to be forcibly dereferencing such references: once we do make
a change of that sort, jsonb_plperl will be out of step with the behavior
for every other datatype, or else we will need to make a subtle
compatibility break to align it with whatever the new behavior is.

So it seems that whichever way you stand on that, it's wrong to have
that dereference loop in SV_to_JsonbValue(). I'm forced to the
conclusion that that's just a hack to band-aid over a bug in the
transform's other direction.

Now, if we did decide that auto-dereferencing should be the general
rule in perl->SQL conversions, I'd be inclined to leave that loop
in place in SV_to_JsonbValue(), because it would be covering the case
where jsonb_plperl is recursively disassembling an AV or HV and finds
a reference-to-scalar. But we also need a dereference loop in at least
one place in plperl.c itself if that's the plan.

I'm inclined to think that auto-dereference is indeed a good idea,
and am tempted to go make that change to make all this consistent.
Comments?

regards, tom lane

I wrote:
> I'm inclined to think that auto-dereference is indeed a good idea,
> and am tempted to go make that change to make all this consistent.
> Comments?

Here's a draft patch for that. I ended up only changing
plperl_sv_to_datum. There is maybe a case for doing something
similar in plperl_return_next_internal's fn_retistuple code path,
so that you can return a reference-to-reference-to-hash there;
but I was unable to muster much enthusiasm for touching that.

regards, tom lane

On 2018-Jun-08, Tom Lane wrote:

> I wrote:
> > I'm inclined to think that auto-dereference is indeed a good idea,
> > and am tempted to go make that change to make all this consistent.
> > Comments?
>
> Here's a draft patch for that. I ended up only changing
> plperl_sv_to_datum. There is maybe a case for doing something
> similar in plperl_return_next_internal's fn_retistuple code path,
> so that you can return a reference-to-reference-to-hash there;
> but I was unable to muster much enthusiasm for touching that.

ilmari, did you have time to give Tom's patch a spin?

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

ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> [ 0001-Fix-excess-enreferencing-in-plperl-jsonb-transform.patch ]

I tested this a bit more thoroughly by dint of applying Data::Dumper
to the Perl values, and found that we were still getting extra references
to sub-objects, for example

INFO: $VAR1 = {'1' => \{'2' => \['3','4','5']},'2' => '3'};

where what we want is

INFO: $VAR1 = {'1' => {'2' => ['3','4','5']},'2' => '3'};

That turns out to be because the newRV() call in JsonbValue_to_SV()
is also superfluous, if we've set up refs around HV and AV scalars.

Pushed with that change and the extra testing technology. I'll go
push the dereferencing patch I proposed shortly, as well.

The remaining unresolved issue in this thread is Ilmari's suggestion
that we should convert integers to Perl IV (or UV?) if they fit, rather
than always convert to NV as now. I'm inclined to reject that proposal,
though, and not just because we don't have a patch for it. What's
bothering me about it is that then the behavior would be dependent
on the width of IV in the particular Perl installation. I think that
is a platform dependency we can do without.

regards, tom lane

I wrote:
> The remaining unresolved issue in this thread is Ilmari's suggestion
> that we should convert integers to Perl IV (or UV?) if they fit, rather
> than always convert to NV as now.

Oh ... after re-reading the thread I realized there was one other point
that we'd all forgotten about, namely the business about ~0 getting
converted to -1 rather than what Perl interprets it as. Ilmari sent in
a patch for that, against which I'd raised two complaints:

1. Possible compiler complaints about a constant-false comparison,
on machines where type UV is 32 bits.

2. Need for secondary expected-output files, which'd be a pain to
maintain.

I realized that point 1 could be dealt with just by not trying to be
smart, but always using the convert-to-text code path. Given that it
seems to be hard to produce a UV value in Perl, I doubt it is worth
working any harder than that. Also, point 2 could be dealt with in
this perhaps crude way:

-- this might produce either 18446744073709551615 or 4294967295
SELECT testUVToJsonb() IN ('18446744073709551615'::jsonb, '4294967295'::jsonb);

Pushed with those adjustments.

regards, tom lane