From: Tom Lane <tgl@sss.pgh.pa.us>
To: ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=)
cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
        Alvaro Herrera <alvherre@2ndquadrant.com>,
        Pavel Stehule <pavel.stehule@gmail.com>,
        Nikita Glukhov <n.gluhov@postgrespro.ru>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Transform for pl/perl
In-reply-to: <d8jlgbq66t9.fsf@dalvik.ping.uio.no>
References: <20180606161453.tvp4ltavlcvejmmz@alvherre.pgsql>
 <e5130c85-23da-5d11-ca74-6a050cc5fa85@2ndquadrant.com>
 <d8jlgbq66t9.fsf@dalvik.ping.uio.no>
Comments: In-reply-to ilmari@ilmari.org (Dagfinn Ilmari
 =?utf-8?Q?Manns=C3=A5ker?=)
	message dated "Thu, 07 Jun 2018 14:54:10 +0100"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <28335.1528393969.1@sss.pgh.pa.us>
Date: Thu, 07 Jun 2018 13:52:49 -0400
Message-ID: <28336.1528393969@sss.pgh.pa.us>
Precedence: bulk

ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> The way I understand it, it's only how things are passed around
>> internally.  Nothing is noticeable externally, and so there is no
>> backward compatibility issue.
>> 
>> At least that's how I understand it.  So far this is only a claim by one
>> person.  I haven't seen anything conclusive about whether there is an
>> actual issue.

> It's not just how things are passed internally, but how they are passed
> to pl/perl functions with a jsonb transform: JSON scalar values at the
> top level (strings, numbers, booleans, null) get passed as a reference
> to a reference to the value, e.g. \\42 instead of 42.  The reason the
> current tests don't pick this up is that they don't check the value
> inside the pl/perl functions, only that they roundtrip back to jsonb,
> and the plperl to jsonb transform recursively dereferences references.

Yeah, the reason this is important is that it affects what the plperl
function body sees.

> Another side effect of the recursive dereferencing is that returning
> undef from the perl function returns an SQL NULL while returning a
> reference to undef (\undef) returns a jsonb null.

Hm, I think you're blaming the wrong moving part there.  The way the
transform logic is set up (e.g., in plperl_sv_to_datum()), it's
impossible for a transform function to return a SQL null; the decision by
plperl_sv_to_datum as to whether or not the output will be a SQL null is
final.  (Perhaps that was a mistake, but changing the transform function
API seems like a rather Big Deal.)  So if we think that \undef ought to
produce a SQL null, the thing to do is move the dereferencing loop to
the beginning of plperl_sv_to_datum, and then \undef would produce NULL
whether this transform is installed or not.  I don't have a well-informed
opinion on whether that's a good idea, but I tend to the view that it is.
Right now the case produces an error, and not even a very sane one:

regression=# create function foo() returns int language plperlu 
regression-# as '\undef';
CREATE FUNCTION
regression=# select foo();
ERROR:  PL/Perl function must return reference to hash or array
CONTEXT:  PL/Perl function "foo"

so there's not really a compatibility break if we redefine it.

			regards, tom lane