Thread: 2 line patch to allow plpythonu functions to return void ...

Shamelessly cloned from the parallel code in pltcl, an exception for
void in denying pseudotypes being returned. Pl/tcl didn't reference
VOIDOID anywhere else, so ... .

Allowed following trivial test function to succeed:

create or replace function set_gd(int) returns void as
$$
    GD['global_count'] =  args[0]
$$  language plpythonu volatile;





----
James Robinson
Socialserve.com

James Robinson <jlrobins@socialserve.com> writes:
> Shamelessly cloned from the parallel code in pltcl, an exception for
> void in denying pseudotypes being returned. Pl/tcl didn't reference
> VOIDOID anywhere else, so ... .

This sort of thing normally requires more thought than just removing
the safety check.  What happens when the python code does/doesn't return
a value, in both cases (declared return type void or not)?

            regards, tom lane

Tom,

python functions are specified to return "None", if no return is given. I recommend to also see a plpython function as a Python function, and return None if no return is specified.



--
GHUM Harald Massa
persuadere et programmare
Harald Armin Massa
Reinsburgstraße 202b
70197 Stuttgart
0173/9409607
-
When I visit a mosque, I show my respect by taking off my shoes. I follow the customs, just as I do in a church, synagogue or other holy place. But if a believer demands that I, as a nonbeliever, observe his taboos in the public domain, he is not asking for my respect, but for my submission. And that is incompatible with a secular democracy.

On Feb 25, 2006, at 12:10 PM, Tom Lane wrote:

> James Robinson <jlrobins@socialserve.com> writes:
>> Shamelessly cloned from the parallel code in pltcl, an exception for
>> void in denying pseudotypes being returned. Pl/tcl didn't reference
>> VOIDOID anywhere else, so ... .
>
> This sort of thing normally requires more thought than just removing
> the safety check.  What happens when the python code does/doesn't
> return
> a value, in both cases (declared return type void or not)?
>
>             regards, tom lane

Yes of course.

Here's some permutations of declared void functions explictly
returning a value or not, with the closest thing to void in Python
being None [ which is currently mapped to SQL NULL ] ...

create or replace function void_ret_notvoid() returns void as
$$
    return 12
$$  language plpythonu;

-- return value '' comes decorated with oid 2278, which seems to be
VOIDOID. The 12 integer gets discarded. Ugly, yes.

create or replace function void_ret_none() returns void as
$$
    return None
$$  language plpythonu;

-- Once again, returned oid to client is 2278, and likewise for the
subsequent one....

select void_ret_none() is null;

create or replace function void_ret_falloff_none() returns void as
$$
    x = 12 + 4
$$  language plpythonu;


-- This one returns oid 23, with value NULL.

create or replace function notvoid_ret_none() returns int as
$$
    return None
$$  language plpythonu;


Now, the python language semantics are that if a function does not
explictly return a value, None is implictly returned:

jlrobins:~ jlrobins$ python
Python 2.4.1 (#2, Mar 31 2005, 00:05:10)
[GCC 3.3 20030304 (Apple Computer, Inc. build 1666)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
 >>> def falloff():
...     x = 12
...
 >>> val = falloff()
 >>> val is None
True

So there's a bit of a mismatch between Python and SQL semantics since
Python's None is already being used to represent NULL [ of whatever
datatype the pl function was described to return at the SQL level ]
in plpython.

The ugliest case above is certainly the first one -- function
declared to be void explicitly returning a not-None value. That
should probably cause an SQL-level error. Can't really test it a
function compile time, since Python variables are not typed, only
what they reference are.

The intent was to have to keep from having to declare a bogus return
type for a a procedure that returns no meaningful results at all --
such as one which does nothing but inserts or updates or what have you.

I suspect that all of the above functions returned VOIDOID decorating
the return result due to machinery higher-up than plpython -- the
postgres typing system itself, so those cases are probably silly
examples, other than showing that it doesn't immediately crash. Which
you probably would rather be shown a much higher confidence proof
that the system is still correct aside from not immediately going
down in flames.

I'll go back to lurking and reading -- is the plpgsql source the best
model for reading up on procedure language implementation? Thanks.

----
James Robinson
Socialserve.com

Tom Lane wrote:
> This sort of thing normally requires more thought than just removing
> the safety check.  What happens when the python code does/doesn't return
> a value, in both cases (declared return type void or not)?

Attached is a more complete patch:

- if the function is declared to return void, we only accept "None" as
the return value from Python. Returning anything else produces an error.

- if the function is declared to return void and Python returns "None",
we return this to PG as a void datum (*not* NULL)

- if the function is not declared to return void and Python returns
"None", we return this to PG as a NULL datum

One minor inconsistency is that PL/PgSQL (for example) actually
disallows "RETURN expr;" in void-returning functions: only "RETURN;" can
be used. In PL/Python we don't place any restrictions on the syntax of
the function: "return expr" is allowed in void-returning functions so
long as `expr' evaluates to None. I don't think this is a major problem,
though.

The error message for the first case isn't quite right, and I need to
update the "expected" regression tests and possibly the documentation.
But otherwise I'll apply this patch to HEAD tomorrow, barring any
objections.

-Neil
Index: src/pl/plpython/plpython.c
===================================================================
RCS file: /Users/neilc/postgres/cvs_root/pgsql/src/pl/plpython/plpython.c,v
retrieving revision 1.71
diff -c -r1.71 plpython.c
*** src/pl/plpython/plpython.c    20 Feb 2006 20:10:37 -0000    1.71
--- src/pl/plpython/plpython.c    26 Feb 2006 23:22:27 -0000
***************
*** 91,97 ****
   */
  typedef struct PLyObToDatum
  {
!     FmgrInfo    typfunc;
      Oid            typioparam;
      bool        typbyval;
  }    PLyObToDatum;
--- 91,98 ----
   */
  typedef struct PLyObToDatum
  {
!     FmgrInfo    typfunc;        /* The type's input function */
!     Oid            typoid;            /* The OID of the type */
      Oid            typioparam;
      bool        typbyval;
  }    PLyObToDatum;
***************
*** 138,144 ****
      int            nargs;
      PyObject   *code;            /* compiled procedure code */
      PyObject   *statics;        /* data saved across calls, local scope */
!     PyObject   *globals;        /* data saved across calls, global score */
      PyObject   *me;                /* PyCObject containing pointer to this
                                   * PLyProcedure */
  }    PLyProcedure;
--- 139,145 ----
      int            nargs;
      PyObject   *code;            /* compiled procedure code */
      PyObject   *statics;        /* data saved across calls, local scope */
!     PyObject   *globals;        /* data saved across calls, global scope */
      PyObject   *me;                /* PyCObject containing pointer to this
                                   * PLyProcedure */
  }    PLyProcedure;
***************
*** 757,765 ****
              elog(ERROR, "SPI_finish failed");

          /*
!          * convert the python PyObject to a postgresql Datum
           */
!         if (plrv == Py_None)
          {
              fcinfo->isnull = true;
              rv = PointerGetDatum(NULL);
--- 758,781 ----
              elog(ERROR, "SPI_finish failed");

          /*
!          * If the function is declared to return void, the Python
!          * return value must be None. For void-returning functions, we
!          * also treat a None return value as a special "void datum"
!          * rather than NULL (as is the case for non-void-returning
!          * functions).
           */
!         if (proc->result.out.d.typoid == VOIDOID)
!         {
!             if (plrv != Py_None)
!                 ereport(ERROR,
!                         (errcode(ERRCODE_DATA_EXCEPTION),
!                          errmsg("unexpected return value from plpython procedure"),
!                          errdetail("function returns void")));
!
!             fcinfo->isnull = false;
!             rv = (Datum) 0;
!         }
!         else if (plrv == Py_None)
          {
              fcinfo->isnull = true;
              rv = PointerGetDatum(NULL);
***************
*** 1031,1038 ****
                       procStruct->prorettype);
              rvTypeStruct = (Form_pg_type) GETSTRUCT(rvTypeTup);

!             /* Disallow pseudotype result */
!             if (rvTypeStruct->typtype == 'p')
              {
                  if (procStruct->prorettype == TRIGGEROID)
                      ereport(ERROR,
--- 1047,1055 ----
                       procStruct->prorettype);
              rvTypeStruct = (Form_pg_type) GETSTRUCT(rvTypeTup);

!             /* Disallow pseudotype result, except for void */
!             if (rvTypeStruct->typtype == 'p' &&
!                 procStruct->prorettype != VOIDOID)
              {
                  if (procStruct->prorettype == TRIGGEROID)
                      ereport(ERROR,
***************
*** 1329,1334 ****
--- 1346,1352 ----
      Form_pg_type typeStruct = (Form_pg_type) GETSTRUCT(typeTup);

      perm_fmgr_info(typeStruct->typinput, &arg->typfunc);
+     arg->typoid = HeapTupleGetOid(typeTup);
      arg->typioparam = getTypeIOParam(typeTup);
      arg->typbyval = typeStruct->typbyval;
  }
Index: src/pl/plpython/sql/plpython_function.sql
===================================================================
RCS file: /Users/neilc/postgres/cvs_root/pgsql/src/pl/plpython/sql/plpython_function.sql,v
retrieving revision 1.3
diff -c -r1.3 plpython_function.sql
*** src/pl/plpython/sql/plpython_function.sql    10 Jul 2005 04:56:55 -0000    1.3
--- src/pl/plpython/sql/plpython_function.sql    26 Feb 2006 23:27:49 -0000
***************
*** 341,343 ****
--- 341,358 ----
  rv = plpy.execute(plan, u"\\x80", 1)
  return rv[0]["testvalue1"]
  ' LANGUAGE plpythonu;
+
+ -- Tests for functions that return void
+
+ CREATE FUNCTION test_void_func1() RETURNS void AS $$
+ x = 10
+ $$ LANGUAGE plpythonu;
+
+ -- illegal: can't return non-None value in void-returning func
+ CREATE FUNCTION test_void_func2() RETURNS void AS $$
+ return 10
+ $$ LANGUAGE plpythonu;
+
+ CREATE FUNCTION test_return_none() RETURNS int AS $$
+ None
+ $$ LANGUAGE plpythonu;
\ No newline at end of file
Index: src/pl/plpython/sql/plpython_test.sql
===================================================================
RCS file: /Users/neilc/postgres/cvs_root/pgsql/src/pl/plpython/sql/plpython_test.sql,v
retrieving revision 1.1
diff -c -r1.1 plpython_test.sql
*** src/pl/plpython/sql/plpython_test.sql    14 May 2005 17:55:21 -0000    1.1
--- src/pl/plpython/sql/plpython_test.sql    26 Feb 2006 23:29:38 -0000
***************
*** 68,70 ****
--- 68,75 ----
  SELECT newline_lf();
  SELECT newline_cr();
  SELECT newline_crlf();
+
+ -- Tests for functions returning void
+ SELECT test_void_func1(), test_void_func1() IS NULL AS "is null";
+ SELECT test_void_func2(); -- should fail
+ SELECT test_return_none(), test_return_none() IS NULL AS "is null";
\ No newline at end of file

On Sun, 2006-02-26 at 18:40 -0500, Neil Conway wrote:
> Tom Lane wrote:
> > This sort of thing normally requires more thought than just removing
> > the safety check.  What happens when the python code does/doesn't return
> > a value, in both cases (declared return type void or not)?
>
> Attached is a more complete patch: [...]

Applied to HEAD. I'm still not quite satisfied with the error message:

  ereport(ERROR,
          (errcode(ERRCODE_DATATYPE_MISMATCH),
           errmsg("unexpected return value from plpython procedure"),
           errdetail("void-returning functions must return \"None\"")));

Suggestions for something better?

-Neil

Neil Conway <neilc@samurai.com> writes:
> Applied to HEAD. I'm still not quite satisfied with the error message:

>   ereport(ERROR,
>           (errcode(ERRCODE_DATATYPE_MISMATCH),
>            errmsg("unexpected return value from plpython procedure"),
>            errdetail("void-returning functions must return \"None\"")));

> Suggestions for something better?

Yeah, "unexpected" doesn't seem the mot juste here.  Perhaps "invalid"
or "incorrect" (our message style guidelines seem to suggest "invalid"
as a good word in such cases).  Also, say "function" not "procedure"
because that's what it is in SQL terms.

Also, the errdetail doesn't follow the guideline that says detail
messages should be full sentences with proper capitalization and
punctuation.  Perhaps

    Functions returning type "void" must return "None".

            regards, tom lane

On Tue, 2006-02-28 at 15:26 -0500, Tom Lane wrote:
> Yeah, "unexpected" doesn't seem the mot juste here. [...]

All good points -- thanks for the suggestions. I've applied the attached
patch to HEAD.

-Neil