From: | Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> |
---|---|
To: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Transform for pl/perl |
Date: | 2018-03-13 14:50:11 |
Message-ID: | 505345c5-0159-077d-f64e-60afb01e5fbe@postgrespro.ru |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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(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
Attachment | Content-Type | Size |
---|---|---|
0001-jsonb_plperl-extension-v9.patch | text/x-patch | 31.8 KB |
0001-jsonb_plperl-extension-v8-v9-delta.patch | text/x-patch | 11.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2018-03-13 15:00:20 | Re: PATCH: Configurable file mode mask |
Previous Message | David Steele | 2018-03-13 14:45:42 | Re: PATCH: Configurable file mode mask |