Transform for pl/perl

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

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

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

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

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hello Anthony,

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

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

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

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

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

The new status of this patch is: Waiting on Author

On 14 Nov 2017 11:35, "Anthony Bykov" <a.bykov@postgrespro.ru> wrote:

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

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

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

2017-11-15 10:24 GMT+01:00 Oleg Bartunov <obartunov@gmail.com>:

On 14 Nov 2017 11:35, "Anthony Bykov" <a.bykov@postgrespro.ru> wrote:

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

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

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 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.
--
Michael

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