Re: Transform for pl/perl

[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

Re: Transform for pl/perl

[Tom Lane]

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

Re: Transform for pl/perl

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Transform for pl/perl

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

Re: Transform for pl/perl

[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

Re: Transform for pl/perl

[Tom Lane]

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

Re: Transform for pl/perl

[Dagfinn Ilmari Mannsåker]

Peter Eisentraut  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?= 
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');
   

Re: Transform for pl/perl

[Peter Eisentraut]

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

Re: Transform for pl/perl

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Transform for pl/perl

[Anthony Bykov]

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

> Peter Eisentraut <peter.eisentr...@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

Re: Transform for pl/perl

[Peter Eisentraut]

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

Re: Transform for pl/perl

[Tom Lane]

Alvaro Herrera  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

Re: Transform for pl/perl

[Alvaro Herrera]

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.eisentr...@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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Transform for pl/perl

[Dagfinn Ilmari Mannsåker]

Peter Eisentraut <peter.eisentr...@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

Re: Transform for pl/perl

[Peter Eisentraut]

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

Re: Transform for pl/perl

[Peter Eisentraut]

On 4/29/18 14:28, Tom Lane wrote:
> Peter Eisentraut  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

Re: Transform for pl/perl

[Tom Lane]

Peter Eisentraut  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

Re: Transform for pl/perl

[Peter Eisentraut]

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
From ec8529348008e58826442e43809772c19d02a067 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 26 Apr 2018 15:20:43 -0400
Subject: [PATCH] Prevent infinity and NaN in jsonb/plperl transform

jsonb uses numeric internally, and numeric can store NaN, but that is
not allowed by jsonb on input, so we shouldn't store it.  Also prevent
infinity to get a consistent error message.  (numeric input would reject
infinity anyway.)
---
 .../jsonb_plperl/expected/jsonb_plperl.out| 24 +--
 .../jsonb_plperl/expected/jsonb_plperlu.out   | 24 +--
 contrib/jsonb_plperl/jsonb_plperl.c   | 16 +++--
 contrib/jsonb_plperl/sql/jsonb_plperl.sql | 23 +-
 contrib/jsonb_plperl/sql/jsonb_plperlu.sql| 22 +
 5 files changed, 102 insertions(+), 7 deletions(-)

diff --git a/contrib/jsonb_plperl/expected/jsonb_plperl.out 
b/contrib/jsonb_plperl/expected/jsonb_plperl.out
index 99a2e8e135..d6c3becf63 100644
--- a/contrib/jsonb_plperl/expected/jsonb_plperl.out
+++ b/contrib/jsonb_plperl/expected/jsonb_plperl.out
@@ -39,6 +39,26 @@ SELECT testSVToJsonb();
  1
 (1 row)
 
+CREATE FUNCTION testInf() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = 0 + 'Inf';
+return $val;
+$$;
+SELECT testInf();
+ERROR:  cannot convert infinity to jsonb
+CONTEXT:  PL/Perl function "testinf"
+CREATE FUNCTION testNaN() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = 0 + 'NaN';
+return $val;
+$$;
+SELECT testNaN();
+ERROR:  cannot convert NaN to jsonb
+CONTEXT:  PL/Perl function "testnan"
 -- this revealed a bug in the original implementation
 CREATE FUNCTION testRegexpResultToJsonb() RETURNS jsonb
 LANGUAGE plperl
@@ -71,7 +91,7 @@ SELECT roundtrip('1');
 (1 row)
 
 SELECT roundtrip('1E+131071');
-ERROR:  cannot convert infinite value to jsonb
+ERROR:  cannot convert infinity to jsonb
 CONTEXT:  PL/Perl function "roundtrip"
 SELECT roundtrip('-1');
  roundtrip 
@@ -207,4 +227,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 8 other objects
diff --git a/contrib/jsonb_plperl/expected/jsonb_plperlu.out 
b/contrib/jsonb_plperl/expected/jsonb_plperlu.out
index 8053cf6aa8..65ed21f3b2 100644
--- a/contrib/jsonb_plperl/expected/jsonb_plperlu.out
+++ b/contrib/jsonb_plperl/expected/jsonb_plperlu.out
@@ -39,6 +39,26 @@ SELECT testSVToJsonb();
  1
 (1 row)
 
+CREATE FUNCTION testInf() RETURNS jsonb
+LANGUAGE plperlu
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = 0 + 'Inf';
+return $val;
+$$;
+SELECT testInf();
+ERROR:  cannot convert infinity to jsonb
+CONTEXT:  PL/Perl function "testinf"
+CREATE FUNCTION testNaN() RETURNS jsonb
+LANGUAGE plperlu
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = 0 + 'NaN';
+return $val;
+$$;
+SELECT testNaN();
+ERROR:  cannot convert NaN to jsonb
+CONTEXT:  PL/Perl function "testnan"
 -- this revealed a bug in the original implementation
 CREATE FUNCTION testRegexpResultToJsonb() RETURNS jsonb
 LANGUAGE plperlu
@@ -71,7 +91,7 @@ SELECT roundtrip('1');
 (1 row)
 
 SELECT roundtrip('1E+131071');
-ERROR:  cannot convert infinite value to jsonb
+ERROR:  cannot convert infinity to jsonb
 CONTEXT:  PL/Perl function "roundtrip"
 SELECT roundtrip('-1');
  roundtrip 
@@ -207,4 +227,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 8 other objects
diff --git a/contrib/jsonb_plperl/jsonb_plperl.c 
b/contrib/jsonb_plperl/jsonb_plperl.c
index 837bae2ab5..3f9802c696 100644
--- a/contrib/jsonb_plperl/jsonb_plperl.c
+++ b/contrib/jsonb_plperl/jsonb_plperl.c
@@ -209,10 +209,22 @@ SV_to_JsonbValue(SV *in, JsonbParseState **jsonb_state, 
bool is_elem)
{
double  nval = SvNV(in);
 
+   /*
+* jsonb doesn't allow infinity or NaN (per JSON
+* specification), but the numeric type that is 
used for the
+* storage accepts NaN, so we have to prevent 
it here
+* explicitly.  We don't really have to check 
for isinf()
+* here, as 

Re: Transform for pl/perl

[Andrew Dunstan]

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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Transform for pl/perl

[Peter Eisentraut]

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

Re: Transform for pl/perl

[Peter Eisentraut]

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

Re: Transform for pl/perl

[Dagfinn Ilmari Mannsåker]

ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

> Tom Lane  writes:
>
>> ilm...@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

Re: Transform for pl/perl

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

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

Re: Transform for pl/perl

[Tom Lane]

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

Re: Transform for pl/perl

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

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

Re: Transform for pl/perl

[Tom Lane]

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

Re: Transform for pl/perl

[Dagfinn Ilmari Mannsåker]

Tom Lane <t...@sss.pgh.pa.us> writes:

> ilm...@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?= <ilm...@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 00..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

Re: Transform for pl/perl

[Tom Lane]

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

Re: Transform for pl/perl

[Dagfinn Ilmari Mannsåker]

Peter Eisentraut  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

Re: Transform for pl/perl

[Peter Eisentraut]

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

Re: Transform for pl/perl

[Pavel Stehule]

Hi

2018-03-13 15:50 GMT+01:00 Nikita Glukhov <n.glu...@postgrespro.ru>:

> Hi.
>
> I have reviewed this patch too.  Attached new version with v8-v9 delta-patch.
>
> Here is my changes:
>
>  * HV_ToJsonbValue():
> - addded missing hv_iterinit()
> - used hv_iternextsv() instead of hv_iternext(), HeSVKEY_force(), HeVAL()
>
>  * SV_ToJsonbValue():
> - added recursive dereferencing for all SV types
> - removed unnecessary JsonbValue heap-allocations
>
>  * Jsonb_ToSV():
> - added iteration to the end of iterator needed for correct freeing of
>   JsonbIterators
>
>  * passed JsonbParseState ** to XX_ToJsonbValue() functions.
>
>  * fixed warnings (see below)
>
>  * fixed comments (see below)
>
>
> Also I am not sure if we need to use newRV() for returning SVs in
> Jsonb_ToSV() and JsonbValue_ToSV().
>
>
> On 12.03.2018 18:06, Pavel Stehule wrote:
>
> 2018-03-12 9:08 GMT+01:00 Anthony Bykov <a.by...@postgrespro.ru>:
>
>> On Mon, 5 Mar 2018 14:03:37 +0100
>> Pavel Stehule <pavel.steh...@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.
>>
>> pg_unreachable() is replaced with elog(ERROR) for reporting impossible
> JsonbValue types and JsonbIteratorTokens.
>
> > 3. Do we need two identical tests fro PLPerl and PLPerlu? Why?
>> >
>> > Regards
>> >
>> > Pavel
>>
>> About the 3 point - I thought that plperlu and plperl uses different
>> interpreters. And if they act identically on same examples - there
>> is no need in identical tests for them indeed.
>>
>
> plperlu and plperl uses same interprets - so the duplicate tests has not
> any sense. But in last versions there are duplicate tests again
>
> I have not removed duplicate test yet, because I am not sure that this
> test does not make sense at all.
>

ok .. the commiter can decide it

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

It looks well

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

I'll mark this patch as ready for commiter

Regards

Pavel

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

Re: Transform for pl/perl

[Nikita Glukhov]

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.by...@postgrespro.ru 
<mailto:a.by...@postgrespro.ru>>:


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


pg_unreachable() is replaced with elog(ERROR) for reporting impossible
JsonbValue types and JsonbIteratorTokens.


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

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


plperlu and plperl uses same interprets - so the duplicate tests has 
not any sense. But in last versions there are duplicate tests again
I have not removed duplicate test yet, because I am not sure that this 
test does not make sense at all.



The naming convention of functions is not consistent

almost are are src_to_dest

This is different and it is little bit messy

+static SV  *
+SV_FromJsonb(JsonbContainer *jsonb)


Renamed to Jsonb_ToSV() and JsonbValue_ToSV().


This comment is broken

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

Fixed.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/contrib/Makefile b/contrib/Makefile
index 8046ca4..53d44fe 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -75,9 +75,9 @@ ALWAYS_SUBDIRS += sepgsql
 endif
 
 ifeq ($(with_perl),yes)
-SUBDIRS += hstore_plperl
+SUBDIRS += hstore_plperl jsonb_plperl
 else
-ALWAYS_SUBDIRS += hstore_plperl
+ALWAYS_SUBDIRS += hstore_plperl jsonb_plperl
 endif
 
 ifeq ($(with_python),yes)
diff --git a/contrib/jsonb_plperl/Makefile b/contrib/jsonb_plperl/Makefile
new file mode 100644
index 000..cd86553
--- /dev/null
+++ b/contrib/jsonb_plperl/Makefile
@@ -0,0 +1,40 @@
+# contrib/jsonb_plperl/Makefile
+
+MODULE_big = jsonb_plperl
+OBJS = jsonb_plperl.o $(WIN32RES)
+PGFILEDESC = "jsonb_plperl - jsonb transform for plperl"
+
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl
+
+EXTENSION = jsonb_plperlu jsonb_plperl
+DATA = jsonb_plperlu--1.0.sql jsonb_plperl--1.0.sql
+
+REGRESS = jsonb_plperl jsonb_plperl_relocatability jsonb_plperlu jsonb_plperlu_relocatability
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/jsonb_plperl
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+# We must link libperl explicitly
+ifeq ($(PORTNAME), win32)
+# these settings are the same as for plperl
+override CPPFLAGS += -DPLPERL_HAVE_UID_GID -Wno-comment
+# ... see silliness in plperl Makefile ...
+SHLIB_LINK += $(sort $(wildcard ../../src/pl/plperl/libperl*.a))
+else
+rpathdir = $(perl_archlibexp)/CORE
+SHLIB_LINK += $(perl_embed_ldflags)
+endif
+
+# As with plperl we need to make sure that the CORE directory is included
+# last, probably because it sometimes contai

Re: Transform for pl/perl

[Anthony Bykov]

On Mon, 5 Mar 2018 14:03:37 +0100
Pavel Stehule <pavel.steh...@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
diff --git a/contrib/Makefile b/contrib/Makefile
index 8046ca4..53d44fe 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -75,9 +75,9 @@ ALWAYS_SUBDIRS += sepgsql
 endif
 
 ifeq ($(with_perl),yes)
-SUBDIRS += hstore_plperl
+SUBDIRS += hstore_plperl jsonb_plperl
 else
-ALWAYS_SUBDIRS += hstore_plperl
+ALWAYS_SUBDIRS += hstore_plperl jsonb_plperl
 endif
 
 ifeq ($(with_python),yes)
diff --git a/contrib/jsonb_plperl/Makefile b/contrib/jsonb_plperl/Makefile
new file mode 100644
index 000..cd86553
--- /dev/null
+++ b/contrib/jsonb_plperl/Makefile
@@ -0,0 +1,40 @@
+# contrib/jsonb_plperl/Makefile
+
+MODULE_big = jsonb_plperl
+OBJS = jsonb_plperl.o $(WIN32RES)
+PGFILEDESC = "jsonb_plperl - jsonb transform for plperl"
+
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl
+
+EXTENSION = jsonb_plperlu jsonb_plperl
+DATA = jsonb_plperlu--1.0.sql jsonb_plperl--1.0.sql
+
+REGRESS = jsonb_plperl jsonb_plperl_relocatability jsonb_plperlu jsonb_plperlu_relocatability
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/jsonb_plperl
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+# We must link libperl explicitly
+ifeq ($(PORTNAME), win32)
+# these settings are the same as for plperl
+override CPPFLAGS += -DPLPERL_HAVE_UID_GID -Wno-comment
+# ... see silliness in plperl Makefile ...
+SHLIB_LINK += $(sort $(wildcard ../../src/pl/plperl/libperl*.a))
+else
+rpathdir = $(perl_archlibexp)/CORE
+SHLIB_LINK += $(perl_embed_ldflags)
+endif
+
+# As with plperl we need to make sure that the CORE directory is included
+# last, probably because it sometimes contains some header files with names
+# that clash with some of ours, or with some that we include, notably on
+# Windows.
+override CPPFLAGS := $(CPPFLAGS) $(perl_embed_ccflags) -I$(perl_archlibexp)/CORE
diff --git a/contrib/jsonb_plperl/expected/jsonb_plperl.out b/contrib/jsonb_plperl/expected/jsonb_plperl.out
new file mode 100644
index 000..152e62d
--- /dev/null
+++ b/contrib/jsonb_plperl/expected/jsonb_plperl.out
@@ -0,0 +1,243 @@
+CREATE EXTENSION jsonb_plperl CASCADE;
+NOTICE:  installing required extension "plperl"
+-- test hash -> jsonb
+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)
+
+-- test array -> jsonb
+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)
+
+-- test scalar -> jsonb
+CREATE FUN

Re: Transform for pl/perl

[Pavel Stehule]

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

Re: Transform for pl/perl

[Anthony Bykov]

On Wed, 31 Jan 2018 13:36:22 +0300
Arthur Zakirov  wrote:

> I've noticed a possible bug:
> 
> > +   /* json key in v */
> > +   key =
> > pstrdup(v.val.string.val);
> > +   keyLength =
> > v.val.string.len;
> > +   JsonbIteratorNext(, ,
> > 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 Companydiff --git a/contrib/Makefile b/contrib/Makefile
index 8046ca4..53d44fe 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -75,9 +75,9 @@ ALWAYS_SUBDIRS += sepgsql
 endif
 
 ifeq ($(with_perl),yes)
-SUBDIRS += hstore_plperl
+SUBDIRS += hstore_plperl jsonb_plperl
 else
-ALWAYS_SUBDIRS += hstore_plperl
+ALWAYS_SUBDIRS += hstore_plperl jsonb_plperl
 endif
 
 ifeq ($(with_python),yes)
diff --git a/contrib/jsonb_plperl/Makefile b/contrib/jsonb_plperl/Makefile
new file mode 100644
index 000..cd86553
--- /dev/null
+++ b/contrib/jsonb_plperl/Makefile
@@ -0,0 +1,40 @@
+# contrib/jsonb_plperl/Makefile
+
+MODULE_big = jsonb_plperl
+OBJS = jsonb_plperl.o $(WIN32RES)
+PGFILEDESC = "jsonb_plperl - jsonb transform for plperl"
+
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl
+
+EXTENSION = jsonb_plperlu jsonb_plperl
+DATA = jsonb_plperlu--1.0.sql jsonb_plperl--1.0.sql
+
+REGRESS = jsonb_plperl jsonb_plperl_relocatability jsonb_plperlu jsonb_plperlu_relocatability
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/jsonb_plperl
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+# We must link libperl explicitly
+ifeq ($(PORTNAME), win32)
+# these settings are the same as for plperl
+override CPPFLAGS += -DPLPERL_HAVE_UID_GID -Wno-comment
+# ... see silliness in plperl Makefile ...
+SHLIB_LINK += $(sort $(wildcard ../../src/pl/plperl/libperl*.a))
+else
+rpathdir = $(perl_archlibexp)/CORE
+SHLIB_LINK += $(perl_embed_ldflags)
+endif
+
+# As with plperl we need to make sure that the CORE directory is included
+# last, probably because it sometimes contains some header files with names
+# that clash with some of ours, or with some that we include, notably on
+# Windows.
+override CPPFLAGS := $(CPPFLAGS) $(perl_embed_ccflags) -I$(perl_archlibexp)/CORE
diff --git a/contrib/jsonb_plperl/expected/jsonb_plperl.out b/contrib/jsonb_plperl/expected/jsonb_plperl.out
new file mode 100644
index 000..152e62d
--- /dev/null
+++ b/contrib/jsonb_plperl/expected/jsonb_plperl.out
@@ -0,0 +1,243 @@
+CREATE EXTENSION jsonb_plperl CASCADE;
+NOTICE:  installing required extension "plperl"
+-- test hash -> jsonb
+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)
+
+-- test array -> jsonb
+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)
+
+-- test scalar -> jsonb
+CREATE FUNCTION testSVToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = 1;
+return $val;
+$$;
+SELECT testSVToJsonb();
+ testsvtojsonb 
+---
+ 1
+(1 row)
+
+-- test blessed scalar -> jsonb
+CREATE FUNCTION testBlessedToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+my $class = shift;
+my $tmp = { a=>"a", 1=>"1" };

Re: Transform for pl/perl

[Arthur Zakirov]

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(, , 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

Re: Transform for pl/perl

[Thomas Munro]

On Fri, Jan 12, 2018 at 9:47 PM, Anthony Bykov  wrote:
> On Fri, 12 Jan 2018 15:19:26 +1300
> Thomas Munro  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

Re: Transform for pl/perl

[Andrew Dunstan]

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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Transform for pl/perl

[Anthony Bykov]

On Fri, 12 Jan 2018 15:19:26 +1300
Thomas Munro  wrote:

> On Thu, Dec 7, 2017 at 10:56 PM, Anthony Bykov
>  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 +
> --- 
> /home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/results/jsonb_plperl.out
> 2018-01-11 21:55:08.564204175 +
> ***
> *** 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 +
> --- 
> /home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/results/jsonb_plperlu.out
> 2018-01-11 21:55:08.704204228 +
> ***
> *** 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 Companydiff --git a/contrib/Makefile b/contrib/Makefile
index 8046ca4..53d44fe 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -75,9 +75,9 @@ ALWAYS_SUBDIRS += sepgsql
 endif
 
 ifeq ($(with_perl),yes)
-SUBDIRS += hstore_plperl
+SUBDIRS += hstore_plperl jsonb_plperl
 else
-ALWAYS_SUBDIRS += hstore_plperl
+ALWAYS_SUBDIRS += hstore_plperl jsonb_plperl
 endif
 
 ifeq ($(with_python),yes)
diff --git a/contrib/jsonb_plperl/Makefile b/contrib/jsonb_plperl/Makefile
new file mode 100644
index 000..cd86553
--- /dev/null
+++ b/contrib/jsonb_plperl/Makefile
@@ -0,0 +1,40 @@
+# contrib/jsonb_plperl/Makefile
+
+MODULE_big = jsonb_plperl
+OBJS = jsonb_plperl.o $(WIN32RES)
+PGFILEDESC = "jsonb_plperl - jsonb transform for plperl"
+
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl
+
+EXTENSION = jsonb_plperlu jsonb_plperl
+DATA = jsonb_plperlu--1.0.sql jsonb_plperl--1.0.sql
+
+REGRESS = jsonb_plperl jsonb_plperl_relocatability jsonb_plperlu jsonb_plperlu_relocatability
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/jsonb_plperl
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+# We must link libperl explicitly
+ifeq ($(PORTNAME), win32)
+# these settings are the same as for plperl
+override CPPFLAGS += -DPLPERL_HAVE_UID_GID -Wno-comment
+# ... see silliness in plperl Makefile ...
+SHLIB_LINK += $(sort $(wildcard ../../src/pl/plperl/libperl*.a))
+else
+rpathdir = $(perl_archlibexp)/CORE
+SHLIB_LINK += $(perl_embed_ldflags)
+endif
+
+# As with plperl we need to make sure that the CORE directory is included
+# last, probably because it sometimes contains some header files with names
+# that clash with some of ours, or with some that we include, notably on
+# Windows.
+override CPPFLAGS := $(CPPFLAGS) $(perl_embed_ccflags) -I$(perl_archlibexp)/CORE
diff --git a/contrib/jsonb_plperl/expected/jsonb_plperl.out b/contrib/jsonb_plperl/expected/jsonb_plperl.out
new file mode 100644
index 000..9032f7e
--- /dev/null
+++ b/contrib/jsonb_plperl/expected/jsonb_plperl.out
@@ -0,0 +1,235 @@
+CREATE EXTENSION jsonb_plperl CASCADE;
+NOTICE:  installing 

Re: Transform for pl/perl

[Thomas Munro]

On Thu, Dec 7, 2017 at 10:56 PM, Anthony Bykov  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 +
--- 
/home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/results/jsonb_plperl.out
2018-01-11 21:55:08.564204175 +
***
*** 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 +
--- 
/home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/results/jsonb_plperlu.out
2018-01-11 21:55:08.704204228 +
***
*** 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

Re: Transform for pl/perl

[Peter Eisentraut]

On 12/1/17 13:15, Tom Lane wrote:
> Robert Haas  writes:
>> On Fri, Dec 1, 2017 at 12:30 AM, Michael Paquier
>>  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

Re: Transform for pl/perl

[Anthony Bykov]

On Thu, 7 Dec 2017 12:54:55 +0300
Anthony Bykov  wrote:

> On Fri, 1 Dec 2017 15:49:21 -0300
> Alvaro Herrera  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 Companydiff --git a/contrib/Makefile b/contrib/Makefile
index 8046ca4..53d44fe 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -75,9 +75,9 @@ ALWAYS_SUBDIRS += sepgsql
 endif
 
 ifeq ($(with_perl),yes)
-SUBDIRS += hstore_plperl
+SUBDIRS += hstore_plperl jsonb_plperl
 else
-ALWAYS_SUBDIRS += hstore_plperl
+ALWAYS_SUBDIRS += hstore_plperl jsonb_plperl
 endif
 
 ifeq ($(with_python),yes)
diff --git a/contrib/jsonb_plperl/Makefile b/contrib/jsonb_plperl/Makefile
new file mode 100644
index 000..cd86553
--- /dev/null
+++ b/contrib/jsonb_plperl/Makefile
@@ -0,0 +1,40 @@
+# contrib/jsonb_plperl/Makefile
+
+MODULE_big = jsonb_plperl
+OBJS = jsonb_plperl.o $(WIN32RES)
+PGFILEDESC = "jsonb_plperl - jsonb transform for plperl"
+
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl
+
+EXTENSION = jsonb_plperlu jsonb_plperl
+DATA = jsonb_plperlu--1.0.sql jsonb_plperl--1.0.sql
+
+REGRESS = jsonb_plperl jsonb_plperl_relocatability jsonb_plperlu jsonb_plperlu_relocatability
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/jsonb_plperl
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+# We must link libperl explicitly
+ifeq ($(PORTNAME), win32)
+# these settings are the same as for plperl
+override CPPFLAGS += -DPLPERL_HAVE_UID_GID -Wno-comment
+# ... see silliness in plperl Makefile ...
+SHLIB_LINK += $(sort $(wildcard ../../src/pl/plperl/libperl*.a))
+else
+rpathdir = $(perl_archlibexp)/CORE
+SHLIB_LINK += $(perl_embed_ldflags)
+endif
+
+# As with plperl we need to make sure that the CORE directory is included
+# last, probably because it sometimes contains some header files with names
+# that clash with some of ours, or with some that we include, notably on
+# Windows.
+override CPPFLAGS := $(CPPFLAGS) $(perl_embed_ccflags) -I$(perl_archlibexp)/CORE
diff --git a/contrib/jsonb_plperl/expected/jsonb_plperl.out b/contrib/jsonb_plperl/expected/jsonb_plperl.out
new file mode 100644
index 000..9032f7e
--- /dev/null
+++ b/contrib/jsonb_plperl/expected/jsonb_plperl.out
@@ -0,0 +1,235 @@
+CREATE EXTENSION jsonb_plperl CASCADE;
+NOTICE:  installing required 

Re: Transform for pl/perl

[Anthony Bykov]

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

Re: Transform for pl/perl

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Transform for pl/perl

[Tom Lane]

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

Re: Transform for pl/perl

[Robert Haas]

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

Re: Transform for pl/perl

[Michael Paquier]

On Tue, Nov 28, 2017 at 5:14 PM, Aleksander Alekseev
 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

Re: Transform for pl/perl

[Anthony Bykov]

On Wed, 15 Nov 2017 08:58:54 +
Aleksander Alekseev  wrote:

> The following review has been posted through the commitfest
> application: make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   tested, passed
> Documentation:tested, passed
> 
> 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 Companydiff --git a/contrib/Makefile b/contrib/Makefile
index 8046ca4..53d44fe 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -75,9 +75,9 @@ ALWAYS_SUBDIRS += sepgsql
 endif
 
 ifeq ($(with_perl),yes)
-SUBDIRS += hstore_plperl
+SUBDIRS += hstore_plperl jsonb_plperl
 else
-ALWAYS_SUBDIRS += hstore_plperl
+ALWAYS_SUBDIRS += hstore_plperl jsonb_plperl
 endif
 
 ifeq ($(with_python),yes)
diff --git a/contrib/jsonb_plperl/Makefile b/contrib/jsonb_plperl/Makefile
new file mode 100644
index 000..cd86553
--- /dev/null
+++ b/contrib/jsonb_plperl/Makefile
@@ -0,0 +1,40 @@
+# contrib/jsonb_plperl/Makefile
+
+MODULE_big = jsonb_plperl
+OBJS = jsonb_plperl.o $(WIN32RES)
+PGFILEDESC = "jsonb_plperl - jsonb transform for plperl"
+
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl
+
+EXTENSION = jsonb_plperlu jsonb_plperl
+DATA = jsonb_plperlu--1.0.sql jsonb_plperl--1.0.sql
+
+REGRESS = jsonb_plperl jsonb_plperl_relocatability jsonb_plperlu jsonb_plperlu_relocatability
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/jsonb_plperl
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+# We must link libperl explicitly
+ifeq ($(PORTNAME), win32)
+# these settings are the same as for plperl
+override CPPFLAGS += -DPLPERL_HAVE_UID_GID -Wno-comment
+# ... see silliness in plperl Makefile ...
+SHLIB_LINK += $(sort $(wildcard ../../src/pl/plperl/libperl*.a))
+else
+rpathdir = $(perl_archlibexp)/CORE
+SHLIB_LINK += $(perl_embed_ldflags)
+endif
+
+# As with plperl we need to make sure that the CORE directory is included
+# last, probably because it sometimes contains some header files with names
+# that clash with some of ours, or with some that we include, notably on
+# Windows.
+override CPPFLAGS := $(CPPFLAGS) $(perl_embed_ccflags) -I$(perl_archlibexp)/CORE
diff --git a/contrib/jsonb_plperl/expected/jsonb_plperl.out b/contrib/jsonb_plperl/expected/jsonb_plperl.out
new file mode 100644
index 000..b5c0980
--- /dev/null
+++ b/contrib/jsonb_plperl/expected/jsonb_plperl.out
@@ -0,0 +1,239 @@
+CREATE EXTENSION jsonb_plperl CASCADE;
+NOTICE:  installing required extension "plperl"
+-- test hash -> jsonb
+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)
+
+-- test array -> jsonb
+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)
+
+-- test scalar -> jsonb
+CREATE FUNCTION testSVToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = 1;
+return $val;
+$$;
+SELECT testSVToJsonb();
+ testsvtojsonb 
+---
+ 1
+(1 row)
+
+-- test blessed scalar -> jsonb
+CREATE FUNCTION testBlessedToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+my $class = shift;
+my $tmp = { a=>"a", 1=>"1" };
+bless $tmp, $class;
+return $tmp;
+$$;
+SELECT testBlessedToJsonb();
+  testblessedtojsonb  
+--
+ {"1": "1", "a": "a"}
+(1 row)
+
+-- test blessed scalar -> jsonb
+CREATE FUNCTION 

Re: [HACKERS] 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
Description: PGP signature

Re: [HACKERS] Transform for pl/perl

[Pavel Stehule]

2017-11-15 10:24 GMT+01:00 Oleg Bartunov :

>
>
> On 14 Nov 2017 11:35, "Anthony Bykov"  wrote:
>
> On Fri, 10 Nov 2017 14:40:21 +0100
> Pavel Stehule  wrote:
>
> > Hi
> >
> > 2017-10-24 14:27 GMT+02:00 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);
> > >
> > >
> > 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-hack...@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 

Re: [HACKERS] Transform for pl/perl

[Oleg Bartunov]

On 14 Nov 2017 11:35, "Anthony Bykov"  wrote:

On Fri, 10 Nov 2017 14:40:21 +0100
Pavel Stehule  wrote:

> Hi
>
> 2017-10-24 14:27 GMT+02:00 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);
> >
> >
> 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-hack...@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

Re: [HACKERS] 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
>
diff --git a/contrib/Makefile b/contrib/Makefile
index 8046ca4f39..53d44fe4a6 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -75,9 +75,9 @@ ALWAYS_SUBDIRS += sepgsql
 endif
 
 ifeq ($(with_perl),yes)
-SUBDIRS += hstore_plperl
+SUBDIRS += hstore_plperl jsonb_plperl
 else
-ALWAYS_SUBDIRS += hstore_plperl
+ALWAYS_SUBDIRS += hstore_plperl jsonb_plperl
 endif
 
 ifeq ($(with_python),yes)
diff --git a/contrib/jsonb_plperl/Makefile b/contrib/jsonb_plperl/Makefile
new file mode 100644
index 00..8c427c545a
--- /dev/null
+++ b/contrib/jsonb_plperl/Makefile
@@ -0,0 +1,40 @@
+# contrib/jsonb_plperl/Makefile
+
+MODULE_big = jsonb_plperl
+OBJS = jsonb_plperl.o $(WIN32RES)
+PGFILEDESC = "jsonb_plperl - jsonb transform for plperl"
+
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl
+
+EXTENSION = jsonb_plperlu jsonb_plperl
+DATA = jsonb_plperlu--1.0.sql jsonb_plperl--1.0.sql
+
+REGRESS = jsonb_plperl jsonb_plperlu
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/jsonb_plperl
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+# We must link libperl explicitly
+ifeq ($(PORTNAME), win32)
+# these settings are the same as for plperl
+override CPPFLAGS += -DPLPERL_HAVE_UID_GID -Wno-comment
+# ... see silliness in plperl Makefile ...
+SHLIB_LINK += $(sort $(wildcard ../../src/pl/plperl/libperl*.a))
+else
+rpathdir = $(perl_archlibexp)/CORE
+SHLIB_LINK += $(perl_embed_ldflags)
+endif
+
+# As with plperl we need to make sure that the CORE directory is included
+# last, probably because it sometimes contains some header files with names
+# that clash with some of ours, or with some that we include, notably on
+# Windows.
+override CPPFLAGS := $(CPPFLAGS) $(perl_embed_ccflags) -I$(perl_archlibexp)/CORE
diff --git a/contrib/jsonb_plperl/expected/jsonb_plperl.out b/contrib/jsonb_plperl/expected/jsonb_plperl.out
new file mode 100644
index 00..fdaa4d013b
--- /dev/null
+++ b/contrib/jsonb_plperl/expected/jsonb_plperl.out
@@ -0,0 +1,197 @@
+CREATE EXTENSION jsonb_plperl CASCADE;
+NOTICE:  installing required extension "plperl"
+-- test hash -> jsonb
+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)
+
+-- test array -> jsonb
+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)
+
+-- test scalar -> jsonb
+CREATE FUNCTION testSVToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = 1;
+return $val;
+$$;
+SELECT testSVToJsonb();
+ testsvtojsonb 
+---
+ 1
+(1 row)
+
+-- test jsonb -> scalar -> jsonb
+CREATE FUNCTION testSVToJsonb2(val jsonb) RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+return $_[0];
+$$;
+SELECT testSVToJsonb2('null');
+ testsvtojsonb2 
+
+ null
+(1 row)
+
+SELECT testSVToJsonb2('1');
+ testsvtojsonb2 
+
+ 1
+(1 row)
+
+SELECT testSVToJsonb2('-1');
+ testsvtojsonb2 
+
+ -1
+(1 row)
+
+SELECT testSVToJsonb2('1.2');
+ testsvtojsonb2 
+
+ 1.2
+(1 row)
+
+SELECT testSVToJsonb2('-1.2');
+ testsvtojsonb2 
+
+ -1.2
+(1 row)
+
+SELECT testSVToJsonb2('"string"');
+ testsvtojsonb2 
+
+ "string"
+(1 row)
+
+-- type information is lost inside
+SELECT testSVToJsonb2('true');
+ testsvtojsonb2 
+
+ 1
+(1 row)
+
+SELECT testSVToJsonb2('false');
+ testsvtojsonb2 
+
+ 0
+(1 row)
+
+SELECT testSVToJsonb2('[]');
+ testsvtojsonb2 
+
+ []
+(1 row)
+
+SELECT testSVToJsonb2('[null,null]');
+ testsvtojsonb2 
+
+ [null, null]
+(1 row)
+
+SELECT testSVToJsonb2('[1,2,3]');
+ testsvtojsonb2 
+
+ [1, 2, 3]
+(1 row)
+
+SELECT testSVToJsonb2('[-1,2,-3]');
+ 

Re: [HACKERS] Transform for pl/perl

[Anthony Bykov]

On Fri, 10 Nov 2017 14:40:21 +0100
Pavel Stehule  wrote:

> Hi
> 
> 2017-10-24 14:27 GMT+02:00 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);
> >
> >  
> 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-hack...@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
diff --git a/contrib/Makefile b/contrib/Makefile
index 8046ca4..53d44fe 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -75,9 +75,9 @@ ALWAYS_SUBDIRS += sepgsql
 endif
 
 ifeq ($(with_perl),yes)
-SUBDIRS += hstore_plperl
+SUBDIRS += hstore_plperl jsonb_plperl
 else
-ALWAYS_SUBDIRS += hstore_plperl
+ALWAYS_SUBDIRS += hstore_plperl jsonb_plperl
 endif
 
 ifeq