Transform for pl/perl

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Transform for pl/perl

Anthony Bykov
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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

0001-jsonb_plperl-extension.patch (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Anthony Bykov
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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Robert Haas
In reply to this post by Anthony Bykov
On Tue, Oct 24, 2017 at 1:01 PM, anthony <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Pavel Stehule
In reply to this post by Anthony Bykov
Hi

2017-10-24 14:27 GMT+02:00 Anthony Bykov <[hidden email]>:
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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Anthony Bykov
On Fri, 10 Nov 2017 14:40:21 +0100
Pavel Stehule <[hidden email]> wrote:

> Hi
>
> 2017-10-24 14:27 GMT+02:00 Anthony Bykov <[hidden email]>:
>
> > 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 ([hidden email])
> > 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

0001-jsonb_plperl-extension-v2.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Pavel Stehule

Hi


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

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

I changed few lines in regress tests.

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

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

I have not any other objections

Regards

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


0001-jsonb_plperl-extension-v3.patch (42K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Aleksander Alekseev
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
Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Oleg Bartunov-2
In reply to this post by Anthony Bykov


On 14 Nov 2017 11:35, "Anthony Bykov" <[hidden email]> wrote:
On Fri, 10 Nov 2017 14:40:21 +0100
Pavel Stehule <[hidden email]> wrote:

> Hi
>
> 2017-10-24 14:27 GMT+02:00 Anthony Bykov <[hidden email]>:
>
> > 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 ([hidden email])
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
> >

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

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

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


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

Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Pavel Stehule


2017-11-15 10:24 GMT+01:00 Oleg Bartunov <[hidden email]>:


On 14 Nov 2017 11:35, "Anthony Bykov" <[hidden email]> wrote:
On Fri, 10 Nov 2017 14:40:21 +0100
Pavel Stehule <[hidden email]> wrote:

> Hi
>
> 2017-10-24 14:27 GMT+02:00 Anthony Bykov <[hidden email]>:
>
> > 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 ([hidden email])
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
> >

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

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

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

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

But some performance check can be interesting

Regards

Pavel


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


Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Aleksander Alekseev
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Anthony Bykov
In reply to this post by Aleksander Alekseev
On Wed, 15 Nov 2017 08:58:54 +0000
Aleksander Alekseev <[hidden email]> 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

0001-jsonb_plperl-extension-v4.patch (33K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Aleksander Alekseev
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
Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Michael Paquier
On Tue, Nov 28, 2017 at 5:14 PM, Aleksander Alekseev
<[hidden email]> wrote:
> The new status of this patch is: Ready for Committer

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

Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Robert Haas
On Fri, Dec 1, 2017 at 12:30 AM, Michael Paquier
<[hidden email]> wrote:
> On Tue, Nov 28, 2017 at 5:14 PM, Aleksander Alekseev
> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Tom Lane-2
Robert Haas <[hidden email]> writes:
> On Fri, Dec 1, 2017 at 12:30 AM, Michael Paquier
> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Álvaro Herrera
In reply to this post by Anthony Bykov
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

Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Andrew Dunstan-8
In reply to this post by Robert Haas


On 12/01/2017 11:37 AM, Robert Haas wrote:

> On Fri, Dec 1, 2017 at 12:30 AM, Michael Paquier
> <[hidden email]> wrote:
>> On Tue, Nov 28, 2017 at 5:14 PM, Aleksander Alekseev
>> <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Anthony Bykov
In reply to this post by Álvaro Herrera
On Fri, 1 Dec 2017 15:49:21 -0300
Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Anthony Bykov
On Thu, 7 Dec 2017 12:54:55 +0300
Anthony Bykov <[hidden email]> wrote:

> On Fri, 1 Dec 2017 15:49:21 -0300
> Alvaro Herrera <[hidden email]> 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

0001-jsonb_plperl-extension-v5.patch (30K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Transform for pl/perl

Peter Eisentraut-6
In reply to this post by Tom Lane-2
On 12/1/17 13:15, Tom Lane wrote:

> Robert Haas <[hidden email]> writes:
>> On Fri, Dec 1, 2017 at 12:30 AM, Michael Paquier
>> <[hidden email]> 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

Previous Thread Next Thread