Re: Transform for pl/perl

Hi

2018-03-13 15:50 GMT+01:00 Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)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(dot)bykov(at)postgrespro(dot)ru>:
>
>> On Mon, 5 Mar 2018 14:03:37 +0100
>> Pavel Stehule <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.
>

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
>