Thread: [HACKERS] 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.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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);


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

On Tue, Oct 24, 2017 at 1:01 PM, anthony <a.bykov@postgrespro.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.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.stehule@gmail.com> wrote:

> Hi
>
> 2017-10-24 14:27 GMT+02:00 Anthony Bykov <a.bykov@postgrespro.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@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@postgresql.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

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, 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.alekseev@postgrespro.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 Fri, Dec 1, 2017 at 12:30 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Nov 28, 2017 at 5:14 PM, Aleksander Alekseev
> <a.alekseev@postgrespro.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@gmail.com> writes:
> On Fri, Dec 1, 2017 at 12:30 AM, Michael Paquier
> <michael.paquier@gmail.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.paquier@gmail.com> wrote:
>> On Tue, Nov 28, 2017 at 5:14 PM, Aleksander Alekseev
>> <a.alekseev@postgrespro.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@alvh.no-ip.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.bykov@postgrespro.ru> wrote:

> On Fri, 1 Dec 2017 15:49:21 -0300
> Alvaro Herrera <alvherre@alvh.no-ip.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@gmail.com> writes:
>> On Fri, Dec 1, 2017 at 12:30 AM, Michael Paquier
>> <michael.paquier@gmail.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.bykov@postgrespro.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.munro@enterprisedb.com> wrote:

> On Thu, Dec 7, 2017 at 10:56 PM, Anthony Bykov
> <a.bykov@postgrespro.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.bykov@postgrespro.ru> wrote:
> On Fri, 12 Jan 2018 15:19:26 +1300
> Thomas Munro <thomas.munro@enterprisedb.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.dunstan@2ndquadrant.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.zakirov@postgrespro.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

On Mon, 5 Mar 2018 14:03:37 +0100
Pavel Stehule <pavel.stehule@gmail.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

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.bykov@postgrespro.ru>:

On Mon, 5 Mar 2018 14:03:37 +0100
Pavel Stehule <pavel.stehule@gmail.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.

> 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

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

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.eisentraut@2ndquadrant.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@[local]:5432 ~=# CREATE FUNCTION testUVToJsonb() RETURNS jsonb
    ilmari@[local]:5432 ~-# LANGUAGE plperl TRANSFORM FOR TYPE jsonb
    ilmari@[local]:5432 ~-# as $$
    ilmari@[local]:5432 ~$# $val = ~0;
    ilmari@[local]:5432 ~$# elog(NOTICE, "value is $val");
    ilmari@[local]:5432 ~$# return $val;
    ilmari@[local]:5432 ~$# $$;
    CREATE FUNCTION
    Time: 6.795 ms
    ilmari@[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@ilmari.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@sss.pgh.pa.us> writes:

> ilmari@ilmari.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

From acf968b4df81797fc06868dac87123413f3f4167 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Thu, 5 Apr 2018 16:23:59 +0100
Subject: [PATCH] Handle integers > PG_INT64_MAX in PL/Perl JSONB transform

---
 .../jsonb_plperl/expected/jsonb_plperl.out    |  15 +-
 .../jsonb_plperl/expected/jsonb_plperl_1.out  | 223 ++++++++++++++++++
 .../jsonb_plperl/expected/jsonb_plperlu.out   |  15 +-
 .../jsonb_plperl/expected/jsonb_plperlu_1.out | 223 ++++++++++++++++++
 contrib/jsonb_plperl/jsonb_plperl.c           |  20 +-
 contrib/jsonb_plperl/sql/jsonb_plperl.sql     |   9 +
 contrib/jsonb_plperl/sql/jsonb_plperlu.sql    |  10 +
 7 files changed, 512 insertions(+), 3 deletions(-)
 create mode 100644 contrib/jsonb_plperl/expected/jsonb_plperl_1.out
 create mode 100644 contrib/jsonb_plperl/expected/jsonb_plperlu_1.out

diff --git a/contrib/jsonb_plperl/expected/jsonb_plperl.out b/contrib/jsonb_plperl/expected/jsonb_plperl.out
index 99a2e8e135..c311a603f0 100644
--- a/contrib/jsonb_plperl/expected/jsonb_plperl.out
+++ b/contrib/jsonb_plperl/expected/jsonb_plperl.out
@@ -52,6 +52,19 @@ SELECT testRegexpResultToJsonb();
  0
 (1 row)
 
+CREATE FUNCTION testUVToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+as $$
+$val = ~0;
+return $val;
+$$;
+SELECT testUVToJsonb();
+    testuvtojsonb     
+----------------------
+ 18446744073709551615
+(1 row)
+
 CREATE FUNCTION roundtrip(val jsonb) RETURNS jsonb
 LANGUAGE plperl
 TRANSFORM FOR TYPE jsonb
@@ -207,4 +220,4 @@ SELECT roundtrip('{"1": {"2": [3, 4, 5]}, "2": 3}');
 
 \set VERBOSITY terse \\ -- suppress cascade details
 DROP EXTENSION plperl CASCADE;
-NOTICE:  drop cascades to 6 other objects
+NOTICE:  drop cascades to 7 other objects
diff --git a/contrib/jsonb_plperl/expected/jsonb_plperl_1.out b/contrib/jsonb_plperl/expected/jsonb_plperl_1.out
new file mode 100644
index 0000000000..c425c73b9c
--- /dev/null
+++ b/contrib/jsonb_plperl/expected/jsonb_plperl_1.out
@@ -0,0 +1,223 @@
+CREATE EXTENSION jsonb_plperl CASCADE;
+NOTICE:  installing required extension "plperl"
+CREATE FUNCTION testHVToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = {a => 1, b => 'boo', c => undef};
+return $val;
+$$;
+SELECT testHVToJsonb();
+          testhvtojsonb          
+---------------------------------
+ {"a": 1, "b": "boo", "c": null}
+(1 row)
+
+CREATE FUNCTION testAVToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = [{a => 1, b => 'boo', c => undef}, {d => 2}];
+return $val;
+$$;
+SELECT testAVToJsonb();
+                testavtojsonb                
+---------------------------------------------
+ [{"a": 1, "b": "boo", "c": null}, {"d": 2}]
+(1 row)
+
+CREATE FUNCTION testSVToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = 1;
+return $val;
+$$;
+SELECT testSVToJsonb();
+ testsvtojsonb 
+---------------
+ 1
+(1 row)
+
+-- this revealed a bug in the original implementation
+CREATE FUNCTION testRegexpResultToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+return ('1' =~ m(0\t2));
+$$;
+SELECT testRegexpResultToJsonb();
+ testregexpresulttojsonb 
+-------------------------
+ 0
+(1 row)
+
+CREATE FUNCTION testUVToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+as $$
+$val = ~0;
+return $val;
+$$;
+SELECT testUVToJsonb();
+ testuvtojsonb 
+---------------
+ 4294967295
+(1 row)
+
+CREATE FUNCTION roundtrip(val jsonb) RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+return $_[0];
+$$;
+SELECT roundtrip('null');
+ roundtrip 
+-----------
+ null
+(1 row)
+
+SELECT roundtrip('1');
+ roundtrip 
+-----------
+ 1
+(1 row)
+
+SELECT roundtrip('1E+131071');
+ERROR:  cannot convert infinite value to jsonb
+CONTEXT:  PL/Perl function "roundtrip"
+SELECT roundtrip('-1');
+ roundtrip 
+-----------
+ -1
+(1 row)
+
+SELECT roundtrip('1.2');
+ roundtrip 
+-----------
+ 1.2
+(1 row)
+
+SELECT roundtrip('-1.2');
+ roundtrip 
+-----------
+ -1.2
+(1 row)
+
+SELECT roundtrip('"string"');
+ roundtrip 
+-----------
+ "string"
+(1 row)
+
+SELECT roundtrip('"NaN"');
+ roundtrip 
+-----------
+ "NaN"
+(1 row)
+
+SELECT roundtrip('true');
+ roundtrip 
+-----------
+ 1
+(1 row)
+
+SELECT roundtrip('false');
+ roundtrip 
+-----------
+ 0
+(1 row)
+
+SELECT roundtrip('[]');
+ roundtrip 
+-----------
+ []
+(1 row)
+
+SELECT roundtrip('[null, null]');
+  roundtrip   
+--------------
+ [null, null]
+(1 row)
+
+SELECT roundtrip('[1, 2, 3]');
+ roundtrip 
+-----------
+ [1, 2, 3]
+(1 row)
+
+SELECT roundtrip('[-1, 2, -3]');
+  roundtrip  
+-------------
+ [-1, 2, -3]
+(1 row)
+
+SELECT roundtrip('[1.2, 2.3, 3.4]');
+    roundtrip    
+-----------------
+ [1.2, 2.3, 3.4]
+(1 row)
+
+SELECT roundtrip('[-1.2, 2.3, -3.4]');
+     roundtrip     
+-------------------
+ [-1.2, 2.3, -3.4]
+(1 row)
+
+SELECT roundtrip('["string1", "string2"]');
+       roundtrip        
+------------------------
+ ["string1", "string2"]
+(1 row)
+
+SELECT roundtrip('{}');
+ roundtrip 
+-----------
+ {}
+(1 row)
+
+SELECT roundtrip('{"1": null}');
+  roundtrip  
+-------------
+ {"1": null}
+(1 row)
+
+SELECT roundtrip('{"1": 1}');
+ roundtrip 
+-----------
+ {"1": 1}
+(1 row)
+
+SELECT roundtrip('{"1": -1}');
+ roundtrip 
+-----------
+ {"1": -1}
+(1 row)
+
+SELECT roundtrip('{"1": 1.1}');
+ roundtrip  
+------------
+ {"1": 1.1}
+(1 row)
+
+SELECT roundtrip('{"1": -1.1}');
+  roundtrip  
+-------------
+ {"1": -1.1}
+(1 row)
+
+SELECT roundtrip('{"1": "string1"}');
+    roundtrip     
+------------------
+ {"1": "string1"}
+(1 row)
+
+SELECT roundtrip('{"1": {"2": [3, 4, 5]}, "2": 3}');
+            roundtrip            
+---------------------------------
+ {"1": {"2": [3, 4, 5]}, "2": 3}
+(1 row)
+
+\set VERBOSITY terse \\ -- suppress cascade details
+DROP EXTENSION plperl CASCADE;
+NOTICE:  drop cascades to 7 other objects
diff --git a/contrib/jsonb_plperl/expected/jsonb_plperlu.out b/contrib/jsonb_plperl/expected/jsonb_plperlu.out
index 8053cf6aa8..c4f7caf4c1 100644
--- a/contrib/jsonb_plperl/expected/jsonb_plperlu.out
+++ b/contrib/jsonb_plperl/expected/jsonb_plperlu.out
@@ -52,6 +52,19 @@ SELECT testRegexpResultToJsonb();
  0
 (1 row)
 
+CREATE FUNCTION testUVToJsonb() RETURNS jsonb
+LANGUAGE plperlu
+TRANSFORM FOR TYPE jsonb
+as $$
+$val = ~0;
+return $val;
+$$;
+SELECT testUVToJsonb();
+    testuvtojsonb     
+----------------------
+ 18446744073709551615
+(1 row)
+
 CREATE FUNCTION roundtrip(val jsonb) RETURNS jsonb
 LANGUAGE plperlu
 TRANSFORM FOR TYPE jsonb
@@ -207,4 +220,4 @@ SELECT roundtrip('{"1": {"2": [3, 4, 5]}, "2": 3}');
 
 \set VERBOSITY terse \\ -- suppress cascade details
 DROP EXTENSION plperlu CASCADE;
-NOTICE:  drop cascades to 6 other objects
+NOTICE:  drop cascades to 7 other objects
diff --git a/contrib/jsonb_plperl/expected/jsonb_plperlu_1.out b/contrib/jsonb_plperl/expected/jsonb_plperlu_1.out
new file mode 100644
index 0000000000..6bebc1ce3d
--- /dev/null
+++ b/contrib/jsonb_plperl/expected/jsonb_plperlu_1.out
@@ -0,0 +1,223 @@
+CREATE EXTENSION jsonb_plperlu CASCADE;
+NOTICE:  installing required extension "plperlu"
+CREATE FUNCTION testHVToJsonb() RETURNS jsonb
+LANGUAGE plperlu
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = {a => 1, b => 'boo', c => undef};
+return $val;
+$$;
+SELECT testHVToJsonb();
+          testhvtojsonb          
+---------------------------------
+ {"a": 1, "b": "boo", "c": null}
+(1 row)
+
+CREATE FUNCTION testAVToJsonb() RETURNS jsonb
+LANGUAGE plperlu
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = [{a => 1, b => 'boo', c => undef}, {d => 2}];
+return $val;
+$$;
+SELECT testAVToJsonb();
+                testavtojsonb                
+---------------------------------------------
+ [{"a": 1, "b": "boo", "c": null}, {"d": 2}]
+(1 row)
+
+CREATE FUNCTION testSVToJsonb() RETURNS jsonb
+LANGUAGE plperlu
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = 1;
+return $val;
+$$;
+SELECT testSVToJsonb();
+ testsvtojsonb 
+---------------
+ 1
+(1 row)
+
+-- this revealed a bug in the original implementation
+CREATE FUNCTION testRegexpResultToJsonb() RETURNS jsonb
+LANGUAGE plperlu
+TRANSFORM FOR TYPE jsonb
+AS $$
+return ('1' =~ m(0\t2));
+$$;
+SELECT testRegexpResultToJsonb();
+ testregexpresulttojsonb 
+-------------------------
+ 0
+(1 row)
+
+CREATE FUNCTION testUVToJsonb() RETURNS jsonb
+LANGUAGE plperlu
+TRANSFORM FOR TYPE jsonb
+as $$
+$val = ~0;
+return $val;
+$$;
+SELECT testUVToJsonb();
+ testuvtojsonb 
+---------------
+ 4294967295
+(1 row)
+
+CREATE FUNCTION roundtrip(val jsonb) RETURNS jsonb
+LANGUAGE plperlu
+TRANSFORM FOR TYPE jsonb
+AS $$
+return $_[0];
+$$;
+SELECT roundtrip('null');
+ roundtrip 
+-----------
+ null
+(1 row)
+
+SELECT roundtrip('1');
+ roundtrip 
+-----------
+ 1
+(1 row)
+
+SELECT roundtrip('1E+131071');
+ERROR:  cannot convert infinite value to jsonb
+CONTEXT:  PL/Perl function "roundtrip"
+SELECT roundtrip('-1');
+ roundtrip 
+-----------
+ -1
+(1 row)
+
+SELECT roundtrip('1.2');
+ roundtrip 
+-----------
+ 1.2
+(1 row)
+
+SELECT roundtrip('-1.2');
+ roundtrip 
+-----------
+ -1.2
+(1 row)
+
+SELECT roundtrip('"string"');
+ roundtrip 
+-----------
+ "string"
+(1 row)
+
+SELECT roundtrip('"NaN"');
+ roundtrip 
+-----------
+ "NaN"
+(1 row)
+
+SELECT roundtrip('true');
+ roundtrip 
+-----------
+ 1
+(1 row)
+
+SELECT roundtrip('false');
+ roundtrip 
+-----------
+ 0
+(1 row)
+
+SELECT roundtrip('[]');
+ roundtrip 
+-----------
+ []
+(1 row)
+
+SELECT roundtrip('[null, null]');
+  roundtrip   
+--------------
+ [null, null]
+(1 row)
+
+SELECT roundtrip('[1, 2, 3]');
+ roundtrip 
+-----------
+ [1, 2, 3]
+(1 row)
+
+SELECT roundtrip('[-1, 2, -3]');
+  roundtrip  
+-------------
+ [-1, 2, -3]
+(1 row)
+
+SELECT roundtrip('[1.2, 2.3, 3.4]');
+    roundtrip    
+-----------------
+ [1.2, 2.3, 3.4]
+(1 row)
+
+SELECT roundtrip('[-1.2, 2.3, -3.4]');
+     roundtrip     
+-------------------
+ [-1.2, 2.3, -3.4]
+(1 row)
+
+SELECT roundtrip('["string1", "string2"]');
+       roundtrip        
+------------------------
+ ["string1", "string2"]
+(1 row)
+
+SELECT roundtrip('{}');
+ roundtrip 
+-----------
+ {}
+(1 row)
+
+SELECT roundtrip('{"1": null}');
+  roundtrip  
+-------------
+ {"1": null}
+(1 row)
+
+SELECT roundtrip('{"1": 1}');
+ roundtrip 
+-----------
+ {"1": 1}
+(1 row)
+
+SELECT roundtrip('{"1": -1}');
+ roundtrip 
+-----------
+ {"1": -1}
+(1 row)
+
+SELECT roundtrip('{"1": 1.1}');
+ roundtrip  
+------------
+ {"1": 1.1}
+(1 row)
+
+SELECT roundtrip('{"1": -1.1}');
+  roundtrip  
+-------------
+ {"1": -1.1}
+(1 row)
+
+SELECT roundtrip('{"1": "string1"}');
+    roundtrip     
+------------------
+ {"1": "string1"}
+(1 row)
+
+SELECT roundtrip('{"1": {"2": [3, 4, 5]}, "2": 3}');
+            roundtrip            
+---------------------------------
+ {"1": {"2": [3, 4, 5]}, "2": 3}
+(1 row)
+
+\set VERBOSITY terse \\ -- suppress cascade details
+DROP EXTENSION plperlu CASCADE;
+NOTICE:  drop cascades to 7 other objects
diff --git a/contrib/jsonb_plperl/jsonb_plperl.c b/contrib/jsonb_plperl/jsonb_plperl.c
index 837bae2ab5..63bc547c88 100644
--- a/contrib/jsonb_plperl/jsonb_plperl.c
+++ b/contrib/jsonb_plperl/jsonb_plperl.c
@@ -196,7 +196,25 @@ SV_to_JsonbValue(SV *in, JsonbParseState **jsonb_state, bool is_elem)
             break;
 
         default:
-            if (SvIOK(in))
+            if (SvUOK(in))
+            {
+                UV            uval = SvUV(in);
+
+                out.type = jbvNumeric;
+                if (uval > PG_INT64_MAX)
+                {
+                    const char *strval = SvPV_nolen(in);
+
+                    out.val.numeric =
+                        DatumGetNumeric(DirectFunctionCall3(numeric_in,
+                                                            CStringGetDatum(strval), 0, -1));
+                }
+                else
+                    out.val.numeric =
+                        DatumGetNumeric(DirectFunctionCall1(int8_numeric,
+                                                            Int64GetDatum((int64) uval)));
+            }
+            else if (SvIOK(in))
             {
                 IV            ival = SvIV(in);
 
diff --git a/contrib/jsonb_plperl/sql/jsonb_plperl.sql b/contrib/jsonb_plperl/sql/jsonb_plperl.sql
index 8b0a8764af..7b0c7683d2 100644
--- a/contrib/jsonb_plperl/sql/jsonb_plperl.sql
+++ b/contrib/jsonb_plperl/sql/jsonb_plperl.sql
@@ -44,6 +44,15 @@ $$;
 
 SELECT testRegexpResultToJsonb();
 
+CREATE FUNCTION testUVToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+as $$
+$val = ~0;
+return $val;
+$$;
+
+SELECT testUVToJsonb();
 
 CREATE FUNCTION roundtrip(val jsonb) RETURNS jsonb
 LANGUAGE plperl
diff --git a/contrib/jsonb_plperl/sql/jsonb_plperlu.sql b/contrib/jsonb_plperl/sql/jsonb_plperlu.sql
index 9287f7672f..a68e7f1b4d 100644
--- a/contrib/jsonb_plperl/sql/jsonb_plperlu.sql
+++ b/contrib/jsonb_plperl/sql/jsonb_plperlu.sql
@@ -45,6 +45,16 @@ $$;
 SELECT testRegexpResultToJsonb();
 
 
+CREATE FUNCTION testUVToJsonb() RETURNS jsonb
+LANGUAGE plperlu
+TRANSFORM FOR TYPE jsonb
+as $$
+$val = ~0;
+return $val;
+$$;
+
+SELECT testUVToJsonb();
+
 CREATE FUNCTION roundtrip(val jsonb) RETURNS jsonb
 LANGUAGE plperlu
 TRANSFORM FOR TYPE jsonb
-- 
2.17.0

ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> writes:

> ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> Tom Lane <tgl@sss.pgh.pa.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

From cf5771f98aa33bc7f12054d65857a3cc347465dc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Tue, 10 Apr 2018 10:57:50 +0100
Subject: [PATCH] Fix clashing function names bettween jsonb_plperl and
 jsonb_plperlu

---
 contrib/jsonb_plperl/jsonb_plperlu--1.0.sql | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/contrib/jsonb_plperl/jsonb_plperlu--1.0.sql b/contrib/jsonb_plperl/jsonb_plperlu--1.0.sql
index 99b8644e3b..5a5e475ad3 100644
--- a/contrib/jsonb_plperl/jsonb_plperlu--1.0.sql
+++ b/contrib/jsonb_plperl/jsonb_plperlu--1.0.sql
@@ -3,17 +3,17 @@
 -- complain if script is sourced in psql, rather than via CREATE EXTENSION
 \echo Use "CREATE EXTENSION jsonb_plperlu" to load this file. \quit
 
-CREATE FUNCTION jsonb_to_plperl(val internal) RETURNS internal
+CREATE FUNCTION jsonb_to_plperlu(val internal) RETURNS internal
 LANGUAGE C STRICT IMMUTABLE
-AS 'MODULE_PATHNAME';
+AS 'MODULE_PATHNAME', 'jsonb_to_plperl';
 
-CREATE FUNCTION plperl_to_jsonb(val internal) RETURNS jsonb
+CREATE FUNCTION plperlu_to_jsonb(val internal) RETURNS jsonb
 LANGUAGE C STRICT IMMUTABLE
-AS 'MODULE_PATHNAME';
+AS 'MODULE_PATHNAME', 'plperl_to_jsonb';
 
 CREATE TRANSFORM FOR jsonb LANGUAGE plperlu (
-    FROM SQL WITH FUNCTION jsonb_to_plperl(internal),
-    TO SQL WITH FUNCTION plperl_to_jsonb(internal)
+    FROM SQL WITH FUNCTION jsonb_to_plperlu(internal),
+    TO SQL WITH FUNCTION plperlu_to_jsonb(internal)
 );
 
 COMMENT ON TRANSFORM FOR jsonb LANGUAGE plperlu IS 'transform between jsonb and Perl';
-- 
2.17.0

ilmari@ilmari.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@sss.pgh.pa.us> writes:

> ilmari@ilmari.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@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

> Tom Lane <tgl@sss.pgh.pa.us> writes:
>
>> ilmari@ilmari.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.eisentraut@2ndquadrant.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.eisentraut@2ndquadrant.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.eisentraut@2ndquadrant.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.eisentraut@2ndquadrant.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@2ndquadrant.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@ilmari.org (Dagfinn Ilmari Mannsåker) wrote:

> Peter Eisentraut <peter.eisentraut@2ndquadrant.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.eisentraut@2ndquadrant.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

From e0948ca29055241874ba455d39728da66fdf3fd5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Tue, 17 Apr 2018 18:18:31 +0100
Subject: [PATCH] Fix excess enreferencing in plperl jsonb transform

---
 .../jsonb_plperl/expected/jsonb_plperl.out    | 42 +++++++++----------
 .../jsonb_plperl/expected/jsonb_plperlu.out   | 36 ++++++++--------
 contrib/jsonb_plperl/jsonb_plperl.c           |  8 ++--
 contrib/jsonb_plperl/sql/jsonb_plperl.sql     | 39 ++++++++---------
 contrib/jsonb_plperl/sql/jsonb_plperlu.sql    | 38 +++++++++--------
 5 files changed, 82 insertions(+), 81 deletions(-)

diff --git a/contrib/jsonb_plperl/expected/jsonb_plperl.out b/contrib/jsonb_plperl/expected/jsonb_plperl.out
index e4f3cdd41a..3364057a64 100644
--- a/contrib/jsonb_plperl/expected/jsonb_plperl.out
+++ b/contrib/jsonb_plperl/expected/jsonb_plperl.out
@@ -52,16 +52,18 @@ SELECT testRegexpResultToJsonb();
  0
 (1 row)
 
-CREATE FUNCTION roundtrip(val jsonb) RETURNS jsonb
+CREATE FUNCTION roundtrip(val jsonb, ref text = '') RETURNS jsonb
 LANGUAGE plperl
 TRANSFORM FOR TYPE jsonb
 AS $$
+die 'unexpected '.(ref($_[0]) || 'not a').' reference'
+    if ref($_[0]) ne $_[1];
 return $_[0];
 $$;
 SELECT roundtrip('null');
  roundtrip 
 -----------
- null
+ 
 (1 row)
 
 SELECT roundtrip('1');
@@ -97,12 +99,6 @@ SELECT roundtrip('"string"');
  "string"
 (1 row)
 
-SELECT roundtrip('"NaN"');
- roundtrip 
------------
- "NaN"
-(1 row)
-
 SELECT roundtrip('true');
  roundtrip 
 -----------
@@ -115,91 +111,91 @@ SELECT roundtrip('false');
  0
 (1 row)
 
-SELECT roundtrip('[]');
+SELECT roundtrip('[]', 'ARRAY');
  roundtrip 
 -----------
  []
 (1 row)
 
-SELECT roundtrip('[null, null]');
+SELECT roundtrip('[null, null]', 'ARRAY');
   roundtrip   
 --------------
  [null, null]
 (1 row)
 
-SELECT roundtrip('[1, 2, 3]');
+SELECT roundtrip('[1, 2, 3]', 'ARRAY');
  roundtrip 
 -----------
  [1, 2, 3]
 (1 row)
 
-SELECT roundtrip('[-1, 2, -3]');
+SELECT roundtrip('[-1, 2, -3]', 'ARRAY');
   roundtrip  
 -------------
  [-1, 2, -3]
 (1 row)
 
-SELECT roundtrip('[1.2, 2.3, 3.4]');
+SELECT roundtrip('[1.2, 2.3, 3.4]', 'ARRAY');
     roundtrip    
 -----------------
  [1.2, 2.3, 3.4]
 (1 row)
 
-SELECT roundtrip('[-1.2, 2.3, -3.4]');
+SELECT roundtrip('[-1.2, 2.3, -3.4]', 'ARRAY');
      roundtrip     
 -------------------
  [-1.2, 2.3, -3.4]
 (1 row)
 
-SELECT roundtrip('["string1", "string2"]');
+SELECT roundtrip('["string1", "string2"]', 'ARRAY');
        roundtrip        
 ------------------------
  ["string1", "string2"]
 (1 row)
 
-SELECT roundtrip('{}');
+SELECT roundtrip('{}', 'HASH');
  roundtrip 
 -----------
  {}
 (1 row)
 
-SELECT roundtrip('{"1": null}');
+SELECT roundtrip('{"1": null}', 'HASH');
   roundtrip  
 -------------
  {"1": null}
 (1 row)
 
-SELECT roundtrip('{"1": 1}');
+SELECT roundtrip('{"1": 1}', 'HASH');
  roundtrip 
 -----------
  {"1": 1}
 (1 row)
 
-SELECT roundtrip('{"1": -1}');
+SELECT roundtrip('{"1": -1}', 'HASH');
  roundtrip 
 -----------
  {"1": -1}
 (1 row)
 
-SELECT roundtrip('{"1": 1.1}');
+SELECT roundtrip('{"1": 1.1}', 'HASH');
  roundtrip  
 ------------
  {"1": 1.1}
 (1 row)
 
-SELECT roundtrip('{"1": -1.1}');
+SELECT roundtrip('{"1": -1.1}', 'HASH');
   roundtrip  
 -------------
  {"1": -1.1}
 (1 row)
 
-SELECT roundtrip('{"1": "string1"}');
+SELECT roundtrip('{"1": "string1"}', 'HASH');
     roundtrip     
 ------------------
  {"1": "string1"}
 (1 row)
 
-SELECT roundtrip('{"1": {"2": [3, 4, 5]}, "2": 3}');
+SELECT roundtrip('{"1": {"2": [3, 4, 5]}, "2": 3}', 'HASH');
             roundtrip            
 ---------------------------------
  {"1": {"2": [3, 4, 5]}, "2": 3}
diff --git a/contrib/jsonb_plperl/expected/jsonb_plperlu.out b/contrib/jsonb_plperl/expected/jsonb_plperlu.out
index b44995058f..c222e74e2d 100644
--- a/contrib/jsonb_plperl/expected/jsonb_plperlu.out
+++ b/contrib/jsonb_plperl/expected/jsonb_plperlu.out
@@ -52,16 +52,18 @@ SELECT testRegexpResultToJsonb();
  0
 (1 row)
 
-CREATE FUNCTION roundtrip(val jsonb) RETURNS jsonb
+CREATE FUNCTION roundtrip(val jsonb, ref text = '') RETURNS jsonb
 LANGUAGE plperlu
 TRANSFORM FOR TYPE jsonb
 AS $$
+die 'unexpected '.(ref($_[0]) || 'not a').' reference'
+    if ref($_[0]) ne $_[1];
 return $_[0];
 $$;
 SELECT roundtrip('null');
  roundtrip 
 -----------
- null
+ 
 (1 row)
 
 SELECT roundtrip('1');
@@ -115,91 +117,91 @@ SELECT roundtrip('false');
  0
 (1 row)
 
-SELECT roundtrip('[]');
+SELECT roundtrip('[]', 'ARRAY');
  roundtrip 
 -----------
  []
 (1 row)
 
-SELECT roundtrip('[null, null]');
+SELECT roundtrip('[null, null]', 'ARRAY');
   roundtrip   
 --------------
  [null, null]
 (1 row)
 
-SELECT roundtrip('[1, 2, 3]');
+SELECT roundtrip('[1, 2, 3]', 'ARRAY');
  roundtrip 
 -----------
  [1, 2, 3]
 (1 row)
 
-SELECT roundtrip('[-1, 2, -3]');
+SELECT roundtrip('[-1, 2, -3]', 'ARRAY');
   roundtrip  
 -------------
  [-1, 2, -3]
 (1 row)
 
-SELECT roundtrip('[1.2, 2.3, 3.4]');
+SELECT roundtrip('[1.2, 2.3, 3.4]', 'ARRAY');
     roundtrip    
 -----------------
  [1.2, 2.3, 3.4]
 (1 row)
 
-SELECT roundtrip('[-1.2, 2.3, -3.4]');
+SELECT roundtrip('[-1.2, 2.3, -3.4]', 'ARRAY');
      roundtrip     
 -------------------
  [-1.2, 2.3, -3.4]
 (1 row)
 
-SELECT roundtrip('["string1", "string2"]');
+SELECT roundtrip('["string1", "string2"]', 'ARRAY');
        roundtrip        
 ------------------------
  ["string1", "string2"]
 (1 row)
 
-SELECT roundtrip('{}');
+SELECT roundtrip('{}', 'HASH');
  roundtrip 
 -----------
  {}
 (1 row)
 
-SELECT roundtrip('{"1": null}');
+SELECT roundtrip('{"1": null}', 'HASH');
   roundtrip  
 -------------
  {"1": null}
 (1 row)
 
-SELECT roundtrip('{"1": 1}');
+SELECT roundtrip('{"1": 1}', 'HASH');
  roundtrip 
 -----------
  {"1": 1}
 (1 row)
 
-SELECT roundtrip('{"1": -1}');
+SELECT roundtrip('{"1": -1}', 'HASH');
  roundtrip 
 -----------
  {"1": -1}
 (1 row)
 
-SELECT roundtrip('{"1": 1.1}');
+SELECT roundtrip('{"1": 1.1}', 'HASH');
  roundtrip  
 ------------
  {"1": 1.1}
 (1 row)
 
-SELECT roundtrip('{"1": -1.1}');
+SELECT roundtrip('{"1": -1.1}', 'HASH');
   roundtrip  
 -------------
  {"1": -1.1}
 (1 row)
 
-SELECT roundtrip('{"1": "string1"}');
+SELECT roundtrip('{"1": "string1"}', 'HASH');
     roundtrip     
 ------------------
  {"1": "string1"}
 (1 row)
 
-SELECT roundtrip('{"1": {"2": [3, 4, 5]}, "2": 3}');
+SELECT roundtrip('{"1": {"2": [3, 4, 5]}, "2": 3}', 'HASH');
             roundtrip            
 ---------------------------------
  {"1": {"2": [3, 4, 5]}, "2": 3}
diff --git a/contrib/jsonb_plperl/jsonb_plperl.c b/contrib/jsonb_plperl/jsonb_plperl.c
index bde93a71fc..85d5584e07 100644
--- a/contrib/jsonb_plperl/jsonb_plperl.c
+++ b/contrib/jsonb_plperl/jsonb_plperl.c
@@ -83,7 +83,7 @@ Jsonb_to_SV(JsonbContainer *jsonb)
                     (r = JsonbIteratorNext(&it, &tmp, true)) != WJB_DONE)
                     elog(ERROR, "unexpected jsonb token: %d", r);
 
-                return newRV(JsonbValue_to_SV(&v));
+                return JsonbValue_to_SV(&v);
             }
             else
             {
@@ -95,7 +95,7 @@ Jsonb_to_SV(JsonbContainer *jsonb)
                         av_push(av, JsonbValue_to_SV(&v));
                 }
 
-                return (SV *) av;
+                return newRV((SV *) av);
             }
 
         case WJB_BEGIN_OBJECT:
@@ -120,7 +120,7 @@ Jsonb_to_SV(JsonbContainer *jsonb)
                     }
                 }
 
-                return (SV *) hv;
+                return newRV((SV *) hv);
             }
 
         default:
@@ -268,7 +268,7 @@ jsonb_to_plperl(PG_FUNCTION_ARGS)
     Jsonb       *in = PG_GETARG_JSONB_P(0);
     SV           *sv = Jsonb_to_SV(&in->root);
 
-    return PointerGetDatum(newRV(sv));
+    return PointerGetDatum(sv);
 }
 
 
diff --git a/contrib/jsonb_plperl/sql/jsonb_plperl.sql b/contrib/jsonb_plperl/sql/jsonb_plperl.sql
index 8b0a8764af..e49a997cba 100644
--- a/contrib/jsonb_plperl/sql/jsonb_plperl.sql
+++ b/contrib/jsonb_plperl/sql/jsonb_plperl.sql
@@ -45,10 +45,12 @@ $$;
 SELECT testRegexpResultToJsonb();

 
-CREATE FUNCTION roundtrip(val jsonb) RETURNS jsonb
+CREATE FUNCTION roundtrip(val jsonb, ref text = '') RETURNS jsonb
 LANGUAGE plperl
 TRANSFORM FOR TYPE jsonb
 AS $$
+die 'unexpected '.(ref($_[0]) || 'not a').' reference'
+    if ref($_[0]) ne $_[1];
 return $_[0];
 $$;
 
@@ -60,28 +62,27 @@ SELECT roundtrip('-1');
 SELECT roundtrip('1.2');
 SELECT roundtrip('-1.2');
 SELECT roundtrip('"string"');
-SELECT roundtrip('"NaN"');
 
 SELECT roundtrip('true');
 SELECT roundtrip('false');
 
-SELECT roundtrip('[]');
-SELECT roundtrip('[null, null]');
-SELECT roundtrip('[1, 2, 3]');
-SELECT roundtrip('[-1, 2, -3]');
-SELECT roundtrip('[1.2, 2.3, 3.4]');
-SELECT roundtrip('[-1.2, 2.3, -3.4]');
-SELECT roundtrip('["string1", "string2"]');
-
-SELECT roundtrip('{}');
-SELECT roundtrip('{"1": null}');
-SELECT roundtrip('{"1": 1}');
-SELECT roundtrip('{"1": -1}');
-SELECT roundtrip('{"1": 1.1}');
-SELECT roundtrip('{"1": -1.1}');
-SELECT roundtrip('{"1": "string1"}');
-
-SELECT roundtrip('{"1": {"2": [3, 4, 5]}, "2": 3}');
+SELECT roundtrip('[]', 'ARRAY');
+SELECT roundtrip('[null, null]', 'ARRAY');
+SELECT roundtrip('[1, 2, 3]', 'ARRAY');
+SELECT roundtrip('[-1, 2, -3]', 'ARRAY');
+SELECT roundtrip('[1.2, 2.3, 3.4]', 'ARRAY');
+SELECT roundtrip('[-1.2, 2.3, -3.4]', 'ARRAY');
+SELECT roundtrip('["string1", "string2"]', 'ARRAY');
+
+SELECT roundtrip('{}', 'HASH');
+SELECT roundtrip('{"1": null}', 'HASH');
+SELECT roundtrip('{"1": 1}', 'HASH');
+SELECT roundtrip('{"1": -1}', 'HASH');
+SELECT roundtrip('{"1": 1.1}', 'HASH');
+SELECT roundtrip('{"1": -1.1}', 'HASH');
+SELECT roundtrip('{"1": "string1"}', 'HASH');
+
+SELECT roundtrip('{"1": {"2": [3, 4, 5]}, "2": 3}', 'HASH');
 
 
 \set VERBOSITY terse \\ -- suppress cascade details
diff --git a/contrib/jsonb_plperl/sql/jsonb_plperlu.sql b/contrib/jsonb_plperl/sql/jsonb_plperlu.sql
index 9287f7672f..0c19e9066a 100644
--- a/contrib/jsonb_plperl/sql/jsonb_plperlu.sql
+++ b/contrib/jsonb_plperl/sql/jsonb_plperlu.sql
@@ -45,10 +45,12 @@ $$;
 SELECT testRegexpResultToJsonb();
 
 
-CREATE FUNCTION roundtrip(val jsonb) RETURNS jsonb
+CREATE FUNCTION roundtrip(val jsonb, ref text = '') RETURNS jsonb
 LANGUAGE plperlu
 TRANSFORM FOR TYPE jsonb
 AS $$
+die 'unexpected '.(ref($_[0]) || 'not a').' reference'
+    if ref($_[0]) ne $_[1];
 return $_[0];
 $$;
 
@@ -65,23 +67,23 @@ SELECT roundtrip('"NaN"');
 SELECT roundtrip('true');
 SELECT roundtrip('false');
 
-SELECT roundtrip('[]');
-SELECT roundtrip('[null, null]');
-SELECT roundtrip('[1, 2, 3]');
-SELECT roundtrip('[-1, 2, -3]');
-SELECT roundtrip('[1.2, 2.3, 3.4]');
-SELECT roundtrip('[-1.2, 2.3, -3.4]');
-SELECT roundtrip('["string1", "string2"]');
-
-SELECT roundtrip('{}');
-SELECT roundtrip('{"1": null}');
-SELECT roundtrip('{"1": 1}');
-SELECT roundtrip('{"1": -1}');
-SELECT roundtrip('{"1": 1.1}');
-SELECT roundtrip('{"1": -1.1}');
-SELECT roundtrip('{"1": "string1"}');
-
-SELECT roundtrip('{"1": {"2": [3, 4, 5]}, "2": 3}');
+SELECT roundtrip('[]', 'ARRAY');
+SELECT roundtrip('[null, null]', 'ARRAY');
+SELECT roundtrip('[1, 2, 3]', 'ARRAY');
+SELECT roundtrip('[-1, 2, -3]', 'ARRAY');
+SELECT roundtrip('[1.2, 2.3, 3.4]', 'ARRAY');
+SELECT roundtrip('[-1.2, 2.3, -3.4]', 'ARRAY');
+SELECT roundtrip('["string1", "string2"]', 'ARRAY');
+
+SELECT roundtrip('{}', 'HASH');
+SELECT roundtrip('{"1": null}', 'HASH');
+SELECT roundtrip('{"1": 1}', 'HASH');
+SELECT roundtrip('{"1": -1}', 'HASH');
+SELECT roundtrip('{"1": 1.1}', 'HASH');
+SELECT roundtrip('{"1": -1.1}', 'HASH');
+SELECT roundtrip('{"1": "string1"}', 'HASH');
+
+SELECT roundtrip('{"1": {"2": [3, 4, 5]}, "2": 3}', 'HASH');
 
 
 \set VERBOSITY terse \\ -- suppress cascade details
-- 
2.17.1

ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.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

diff --git a/src/pl/plperl/expected/plperl.out b/src/pl/plperl/expected/plperl.out
index ebfba3e..d8a1ff5 100644
*** a/src/pl/plperl/expected/plperl.out
--- b/src/pl/plperl/expected/plperl.out
*************** $$ LANGUAGE plperl;
*** 763,776 ****
  SELECT text_obj();
  ERROR:  cannot convert Perl hash to non-composite type text
  CONTEXT:  PL/Perl function "text_obj"
! ----- make sure we can't return a scalar ref
  CREATE OR REPLACE FUNCTION text_scalarref() RETURNS text AS $$
      my $str = 'str';
      return \$str;
  $$ LANGUAGE plperl;
  SELECT text_scalarref();
! ERROR:  PL/Perl function must return reference to hash or array
! CONTEXT:  PL/Perl function "text_scalarref"
  -- check safe behavior when a function body is replaced during execution
  CREATE OR REPLACE FUNCTION self_modify(INTEGER) RETURNS INTEGER AS $$
     spi_exec_query('CREATE OR REPLACE FUNCTION self_modify(INTEGER) RETURNS INTEGER AS \'return $_[0] * 3;\' LANGUAGE
plperl;');
--- 763,779 ----
  SELECT text_obj();
  ERROR:  cannot convert Perl hash to non-composite type text
  CONTEXT:  PL/Perl function "text_obj"
! -- test looking through a scalar ref
  CREATE OR REPLACE FUNCTION text_scalarref() RETURNS text AS $$
      my $str = 'str';
      return \$str;
  $$ LANGUAGE plperl;
  SELECT text_scalarref();
!  text_scalarref
! ----------------
!  str
! (1 row)
!
  -- check safe behavior when a function body is replaced during execution
  CREATE OR REPLACE FUNCTION self_modify(INTEGER) RETURNS INTEGER AS $$
     spi_exec_query('CREATE OR REPLACE FUNCTION self_modify(INTEGER) RETURNS INTEGER AS \'return $_[0] * 3;\' LANGUAGE
plperl;');
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 4342c02..4cfc506 100644
*** a/src/pl/plperl/plperl.c
--- b/src/pl/plperl/plperl.c
*************** plperl_sv_to_datum(SV *sv, Oid typid, in
*** 1402,1412 ****
              return ret;
          }

!         /* Reference, but not reference to hash or array ... */
!         ereport(ERROR,
!                 (errcode(ERRCODE_DATATYPE_MISMATCH),
!                  errmsg("PL/Perl function must return reference to hash or array")));
!         return (Datum) 0;        /* shut up compiler */
      }
      else
      {
--- 1402,1414 ----
              return ret;
          }

!         /*
!          * If it's a reference to something else, such as a scalar, just
!          * recursively look through the reference.
!          */
!         return plperl_sv_to_datum(SvRV(sv), typid, typmod,
!                                   fcinfo, finfo, typioparam,
!                                   isnull);
      }
      else
      {
diff --git a/src/pl/plperl/sql/plperl.sql b/src/pl/plperl/sql/plperl.sql
index c36da0f..b0d950b 100644
*** a/src/pl/plperl/sql/plperl.sql
--- b/src/pl/plperl/sql/plperl.sql
*************** $$ LANGUAGE plperl;
*** 504,510 ****

  SELECT text_obj();

! ----- make sure we can't return a scalar ref
  CREATE OR REPLACE FUNCTION text_scalarref() RETURNS text AS $$
      my $str = 'str';
      return \$str;
--- 504,510 ----

  SELECT text_obj();

! -- test looking through a scalar ref
  CREATE OR REPLACE FUNCTION text_scalarref() RETURNS text AS $$
      my $str = 'str';
      return \$str;

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@ilmari.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