Re: poll: CHECK TRIGGER?

On Wed, Mar 7, 2012 at 12:17 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Robert, please, can you comment to this issue? And other, please. I am
> able to fix syntax to any form where we will have agreement.

Well, so far I don't see that anyone has offered a compelling reason
why this needs core syntax support. If we just define a function
called plpgsql_checker() or plpgsql_lint() or whatever, someone can
check one function like this:

SELECT plpgsql_checker('myfunc(int)'::regprocedure);

If they want to check all the functions in a schema, they can do this:

SELECT plpgsql_checker(oid) FROM pg_proc WHERE prolang = (SELECT oid
FROM pg_language WHERE lanname = 'plpgsql') AND pronamespace = (SELECT
oid FROM pg_namespace WHERE nspname = 'myschema');

If they want to check all functions they own, they can do this:

SELECT plpgsql_checker(oid) FROM pg_proc WHERE prolang = (SELECT oid
FROM pg_language WHERE lanname = 'plpgsql') AND pronamespace = (SELECT
oid FROM pg_authid WHERE rolname = 'myrole');

Any other combination of things that they want to check can be
accommodated by varying the WHERE clause. If we think there are
common cases like the above that we want to make easy, we can do that
by creating wrapper functions called, e.g.
plpgsql_check_namespace(text) and plpgsql_check_role(text) that are
just SQL functions that execute the query mentioned above. If we find
that the convenience functions we've added don't cover all the cases
we care about, it's a lot easier and less invasive to just add a few
more than it is to invent new syntax. Moreover, it doesn't require a
major release cycle: using functional notation, a customer who wants
to check all security definer functions owned by roles that are
members of the dev group can do it just by adjusting the queries shown
above. If they want an extra convenience function for that, they can
easily define one. Even with dedicated syntax it's still possible to
define convenience functions by wrapping the CHECK FUNCTION statement
up in a user-defined function that dynamically generates and executes
SQL and then calling it repeatedly from a query, but that's more work.

As things stand today, the "checker" infrastructure is a very large
percentage of this patch. Ripping all that out and just exposing this
as a function, or a couple of functions, will make the patch much
smaller and simpler, and allow us to focus on what I think we really
should be worrying about here, which is the quality and value of the
tests being performed. I don't necessarily say that it could *never*
make sense to add any syntax support here, but I do think there's no
real clear need for it and that, inasmuch as this is a new feature, it
doesn't really make sense for us to commit ourselves to more than
necessary. tsearch lived without core support for several releases
until it became clear that it was a sufficiently important feature to
merit tighter integration. Furthermore, the functional syntax being
proposed here is exactly the same kind of syntax that we use for many
other things that are arguably far more important. If
pg_start_backup() isn't important enough to be implemented as an SQL
command rather than a function, then I don't think this is either.

Just to be clear, I am not proposing that we get rid of CHECK TRIGGER
and keep CHECK FUNCTION. I'm proposing that we get rid of all of the
dedicated syntax support, and expose it all through one or more
SQL-callable functions. If we need both
plpgsql_check_function(procoid) and plpgsql_check_trigger(tgoid), no
problem.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company