Re: Transform for pl/perl

Hi.

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

Here is my changes:

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

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

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

  * passed JsonbParseState ** to XX_ToJsonbValue() functions.
  
  * fixed warnings (see below)

  * fixed comments (see below)


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

On 12.03.2018 18:06, Pavel Stehule wrote:

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