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(dot)bykov(at)postgrespro(dot)ru
> <mailto:a(dot)bykov(at)postgrespro(dot)ru>>:
>
> On Mon, 5 Mar 2018 14:03:37 +0100
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com
> <mailto:pavel(dot)stehule(at)gmail(dot)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