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

On 11/14/2017 10:54 AM, Peter Eisentraut wrote:
> Here is an updated patch.  It's updated for the recent documentation
> format changes.  I added some more documentation as suggested by reviewers.
>
> I also added more tests about how the various privilege commands (GRANT,
> GRANT on ALL, default privileges) would work with the new object type.
> I had not looked at that in much detail with the previous version of the
> patch, but it seems to work the way I would have wanted without any code
> changes, so it's all documentation additions and new tests.
>
> As part of this I have also developed additional tests for how the same
> privilege commands apply to aggregates, which didn't appear to be
> covered yet, and I was worried that I might have broken it, which it
> seems I did not.  This is included in the big patch, but I have also
> included it here as a separate patch, as it could be committed
> independently as additional tests for existing functionality.
>
> It this point, this patch has no more open TODOs or
> need-to-think-about-this-later's that I'm aware of.
>



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.

While returning multiple result sets will be a useful feature, it's also
very common in my experience for stored procedures to have scalar out
params as well. I'm not sure how we should go about providing for it,
but I think we need to be sure we're not closing any doors.

Here, for example, is how the MySQL stored procedure feature works with
JDBC:
<https://dev.mysql.com/doc/connector-j/5.1/en/connector-j-usagenotes-statements-callable.html>
I think it will be OK if we use cursors to return multiple result sets,
along the lines of Peter's next patch, but we shouldn't regard that as
the end of the story. Even if we don't provide for it in this go round
we should aim at eventually providing for stored procedure OUT params.


Apart from that here are a few fairly minor nitpicks:

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".

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.

In create_function.sgml, we have:
       If a schema name is included, then the function is created in the       specified schema.  Otherwise it is
createdin the current schema.   -   The name of the new function must not match any existing function   +   The name of
thenew function must not match any existing   function or procedure       with the same input argument types in the
sameschema.  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.

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.

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.

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.

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.

The context line here looks odd:
   CREATE PROCEDURE test_proc2()   LANGUAGE plpythonu   AS $$   return 5   $$;   CALL test_proc2();   ERROR:  PL/Python
proceduredid not return None   CONTEXT:  PL/Python function "test_proc2"
 

Perhaps we need to change plpython_error_callback() so that "function"
isn't hardcoded.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services