Re: [HACKERS] SQL procedures - Mailing list pgsql-hackers

On 11/20/17 16:25, Andrew Dunstan wrote:
> I've been through this fairly closely, and I think it's pretty much
> committable. The only big item that stands out for me is the issue of
> OUT parameters.

I figured that that's something that would come up.  I had intentionally
prohibited OUT parameters for now so that we can come up with something
for them without having to break any backward compatibility.

From reading some of the references so far, I think it could be
sufficient to return a one-row result set and have the drivers adapt the
returned data into their interfaces.  The only thing that would be a bit
weird about that is that a CALL command with OUT parameters would return
a result set and a CALL command without any OUT parameters would return
CommandComplete instead.  Maybe that's OK.

> Default Privileges docs: although FUNCTIONS and ROUTINES are equivalent
> here, we should probably advise using ROUTINES, as FUNCTIONS could
> easily be take to mean "functions but not procedures".

OK, I'll update the documentation a bit more.

> CREATE/ALTER PROCEDURE: It seems more than a little odd to allow
> attributes that are irrelevant to procedures in these statements. The
> docco states "for compatibility with ALTER FUNCTION" but why do we want
> such compatibility if it's meaningless? If we can manage it without too
> much violence I'd prefer to see an exception raised if these are used.

We can easily be more restrictive here.  I'm open to suggestions.  There
is a difference between options that don't make sense for procedures
(e.g., RETURNS NULL ON NULL INPUT), which are prohibited, and those that
might make sense sometime, but are currently not used.  But maybe that's
too confusing and we should just prohibit options that are not currently
used.

> In create_function.sgml, we have:
> 
>         If a schema name is included, then the function is created in the
>         specified schema.  Otherwise it is created in the current schema.
>     -   The name of the new function must not match any existing function
>     +   The name of the new function must not match any existing
>     function or procedure
>         with the same input argument types in the same schema.  However,
>         functions of different argument types can share a name (this is
>         called <firstterm>overloading</firstterm>).
> 
> The last sentence should probably say "functions and procedures of
> different argument types" There's a similar issue in create_procedure.sqml.

fixing that

> In grant.sgml, there is:
> 
>     +       The <literal>FUNCTION</literal> syntax also works for aggregate
>     +       functions.  Or use <literal>ROUTINE</literal> to refer to a
>     function,
>     +       aggregate function, or procedure regardless of what it is.
> 
> 
> I would replace "Or" by "Alternatively,". I think it reads better that way.

fixing that

> In functions.c, there is:
> 
>                 /* Should only get here for VOID functions */
>     -           Assert(fcache->rettype == VOIDOID);
>     +           Assert(fcache->rettype == InvalidOid || fcache->rettype
>     == VOIDOID);
>                 fcinfo->isnull = true;
>                 result = (Datum) 0;
> 
> The comment should also refer to procedures.

fixing that

> It took me a minute to work out what is going on with the new code in
> aclchk.c:objectsInSchemaToOids(). It probably merits a comment or two.

improving that

> We should document where returned values in PL procedures are ignored
> (plperl, pltcl) and where they are not (plpython, plpgsql). Maybe we
> should think about possibly being more consistent here, too.

Yeah, suggestions?  I think it makes sense in PL/pgSQL and PL/Python to
disallow return values that would end up being ignored, because the only
way a return value could arise is if user code explicit calls
RETURN/return.  However, in Perl, the return value is the result of the
last statement, so prohibiting a return value would be very
inconvenient.  (Don't know about Tcl.)  So maybe the current behavior
makes sense.  Documentation is surely needed.

> The context line here looks odd:
> 
>     CREATE PROCEDURE test_proc2()
>     LANGUAGE plpythonu
>     AS $$
>     return 5
>     $$;
>     CALL test_proc2();
>     ERROR:  PL/Python procedure did not return None
>     CONTEXT:  PL/Python function "test_proc2"
> 
> Perhaps we need to change plpython_error_callback() so that "function"
> isn't hardcoded.

fixing that

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services