From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
cc: ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=),
        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: <28336.1528393969@sss.pgh.pa.us>
References: <20180606161453.tvp4ltavlcvejmmz@alvherre.pgsql>
 <e5130c85-23da-5d11-ca74-6a050cc5fa85@2ndquadrant.com>
 <d8jlgbq66t9.fsf@dalvik.ping.uio.no> <28336.1528393969@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Thu, 07 Jun 2018 13:52:49 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <27212.1528475947.1@sss.pgh.pa.us>
Date: Fri, 08 Jun 2018 12:39:07 -0400
Message-ID: <27213.1528475947@sss.pgh.pa.us>
Precedence: bulk

I wrote:
> ... 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.

After further thought, the only argument I can think of for preserving
this existing behavior is if we wanted to reserve returning a reference-
to-scalar for some future purpose, ie make it do something different
from returning the referenced value.  I can't think of any likely use
of that kind, but maybe I'm just insufficiently creative today.

However, if one makes that argument, then it is clearly a bad idea for
jsonb_plperl to be forcibly dereferencing such references: once we do make
a change of that sort, jsonb_plperl will be out of step with the behavior
for every other datatype, or else we will need to make a subtle
compatibility break to align it with whatever the new behavior is.

So it seems that whichever way you stand on that, it's wrong to have
that dereference loop in SV_to_JsonbValue().  I'm forced to the
conclusion that that's just a hack to band-aid over a bug in the
transform's other direction.

Now, if we did decide that auto-dereferencing should be the general
rule in perl->SQL conversions, I'd be inclined to leave that loop
in place in SV_to_JsonbValue(), because it would be covering the case
where jsonb_plperl is recursively disassembling an AV or HV and finds
a reference-to-scalar.  But we also need a dereference loop in at least
one place in plperl.c itself if that's the plan.

I'm inclined to think that auto-dereference is indeed a good idea,
and am tempted to go make that change to make all this consistent.
Comments?

			regards, tom lane