Re: Transform for pl/perl - Mailing list pgsql-hackers

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>:

On Mon, 5 Mar 2018 14:03:37 +0100
Pavel Stehule <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.