Re: poll: CHECK TRIGGER?

Hi,

Pavel's patch for CHECK FUNCTION is adding another command besides that
one, which is CHECK TRIGGER. The idea behind this is that you give it
the relation to which the trigger is attached in addition to the trigger
name, and it checks the function being called by that trigger.

IMHO having a separate command for this is not warranted. It seems to
me that we could simply have a variant of CREATE FUNCTION for this; I
proposed CREATE FUNCTION trigfunc() AS TRIGGER ON tabname. Besides,
it's really the function code being checked, not the trigger itself; the
trigger is only providing the tuple descriptor for NEW and OLD.

Pavel didn't like this idea; also, a quick poll on twitter elicited only
two answers: Dimitri Fontaine prefers CHECK TRIGGER, and Guillaume
Lelarge prefers CHECK FUNCTION.

So I still don't know which route to go with this. Thoughts?

One thing to consider is eventual support for triggers that use
anonymous function blocks, without a previous CREATE FUNCTION, which is
being discussed in another thread. Another point is that CHECK TRIGGER
requires a separate documentation page.

--
Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> Pavel's patch for CHECK FUNCTION is adding another command besides that
> one, which is CHECK TRIGGER. The idea behind this is that you give it
> the relation to which the trigger is attached in addition to the trigger
> name, and it checks the function being called by that trigger.

> IMHO having a separate command for this is not warranted. It seems to
> me that we could simply have a variant of CREATE FUNCTION for this; I
> proposed CREATE FUNCTION trigfunc() AS TRIGGER ON tabname.

You mean "CHECK FUNCTION ..." right?

In principle the CHECK TRIGGER command could apply more checks than
are possible with the proposed CHECK FUNCTION syntax: in particular,
AFAICS "AS TRIGGER ON tabname" doesn't provide enough info to know
whether the function should expect new and/or old rows to be provided,
nor what it ought to return (which is different for BEFORE/AFTER cases,
STATEMENT cases, etc). We could add all that info to the CHECK FUNCTION
syntax, but there's definitely some merit to defining the check as
occurring against an existing trigger definition instead.

Now, if there's no intention of ever making the code actually apply
checks of the sort I'm thinking of, maybe CHECK FUNCTION AS TRIGGER
is fine.

> One thing to consider is eventual support for triggers that use
> anonymous function blocks, without a previous CREATE FUNCTION, which is
> being discussed in another thread.

Yeah, that angle seems to be another reason to support CHECK TRIGGER.

regards, tom lane

Excerpts from Tom Lane's message of sáb mar 03 23:00:26 -0300 2012:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> > Pavel's patch for CHECK FUNCTION is adding another command besides that
> > one, which is CHECK TRIGGER. The idea behind this is that you give it
> > the relation to which the trigger is attached in addition to the trigger
> > name, and it checks the function being called by that trigger.
>
> > IMHO having a separate command for this is not warranted. It seems to
> > me that we could simply have a variant of CREATE FUNCTION for this; I
> > proposed CREATE FUNCTION trigfunc() AS TRIGGER ON tabname.
>
> You mean "CHECK FUNCTION ..." right?

Yeah, sorry.

> In principle the CHECK TRIGGER command could apply more checks than
> are possible with the proposed CHECK FUNCTION syntax: in particular,
> AFAICS "AS TRIGGER ON tabname" doesn't provide enough info to know
> whether the function should expect new and/or old rows to be provided,
> nor what it ought to return (which is different for BEFORE/AFTER cases,
> STATEMENT cases, etc). We could add all that info to the CHECK FUNCTION
> syntax, but there's definitely some merit to defining the check as
> occurring against an existing trigger definition instead.

Uh! Now that I read this I realize that what you're supposed to give to
CHECK TRIGGER is the trigger name, not the function name! In that
light, using CHECK FUNCTION for this doesn't make a lot of sense.

Okay, CHECK TRIGGER it is.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Sat, Mar 3, 2012 at 9:23 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Uh!  Now that I read this I realize that what you're supposed to give to
> CHECK TRIGGER is the trigger name, not the function name!  In that
> light, using CHECK FUNCTION for this doesn't make a lot of sense.
>
> Okay, CHECK TRIGGER it is.

I confess to some bafflement about why we need dedicated syntax for
this, or even any kind of core support at all. What would be wrong
with defining a function that takes regprocedure as an argument and
does whatever? Sure, it's nicer syntax, but we've repeatedly rejected
patches that only provided nicer syntax on the grounds that syntax is
not free, and therefore syntax alone is not a reason to change the
core grammar. What makes this case different?

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

2012/3/5 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sat, Mar 3, 2012 at 9:23 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>> Uh!  Now that I read this I realize that what you're supposed to give to
>> CHECK TRIGGER is the trigger name, not the function name!  In that
>> light, using CHECK FUNCTION for this doesn't make a lot of sense.
>>
>> Okay, CHECK TRIGGER it is.
>
> I confess to some bafflement about why we need dedicated syntax for
> this, or even any kind of core support at all.  What would be wrong
> with defining a function that takes regprocedure as an argument and
> does whatever?  Sure, it's nicer syntax, but we've repeatedly rejected
> patches that only provided nicer syntax on the grounds that syntax is
> not free, and therefore syntax alone is not a reason to change the
> core grammar.  What makes this case different?

Fo checking trigger handler (trigger function) you have to know
trigger definition (only joined relation now), but it can be enhanced
for other tests based on trigger data.

Regards

Pavel

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I confess to some bafflement about why we need dedicated syntax for
> this, or even any kind of core support at all. What would be wrong
> with defining a function that takes regprocedure as an argument and
> does whatever? Sure, it's nicer syntax, but we've repeatedly rejected
> patches that only provided nicer syntax on the grounds that syntax is
> not free, and therefore syntax alone is not a reason to change the
> core grammar. What makes this case different?

There's definitely something to be said for that, since it entirely
eliminates the problem of providing wildcards and control over which
function(s) to check --- the user could write a SELECT from pg_proc
that slices things however he wants.

The trigger case would presumably take arguments matching pg_trigger's
primary key, viz check_trigger(trig_rel regclass, trigger_name name).

But as for needing "core support", we do need to extend the API for PL
validators, so it's not like this could be done as an external project.

regards, tom lane

On Mon, Mar 5, 2012 at 11:41 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I confess to some bafflement about why we need dedicated syntax for
>> this, or even any kind of core support at all.  What would be wrong
>> with defining a function that takes regprocedure as an argument and
>> does whatever?  Sure, it's nicer syntax, but we've repeatedly rejected
>> patches that only provided nicer syntax on the grounds that syntax is
>> not free, and therefore syntax alone is not a reason to change the
>> core grammar.  What makes this case different?
>
> There's definitely something to be said for that, since it entirely
> eliminates the problem of providing wildcards and control over which
> function(s) to check --- the user could write a SELECT from pg_proc
> that slices things however he wants.
> The trigger case would presumably take arguments matching pg_trigger's
> primary key, viz check_trigger(trig_rel regclass, trigger_name name).

Yes...

> But as for needing "core support", we do need to extend the API for PL
> validators, so it's not like this could be done as an external project.

Well, the plpgsql extension could install a function
pg_check_plpgsql_function() that only works on PL/pgsql functions, and
other procedural languages could do the same at their option. I think
we only need to extend the API if we want to provide a dispatch
function so that you can say "check this function, whatever language
it's written in" and have the right checker get called. But since
we've already talked about the possibility of having more than one
checker per language doing different kinds of checks, I'm not even
sure that "the checker" for a language is a concept that we want to
invent.

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

2012/3/5 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Mar 5, 2012 at 11:41 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> I confess to some bafflement about why we need dedicated syntax for
>>> this, or even any kind of core support at all.  What would be wrong
>>> with defining a function that takes regprocedure as an argument and
>>> does whatever?  Sure, it's nicer syntax, but we've repeatedly rejected
>>> patches that only provided nicer syntax on the grounds that syntax is
>>> not free, and therefore syntax alone is not a reason to change the
>>> core grammar.  What makes this case different?
>>
>> There's definitely something to be said for that, since it entirely
>> eliminates the problem of providing wildcards and control over which
>> function(s) to check --- the user could write a SELECT from pg_proc
>> that slices things however he wants.
>> The trigger case would presumably take arguments matching pg_trigger's
>> primary key, viz check_trigger(trig_rel regclass, trigger_name name).
>
> Yes...
>
>> But as for needing "core support", we do need to extend the API for PL
>> validators, so it's not like this could be done as an external project.
>
> Well, the plpgsql extension could install a function
> pg_check_plpgsql_function() that only works on PL/pgsql functions, and
> other procedural languages could do the same at their option.  I think
> we only need to extend the API if we want to provide a dispatch
> function so that you can say "check this function, whatever language
> it's written in" and have the right checker get called.  But since
> we've already talked about the possibility of having more than one
> checker per language doing different kinds of checks, I'm not even
> sure that "the checker" for a language is a concept that we want to
> invent.

There is not multiple PL checker function - or I don't know about it.

Pavel

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

2012/3/5 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sat, Mar 3, 2012 at 9:23 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>> Uh!  Now that I read this I realize that what you're supposed to give to
>> CHECK TRIGGER is the trigger name, not the function name!  In that
>> light, using CHECK FUNCTION for this doesn't make a lot of sense.
>>
>> Okay, CHECK TRIGGER it is.
>
> I confess to some bafflement about why we need dedicated syntax for
> this, or even any kind of core support at all.  What would be wrong
> with defining a function that takes regprocedure as an argument and
> does whatever?  Sure, it's nicer syntax, but we've repeatedly rejected
> patches that only provided nicer syntax on the grounds that syntax is
> not free, and therefore syntax alone is not a reason to change the
> core grammar.  What makes this case different?
>

before argumentation for CHECK TRIGGER I show a proposed PL checker function:

FUNCTION checker_function(regprocedure, regclass, options text[],
fatal_errors boolen)
RETURNS TABLE (functionoid oid, lineno int, statement text, sqlstate
text, message text, detail text, hint text, position int, query)

this function is worker for CHECK FUNCTION and CHECK TRIGGER
statements. The possibility to call this function directly can enable
thousands combinations - all functions, all functions from schema, all
functions that has name starts with, ...

for user friendly there are interface: CHECK FUNCTION and CHECK TRIGGER

* provides more practical reports with caret positioning than SRF function
* support often used combinations of requests - all functions from one
language, all functions from schema, all functions by one user

CHECK FUNCTION is clear - and there are no disagreement

There are two possibilities for checking triggers

a) some like CHECK FUNCTION trgfunc() ON table_name

b) existing CHECK TRIGGER t1_f1 ON table_name;

these forms are almost equal, although CREATE TRIGGER can provide more
unique information for checking. And joining table_name to TRIGGER has
bigger sense then to FUNCTION (in one statement).

When I try to look on some multicheck form:

a) CHECK FUNCTION ALL ON table_name
b) CHECK TRIGGER ALL ON table_name

then more natural form is @b (for me). Personally, I can live with
one, both or second form, although I prefer CHECK TRIGGER.

notes?

Regards

Pavel

> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

Hello

>
> When I try to look on some multicheck form:
>
> a) CHECK FUNCTION ALL ON table_name
> b) CHECK TRIGGER ALL ON table_name
>
> then more natural form is @b (for me). Personally, I can live with
> one, both or second form, although I prefer CHECK TRIGGER.
>

I though some time more.

if somebody would to check all custom function, then he can write

CHECK FUNCTION ALL

what about triggers?

CHECK TRIGGER ALL

but if we don't implement CHECK TRIGGER, then this statement will look like

CHECK FUNCTION ALL ON ALL ???

and this is unclean - probably it doesn't mean - check trigger
function with any table. So this is other argument for CREATE TRIGGER.

Nice a day

Pavel

> notes?
>
> Regards
>
> Pavel
>
>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers

Hello

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.

Regards

Pavel

2012/3/6 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
>>
>> When I try to look on some multicheck form:
>>
>> a) CHECK FUNCTION ALL ON table_name
>> b) CHECK TRIGGER ALL ON table_name
>>
>> then more natural form is @b (for me). Personally, I can live with
>> one, both or second form, although I prefer CHECK TRIGGER.
>>
>
> I though some time more.
>
> if somebody would to check all custom function, then he can write
>
> CHECK FUNCTION ALL
>
> what about triggers?
>
> CHECK TRIGGER ALL
>
> but if we don't implement CHECK TRIGGER, then this statement will look like
>
> CHECK FUNCTION ALL ON ALL ???
>
> and this is unclean - probably it doesn't mean - check trigger
> function with any table. So this is other argument for CREATE TRIGGER.
>
> Nice a day
>
> Pavel
>
>
>> notes?
>>
>> Regards
>>
>> Pavel
>>
>>
>>> --
>>> Robert Haas
>>> EnterpriseDB: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>> --
>>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>>> To make changes to your subscription:
>>> http://www.postgresql.org/mailpref/pgsql-hackers

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> 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.

This seems entirely reasonable to me. The syntax support is not the
value-add in this patch, and it's been clear since day one that it would
be difficult for the syntax to cover all the likely permutations of
"which functions do you want to check?". A function-based interface
seems like both less work and more functionality. Yes, it's marginally
less convenient for simple cases, but I'm not even sure that we know
which "simple cases" are going to be popular. We can and should
postpone that decision until we have some field experience to base it on.

> If we need both
> plpgsql_check_function(procoid) and plpgsql_check_trigger(tgoid), no
> problem.

FWIW, I would suggest check_trigger(regclass, name) not tgoid, because
we do not have a regtrigger convenience type (and I don't think it's
worth adding one).

More importantly, I do not agree with requiring the user to specify the
language name --- that is, it should be check_function(procoid) and have
that look up a language-specific checker. Otherwise, scenarios like
"check all my functions regardless of language" are too painful.
There is value-added in providing that much infrastructure.

regards, tom lane

2012/3/7 Robert Haas <robertmhaas(at)gmail(dot)com>:
> 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.

this is very near to my original design. This design is general so we
really don't special statements - because any action can be processed
in combination query to pg_catalog and calling checker function.

The most expected value of special statements is simplicity for usage
- checking function is common task - it should be called significantly
more often than pg_start_backup() or some tsearch maintaining
procedures.

just: "check function name()" is more shorter than "select
plpgsql_check_function('name()')" and what is important - it is
supported by autocomplete in psql. Other arguments is possibility to
show result.

A special statement has higher possibility to put output more clean
and readable - I am not able do it in function.

I agree, so this patch is relative long, but almost all code in these
statement is related to multiple checking. When I remove it (or when I
simplify) - I can significantly reduce patch (or this part)

But still I think so some reduced CHECK statements is good idea

mainly for:

* autocomplete support
* more readable output in terminal

I don't know if this is enough

Regards

Pavel

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

On Wed, Mar 7, 2012 at 12:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> If we need both
>> plpgsql_check_function(procoid) and plpgsql_check_trigger(tgoid), no
>> problem.
>
> FWIW, I would suggest check_trigger(regclass, name) not tgoid, because
> we do not have a regtrigger convenience type (and I don't think it's
> worth adding one).

I'm OK with either one.

> More importantly, I do not agree with requiring the user to specify the
> language name --- that is, it should be check_function(procoid) and have
> that look up a language-specific checker.  Otherwise, scenarios like
> "check all my functions regardless of language" are too painful.
> There is value-added in providing that much infrastructure.

I might agree with you if we had more than one checker function, but
right now we are proposing to implement this for PL/pgsql and only
PL/pgsql. It seems to me that we can add that when and if a second
checker function shows up, if it still seems like a good idea.

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

2012/3/7 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> 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.
>
> This seems entirely reasonable to me.  The syntax support is not the
> value-add in this patch, and it's been clear since day one that it would
> be difficult for the syntax to cover all the likely permutations of
> "which functions do you want to check?".  A function-based interface
> seems like both less work and more functionality.  Yes, it's marginally
> less convenient for simple cases, but I'm not even sure that we know
> which "simple cases" are going to be popular.  We can and should
> postpone that decision until we have some field experience to base it on.
>
>> If we need both
>> plpgsql_check_function(procoid) and plpgsql_check_trigger(tgoid), no
>> problem.
>
> FWIW, I would suggest check_trigger(regclass, name) not tgoid, because
> we do not have a regtrigger convenience type (and I don't think it's
> worth adding one).
>
> More importantly, I do not agree with requiring the user to specify the
> language name --- that is, it should be check_function(procoid) and have
> that look up a language-specific checker.  Otherwise, scenarios like
> "check all my functions regardless of language" are too painful.
> There is value-added in providing that much infrastructure.

I am able to drop all coded functionality and preparing these simple
check function is relative simple work. But I am not sure if we can
design better interface in future. It is relative well designed and
consistent with other statements.

I know so you and Robert are busy, but if you can have a 10 minutes,
try it play with it. There are nice autocomplete and "well" output. It
working well in psql.

Regards

Pavel

>
>                        regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Mar 7, 2012 at 12:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> More importantly, I do not agree with requiring the user to specify the
>> language name --- that is, it should be check_function(procoid) and have
>> that look up a language-specific checker. Otherwise, scenarios like
>> "check all my functions regardless of language" are too painful.
>> There is value-added in providing that much infrastructure.

> I might agree with you if we had more than one checker function, but
> right now we are proposing to implement this for PL/pgsql and only
> PL/pgsql. It seems to me that we can add that when and if a second
> checker function shows up, if it still seems like a good idea.

That argument is just silly. The only reason there's only one checker
function is that that's all Pavel has bothered to write yet, and all
that he's likely to write since (AFAICT) he doesn't care about the other
PLs. But other people do. There is certainly value in being able to do
checking of other languages, and if we don't set this up properly now,
we're going to have problems with having to change the user-visible API
later.

I said from the beginning that I thought the most important part of this
patch was getting the API for the language-specific validator functions
right, and I remain of that opinion. If we're going to blow that off
then we should forget the patch entirely until we have time to do it
right.

regards, tom lane

2012/3/7 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Mar 7, 2012 at 12:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> More importantly, I do not agree with requiring the user to specify the
>>> language name --- that is, it should be check_function(procoid) and have
>>> that look up a language-specific checker.  Otherwise, scenarios like
>>> "check all my functions regardless of language" are too painful.
>>> There is value-added in providing that much infrastructure.
>
>> I might agree with you if we had more than one checker function, but
>> right now we are proposing to implement this for PL/pgsql and only
>> PL/pgsql.  It seems to me that we can add that when and if a second
>> checker function shows up, if it still seems like a good idea.
>
> That argument is just silly.  The only reason there's only one checker
> function is that that's all Pavel has bothered to write yet, and all
> that he's likely to write since (AFAICT) he doesn't care about the other
> PLs.  But other people do.  There is certainly value in being able to do
> checking of other languages, and if we don't set this up properly now,
> we're going to have problems with having to change the user-visible API
> later.
>
> I said from the beginning that I thought the most important part of this
> patch was getting the API for the language-specific validator functions
> right, and I remain of that opinion.  If we're going to blow that off
> then we should forget the patch entirely until we have time to do it
> right.
>

I believe so with some minimal support for other languages - tj
check_function, there will be other checker functions early. Preparing
plpgsql_check_function instead check_function save 10 lines of code,
and we will close door to other.

I am working on some minimalistic patch

Pavel

>                        regards, tom lane

On Wed, Mar 7, 2012 at 2:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Mar 7, 2012 at 12:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> More importantly, I do not agree with requiring the user to specify the
>>> language name --- that is, it should be check_function(procoid) and have
>>> that look up a language-specific checker.  Otherwise, scenarios like
>>> "check all my functions regardless of language" are too painful.
>>> There is value-added in providing that much infrastructure.
>
>> I might agree with you if we had more than one checker function, but
>> right now we are proposing to implement this for PL/pgsql and only
>> PL/pgsql.  It seems to me that we can add that when and if a second
>> checker function shows up, if it still seems like a good idea.
>
> That argument is just silly.  The only reason there's only one checker
> function is that that's all Pavel has bothered to write yet, and all
> that he's likely to write since (AFAICT) he doesn't care about the other
> PLs.  But other people do.  There is certainly value in being able to do
> checking of other languages, and if we don't set this up properly now,
> we're going to have problems with having to change the user-visible API
> later.

If we publish plpgsql_check(regproc) now and a year from now we
publish anypl_check(regproc), the former will still work. There's no
need for an API break there.

> I said from the beginning that I thought the most important part of this
> patch was getting the API for the language-specific validator functions
> right, and I remain of that opinion.  If we're going to blow that off
> then we should forget the patch entirely until we have time to do it
> right.

Well, I guess I'm still of the opinion that the real question is
whether the particular lint checks that Pavel's implemented are good
and useful things. Has anyone spent any time looking at *that*? I'm
not going to stand here and hold my breath over the interface, but it
seems to me that if we don't know that we've got a worthwhile set of
underlying functionality, sweating the interface too much is putting
the cart before the horse.

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

2012/3/7 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Wed, Mar 7, 2012 at 2:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Wed, Mar 7, 2012 at 12:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> More importantly, I do not agree with requiring the user to specify the
>>>> language name --- that is, it should be check_function(procoid) and have
>>>> that look up a language-specific checker.  Otherwise, scenarios like
>>>> "check all my functions regardless of language" are too painful.
>>>> There is value-added in providing that much infrastructure.
>>
>>> I might agree with you if we had more than one checker function, but
>>> right now we are proposing to implement this for PL/pgsql and only
>>> PL/pgsql.  It seems to me that we can add that when and if a second
>>> checker function shows up, if it still seems like a good idea.
>>
>> That argument is just silly.  The only reason there's only one checker
>> function is that that's all Pavel has bothered to write yet, and all
>> that he's likely to write since (AFAICT) he doesn't care about the other
>> PLs.  But other people do.  There is certainly value in being able to do
>> checking of other languages, and if we don't set this up properly now,
>> we're going to have problems with having to change the user-visible API
>> later.
>
> If we publish plpgsql_check(regproc) now and a year from now we
> publish anypl_check(regproc), the former will still work.  There's no
> need for an API break there.
>
>> I said from the beginning that I thought the most important part of this
>> patch was getting the API for the language-specific validator functions
>> right, and I remain of that opinion.  If we're going to blow that off
>> then we should forget the patch entirely until we have time to do it
>> right.
>
> Well, I guess I'm still of the opinion that the real question is
> whether the particular lint checks that Pavel's implemented are good
> and useful things.  Has anyone spent any time looking at *that*?  I'm
> not going to stand here and hold my breath over the interface, but it
> seems to me that if we don't know that we've got a worthwhile set of
> underlying functionality, sweating the interface too much is putting
> the cart before the horse.

the core is based over plpgsql_lint - and this is used in some
companies about year

Pavel

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Well, I guess I'm still of the opinion that the real question is
> whether the particular lint checks that Pavel's implemented are good
> and useful things. Has anyone spent any time looking at *that*? I'm
> not going to stand here and hold my breath over the interface, but it
> seems to me that if we don't know that we've got a worthwhile set of
> underlying functionality, sweating the interface too much is putting
> the cart before the horse.

No, that's backwards. I have every expectation that the specific set
of checks will be extended and improved in future. But changing the
framework, if we don't bother to get that right to start with, will be
much less pleasant.

Not that having useful checks is not an important part of the patch, of
course. But it's not what everyone has been focusing on, and I do not
believe that we were mistaken to be more worried about the framework.

regards, tom lane

Hello

2012/3/7 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> 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.
>
> This seems entirely reasonable to me.  The syntax support is not the
> value-add in this patch, and it's been clear since day one that it would
> be difficult for the syntax to cover all the likely permutations of
> "which functions do you want to check?".  A function-based interface
> seems like both less work and more functionality.  Yes, it's marginally
> less convenient for simple cases, but I'm not even sure that we know
> which "simple cases" are going to be popular.  We can and should
> postpone that decision until we have some field experience to base it on.
>
>> If we need both
>> plpgsql_check_function(procoid) and plpgsql_check_trigger(tgoid), no
>> problem.
>
> FWIW, I would suggest check_trigger(regclass, name) not tgoid, because
> we do not have a regtrigger convenience type (and I don't think it's
> worth adding one).
>
> More importantly, I do not agree with requiring the user to specify the
> language name --- that is, it should be check_function(procoid) and have
> that look up a language-specific checker.  Otherwise, scenarios like
> "check all my functions regardless of language" are too painful.
> There is value-added in providing that much infrastructure.

here is implementation of reduced patch - it is not final - no doc, no
regress test, just only functional interface

postgres=> \sf fx
CREATE OR REPLACE FUNCTION public.fx()
RETURNS SETOF text
LANGUAGE plpgsql
AS $function$
begin
return next 'In function f1():';
return next 'error:afasdf:asfsafdsgfsgf' || a;
return;
end;
$function$
postgres=> select pg_check_function('fx()');
pg_check_function
-----------------------------------------------------
In function fx():
error:42703:4:RETURN NEXT:column "a" does not exist
Query: SELECT 'error:afasdf:asfsafdsgfsgf' || a
-- ^
(4 rows)

caret is on correct position

I'll prepare check_trigger function tomorrow

Regards

Pavel

>
>                        regards, tom lane

Hello

2012/3/7 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> 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.
>
> This seems entirely reasonable to me.  The syntax support is not the
> value-add in this patch, and it's been clear since day one that it would
> be difficult for the syntax to cover all the likely permutations of
> "which functions do you want to check?".  A function-based interface
> seems like both less work and more functionality.  Yes, it's marginally
> less convenient for simple cases, but I'm not even sure that we know
> which "simple cases" are going to be popular.  We can and should
> postpone that decision until we have some field experience to base it on.
>
>> If we need both
>> plpgsql_check_function(procoid) and plpgsql_check_trigger(tgoid), no
>> problem.
>
> FWIW, I would suggest check_trigger(regclass, name) not tgoid, because
> we do not have a regtrigger convenience type (and I don't think it's
> worth adding one).
>
> More importantly, I do not agree with requiring the user to specify the
> language name --- that is, it should be check_function(procoid) and have
> that look up a language-specific checker.  Otherwise, scenarios like
> "check all my functions regardless of language" are too painful.
> There is value-added in providing that much infrastructure.

here is updated patch (with regress tests, with documentation).

I removed a CHECK FUNCTION and CHECK TRIGGER statements and replaced
it by pg_check_function and pg_check_trigger like Tom proposed.

The core of this patch is same - plpgsql checker, only user interface
was reduced.

postgres=> select pg_check_function('f2()');
pg_check_function
---------------------------------------------------------------
error:42703:3:RETURN:column "missing_variable" does not exist
Query: SELECT 'Hello' || missing_variable
-- ^
(3 rows)

postgres=> select pg_check_trigger('t','t1');
pg_check_trigger
--------------------------------------------------------
error:42703:3:assignment:record "new" has no field "b"
Context: SQL statement "SELECT new.b + 1"
(2 rows)

Regards

Pavel

>
>                        regards, tom lane

Robert Haas wrote:
> Well, I guess I'm still of the opinion that the real question is
> whether the particular lint checks that Pavel's implemented are good
> and useful things. Has anyone spent any time looking at *that*?

Actually, I did when I reviewed the patch the first time round.
I think that the checks implemented are clearly good and useful,
since any problem reported will lead to an error at runtime if
a certain code path in the function is taken. And if the code path
is never taken, that's valuable information too.

I don't say that there are no good checks missing, but the ones
that are there are good IMO.

Yours,
Laurenz Albe

On 03/08/2012 08:35 AM, Pavel Stehule wrote:
> Here is updated patch (with regress tests, with documentation).
>
> I removed a CHECK FUNCTION and CHECK TRIGGER statements and replaced
> it by pg_check_function and pg_check_trigger like Tom proposed.
>
> The core of this patch is same - plpgsql checker, only user interface
> was reduced.
>
> postgres=> select pg_check_function('f2()');
> pg_check_function
> ---------------------------------------------------------------
> error:42703:3:RETURN:column "missing_variable" does not exist
> Query: SELECT 'Hello' || missing_variable
> -- ^
> (3 rows)
>
> postgres=> select pg_check_trigger('t','t1');
> pg_check_trigger
> --------------------------------------------------------
> error:42703:3:assignment:record "new" has no field "b"
> Context: SQL statement "SELECT new.b + 1"
> (2 rows)
>

I did rereview and rechecked with few dozen of real-world functions, and
it still looks good from my point of view. I made bit of adjustment of
english in new comments and Pavel sent me privately fix for proper
handling of languages that don't have checker function. Updated patch
attached.

Regards
Petr Jelinek

On ons, 2012-03-07 at 12:31 -0500, Robert Haas wrote:
> I might agree with you if we had more than one checker function, but
> right now we are proposing to implement this for PL/pgsql and only
> PL/pgsql. It seems to me that we can add that when and if a second
> checker function shows up, if it still seems like a good idea.

I had mentioned upthread that I would like to use this for PL/Python.
There are a number of code quality checkers out there for Python. I
currently have 3 hooked into Emacs, and 2 or 3 are typically used in the
builds of projects I'm working on. All of these are shipped separately
from Python.

This leads to the following requirements:

* Multiple checkers per language must be supported.
* It must be possible to add checkers to a language after it is
created. For example, a checker could be shipped in an
extension.
* It's not terribly important to me to be able to run checkers
separately. If I wanted to do that, I would just disable or
remove the checker.
* Just to make things interesting, it should be possible to
implement checkers for language X in language X.

If it would help, given an API (even if only in C at the moment), I
could probably write up one or two checker function prototypes that
could be run against the PL/Python regression test corpus.

On tor, 2012-03-08 at 10:49 +0100, Albe Laurenz wrote:
> Actually, I did when I reviewed the patch the first time round.
> I think that the checks implemented are clearly good and useful,
> since any problem reported will lead to an error at runtime if
> a certain code path in the function is taken.

Shouldn't the validator just reject the function in those cases?

2012/3/8 Peter Eisentraut <peter_e(at)gmx(dot)net>:
> On tor, 2012-03-08 at 10:49 +0100, Albe Laurenz wrote:
>> Actually, I did when I reviewed the patch the first time round.
>> I think that the checks implemented are clearly good and useful,
>> since any problem reported will lead to an error at runtime if
>> a certain code path in the function is taken.
>
> Shouldn't the validator just reject the function in those cases?
>

Validator check syntax only (and cannot do more, because there should
not be dependency between functions). But it doesn't verify if table
exists, if table has refereed columns, if number of expressions in
raise statement is equal to number of substitute symbols ...

Regards

Pavel

2012/3/8 Peter Eisentraut <peter_e(at)gmx(dot)net>:
> On ons, 2012-03-07 at 12:31 -0500, Robert Haas wrote:
>> I might agree with you if we had more than one checker function, but
>> right now we are proposing to implement this for PL/pgsql and only
>> PL/pgsql.  It seems to me that we can add that when and if a second
>> checker function shows up, if it still seems like a good idea.
>
> I had mentioned upthread that I would like to use this for PL/Python.
> There are a number of code quality checkers out there for Python.  I
> currently have 3 hooked into Emacs, and 2 or 3 are typically used in the
> builds of projects I'm working on.  All of these are shipped separately
> from Python.
>
> This leads to the following requirements:
>
>      * Multiple checkers per language must be supported.
>      * It must be possible to add checkers to a language after it is
>        created.  For example, a checker could be shipped in an
>        extension.
>      * It's not terribly important to me to be able to run checkers
>        separately.  If I wanted to do that, I would just disable or
>        remove the checker.
>      * Just to make things interesting, it should be possible to
>        implement checkers for language X in language X.

But you propose some little bit different than is current plpgsql
checker and current design. You need hook on CREATE FUNCTION probably?
It's not bad, but it is some different and it is not useful for
plpgsql - external stored procedures are different, than SQL
procedures and probably you will check different issues.

I don't think so multiple checkers and external checkers are necessary
- if some can living outside, then it should to live outside. Internal
checker can be one for PL language. It is parametrized - so you can
control goals of checking.

>
> If it would help, given an API (even if only in C at the moment), I
> could probably write up one or two checker function prototypes that
> could be run against the PL/Python regression test corpus.
>

sure, please look on patch and plpgsql checker function. Checker can
be any function with same interface. Some PL/Python checker can be
good.

Regards

Pavel

>

On Thu, Mar 8, 2012 at 4:54 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>      * It's not terribly important to me to be able to run checkers
>        separately.  If I wanted to do that, I would just disable or
>        remove the checker.

Does this requirement mean that you want to essentially associate a
set of checkers with each language and then, when asked to check a
function, run all of them serially in an undefined order?

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

On tor, 2012-03-08 at 23:15 +0100, Pavel Stehule wrote:
> But you propose some little bit different than is current plpgsql
> checker and current design.

Is it? Why? It looks like exactly the same thing, except that the
interfaces you propose are tightly geared toward checking SQL-like
languages, which looks like a mistake to me.

> It's not bad, but it is some different and it is not useful for
> plpgsql - external stored procedures are different, than SQL
> procedures and probably you will check different issues.
>
> I don't think so multiple checkers and external checkers are necessary
> - if some can living outside, then it should to live outside. Internal
> checker can be one for PL language. It is parametrized - so you can
> control goals of checking.

What would be the qualifications for being an internal or an external
checker? Why couldn't your plpgsql checker be an external checker?

On tor, 2012-03-08 at 19:19 -0500, Robert Haas wrote:
> On Thu, Mar 8, 2012 at 4:54 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > * It's not terribly important to me to be able to run checkers
> > separately. If I wanted to do that, I would just disable or
> > remove the checker.
>
> Does this requirement mean that you want to essentially associate a
> set of checkers with each language and then, when asked to check a
> function, run all of them serially in an undefined order?

Well, the more I think about it and look at this patch, the more I think
that this would be complete overkill and possibly quite useless for my
purposes. I can implement the entire essence of this framework (except
the plpgsql_checker itself, which is clearly useful) in 10 lines,
namely:

CREATE OR REPLACE FUNCTION pep8(src text) RETURNS text
IMMUTABLE
LANGUAGE plsh
AS $$
#!/bin/bash

pep8 --ignore=W391 <(echo "$1") 2>&1 | sed -r 's/^[^:]*://'
$$;

SELECT proname, pep8(prosrc) FROM pg_proc WHERE prolang = ANY (SELECT oid FROM pg_language WHERE lanname LIKE '%python%') ORDER BY 1;

I don't know what more one would need.

On Fri, Mar 9, 2012 at 3:15 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On tor, 2012-03-08 at 19:19 -0500, Robert Haas wrote:
>> On Thu, Mar 8, 2012 at 4:54 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> >      * It's not terribly important to me to be able to run checkers
>> >        separately.  If I wanted to do that, I would just disable or
>> >        remove the checker.
>>
>> Does this requirement mean that you want to essentially associate a
>> set of checkers with each language and then, when asked to check a
>> function, run all of them serially in an undefined order?
>
> Well, the more I think about it and look at this patch, the more I think
> that this would be complete overkill and possibly quite useless for my
> purposes.  I can implement the entire essence of this framework (except
> the plpgsql_checker itself, which is clearly useful) in 10 lines,
> namely:
>
> CREATE OR REPLACE FUNCTION pep8(src text) RETURNS text
> IMMUTABLE
> LANGUAGE plsh
> AS $$
> #!/bin/bash
>
> pep8 --ignore=W391 <(echo "$1") 2>&1 | sed -r 's/^[^:]*://'
> $$;
>
> SELECT proname, pep8(prosrc) FROM pg_proc WHERE prolang = ANY (SELECT oid FROM pg_language WHERE lanname LIKE '%python%') ORDER BY 1;
>
> I don't know what more one would need.

Well, I agree with you, but Tom disagrees, so that's why we're talking
about it...

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Mar 9, 2012 at 3:15 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> Well, the more I think about it and look at this patch, the more I think
>> that this would be complete overkill and possibly quite useless for my
>> purposes. I can implement the entire essence of this framework (except
>> the plpgsql_checker itself, which is clearly useful) in 10 lines,
>> namely:
>>
>> CREATE OR REPLACE FUNCTION pep8(src text) RETURNS text
>> IMMUTABLE
>> LANGUAGE plsh
>> AS $$
>> #!/bin/bash
>>
>> pep8 --ignore=W391 <(echo "$1") 2>&1 | sed -r 's/^[^:]*://'
>> $$;
>>
>> SELECT proname, pep8(prosrc) FROM pg_proc WHERE prolang = ANY (SELECT oid FROM pg_language WHERE lanname LIKE '%python%') ORDER BY 1;
>>
>> I don't know what more one would need.

> Well, I agree with you, but Tom disagrees, so that's why we're talking
> about it...

What Peter's example demonstrates is that you can apply a single checker
for a single language without bothering with any common framework.
Well, yeah. What I've wanted from this patch from the beginning was a
common framework. That is, I want to be able to write something like

SELECT check_function(oid) FROM pg_proc WHERE proowner = 'tgl'

and have it "just work" for all languages for which I have checkers.
You can't get that with a collection of ad-hoc checkers.

If we're going to go the ad-hoc route, there seems little reason to be
considering a core patch at all. Freestanding checkers could just as
well be independent projects.

regards, tom lane

2012/3/9 Peter Eisentraut <peter_e(at)gmx(dot)net>:
> On tor, 2012-03-08 at 23:15 +0100, Pavel Stehule wrote:
>> But you propose some little bit different than is current plpgsql
>> checker and current design.
>
> Is it?  Why?  It looks like exactly the same thing, except that the
> interfaces you propose are tightly geared toward checking SQL-like
> languages, which looks like a mistake to me.

no, you can check any PL language - and output result is based on SQL
Errors, so it should be enough for all PL too.

>
>> It's not bad, but it is some different and it is not useful for
>> plpgsql - external stored procedures are different, than SQL
>> procedures and probably you will check different issues.
>>
>> I don't think so multiple checkers and external checkers are necessary
>> - if some can living outside, then it should to live outside. Internal
>> checker can be one for PL language. It is parametrized - so you can
>> control goals of checking.
>
> What would be the qualifications for being an internal or an external
> checker?  Why couldn't your plpgsql checker be an external checker?

plpgsql checker cannot be external checker, because it reuse 70% of
plpgsql environment, - parser, runtime, ...

so creating a external checker is equal to creating a second plpgsql
environment - maybe reduced, but you have to duplicate parser, sql
integration, ...

Regards

Pavel

>
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2012/3/9 Peter Eisentraut <peter_e(at)gmx(dot)net>:
>> What would be the qualifications for being an internal or an external
>> checker? Why couldn't your plpgsql checker be an external checker?

> plpgsql checker cannot be external checker, because it reuse 70% of
> plpgsql environment, - parser, runtime, ...

Well, that just means that it'd be a good idea for that function to be
supplied by the same shared library that supplies the plpgsql execution
functions. There wouldn't need to be any connection that the core
system particularly understands. So, like Peter, I'm not quite sure
what distinction is meant to be drawn by "internal" vs "external".

The thing that really struck me about Peter's previous comments was the
desire to support multiple checkers per PL. I had been assuming that
we'd just extend the "validator" model in some way --- either another
column in pg_language or extending the API for validator functions.
Neither of those work if we want to allow multiple checkers. Now,
I'm not at all convinced that multiple checkers are worth the trouble
... but if they are it seems like we need a different system catalog to
store them in. And the entries in that catalog wouldn't necessarily be
created by the same extension that creates the PL language.

regards, tom lane

On Fri, Mar 9, 2012 at 3:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> If we're going to go the ad-hoc route, there seems little reason to be
> considering a core patch at all.  Freestanding checkers could just as
> well be independent projects.

I completely agree. I think there is little reason to be considering
a core patch. I haven't seen any convincing evidence (or any evidence
at all) that being able to fling checkers at a large number of
functions written in different procedural languages is an important
use case for anyone. I think the vast majority of checking will get
done one function at a time; and therefore we are gilding the lily.

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

2012/3/9 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2012/3/9 Peter Eisentraut <peter_e(at)gmx(dot)net>:
>>> What would be the qualifications for being an internal or an external
>>> checker?  Why couldn't your plpgsql checker be an external checker?
>
>> plpgsql checker cannot be external checker, because it reuse 70% of
>> plpgsql environment, - parser, runtime, ...
>
> Well, that just means that it'd be a good idea for that function to be
> supplied by the same shared library that supplies the plpgsql execution
> functions.  There wouldn't need to be any connection that the core
> system particularly understands.  So, like Peter, I'm not quite sure
> what distinction is meant to be drawn by "internal" vs "external".

internal - implement in core, external - implement in extension.

This is history about this feature:

a) I wrote plgsql_lint, and I proposed it as core plpgsql functionality
b) there was request using some different then GUC, and there was
first check_function
c) there was request do it with more user friendly - there is CHECK xxx
d) there was request for support multiple checks, there is CHECK xxx ALL
e) these features was reduced - step by step,... but really important @a

Personally I think so any form of plpgsql check is big step against
current state. We can move general check functions to plpgsql space,
and it can be good enough for plpgsql developers. It is not user
friendly like CHECK FUNCTION is it, but it has important functionality
- checking of embedded SQL without execution and without dependency on
executed paths.

I cannot to move plpgsql checker to extension, because there is
dependency on plpgsql lib, and this is problem. If I can do it, then I
did it

>
> The thing that really struck me about Peter's previous comments was the
> desire to support multiple checkers per PL.  I had been assuming that
> we'd just extend the "validator" model in some way

I tried to do it, but it is not practical - a interface of validators
is volatile now - it is different for functions and for triggers, and
it doesn't expect returning anything else than exception. More - other
new functionality can increase complexity of current plpgsql
validators. So this is reason, why I designed new handler function.

--- either another
> column in pg_language or extending the API for validator functions.
> Neither of those work if we want to allow multiple checkers.  Now,
> I'm not at all convinced that multiple checkers are worth the trouble

I don't see a reason why we need a multiple checkers - checkers are
parametrised, so there are no real reason, But what statement will be
maintain this catalog - CREATE CHECK ? You need DROP, ALTER, .. it is
lot code too.

> ... but if they are it seems like we need a different system catalog to
> store them in.  And the entries in that catalog wouldn't necessarily be
> created by the same extension that creates the PL language.

This looks like real overhead. Without user interface - like CHECK
statement, what is value of this?

I am skeptic. Can we go back and can we solve a checking just plpgsql
- it just needs integration of plpgsql checker to plpgsql runtime. Any
PL can have one or more these functions. And when we will have a
adequate user API - SQL statements (CHECK statements or some else),
then we can create a new catalog. It is strange do it in different
order.

Regards

Pavel

>
>                        regards, tom lane

On Fri, Mar 9, 2012 at 5:09 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> Well, that just means that it'd be a good idea for that function to be
>> supplied by the same shared library that supplies the plpgsql execution
>> functions.  There wouldn't need to be any connection that the core
>> system particularly understands.  So, like Peter, I'm not quite sure
>> what distinction is meant to be drawn by "internal" vs "external".
>
> internal - implement in core, external - implement in extension.
[...]
> I cannot to move plpgsql checker to extension, because there is
> dependency on plpgsql lib, and this is problem. If I can do it, then I
> did it

I don't object to having this feature live in src/pl/plpgsql, and I
don't think Tom's objecting to that either. I just don't think it
needs any particular support in src/backend.

> I don't see a reason why we need a multiple checkers - checkers are
> parametrised, so there are no real reason, But what statement will be
> maintain this catalog - CREATE CHECK ? You need DROP, ALTER, .. it is
> lot code too.

If the checkers are written by different people and shipped
separately, then a parameter interface does not make anything better.

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

2012/3/9 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Mar 9, 2012 at 5:09 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> Well, that just means that it'd be a good idea for that function to be
>>> supplied by the same shared library that supplies the plpgsql execution
>>> functions.  There wouldn't need to be any connection that the core
>>> system particularly understands.  So, like Peter, I'm not quite sure
>>> what distinction is meant to be drawn by "internal" vs "external".
>>
>> internal - implement in core, external - implement in extension.
> [...]
>> I cannot to move plpgsql checker to extension, because there is
>> dependency on plpgsql lib, and this is problem. If I can do it, then I
>> did it
>
> I don't object to having this feature live in src/pl/plpgsql, and I
> don't think Tom's objecting to that either.  I just don't think it
> needs any particular support in src/backend.
>
>> I don't see a reason why we need a multiple checkers - checkers are
>> parametrised, so there are no real reason, But what statement will be
>> maintain this catalog - CREATE CHECK ? You need DROP, ALTER, .. it is
>> lot code too.
>
> If the checkers are written by different people and shipped
> separately, then a parameter interface does not make anything better.

ok - it has sense, but it has sense only with some "smart" statements
(like CHECK). Without these statements I have to directly call checker
function and then concept of generalised checkers has not sense.

Pavel

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

On Fri, Mar 9, 2012 at 5:31 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> ok - it has sense, but it has sense only with some "smart" statements
> (like CHECK). Without these statements I have to directly call checker
> function and then  concept of generalised checkers has not sense.

I agree.

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

On fre, 2012-03-09 at 21:54 +0100, Pavel Stehule wrote:
> no, you can check any PL language - and output result is based on SQL
> Errors, so it should be enough for all PL too.

But then I would have to map all language-specific error reports to some
SQL error scheme, which is not only cumbersome but pretty useless. For
example, a Python programmer will be familiar with the typical output
that pylint produces and how to fix it. If we hide that output behind
the layer of SQL-ness, that won't make things easier to anyone.

On fre, 2012-03-09 at 15:33 -0500, Tom Lane wrote:
> What I've wanted from this patch from the beginning was a
> common framework. That is, I want to be able to write something like
>
> SELECT check_function(oid) FROM pg_proc WHERE proowner = 'tgl'
>
> and have it "just work" for all languages for which I have checkers.
> You can't get that with a collection of ad-hoc checkers.

Well, there isn't any program either that will run through, say, the
PostgreSQL source tree and check each file according to the respective
programming language. You pick the checkers for each language yourself
and decide for each checker how to apply it and how to process the
results.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> But then I would have to map all language-specific error reports to some
> SQL error scheme, which is not only cumbersome but pretty useless. For
> example, a Python programmer will be familiar with the typical output
> that pylint produces and how to fix it. If we hide that output behind
> the layer of SQL-ness, that won't make things easier to anyone.

Yeah, this is a good point. I'm willing to concede that we are not
close to having a uniform API that could be used for checker functions,
so maybe what we should do for now is just invent
plpgsql_check_function(regprocedure). I'd still like to see the
question revisited sometime in the future, but it would be appropriate
to have a few working examples of popular checker functions for
different languages before we try to invent a common API.

regards, tom lane

2012/3/10 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> But then I would have to map all language-specific error reports to some
>> SQL error scheme, which is not only cumbersome but pretty useless.  For
>> example, a Python programmer will be familiar with the typical output
>> that pylint produces and how to fix it.  If we hide that output behind
>> the layer of SQL-ness, that won't make things easier to anyone.
>
> Yeah, this is a good point.  I'm willing to concede that we are not
> close to having a uniform API that could be used for checker functions,
> so maybe what we should do for now is just invent
> plpgsql_check_function(regprocedure).  I'd still like to see the
> question revisited sometime in the future, but it would be appropriate
> to have a few working examples of popular checker functions for
> different languages before we try to invent a common API.

ok, I'll prepare patch

Pavel
>
>                        regards, tom lane

Hello

2012/3/10 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> But then I would have to map all language-specific error reports to some
>> SQL error scheme, which is not only cumbersome but pretty useless.  For
>> example, a Python programmer will be familiar with the typical output
>> that pylint produces and how to fix it.  If we hide that output behind
>> the layer of SQL-ness, that won't make things easier to anyone.
>
> Yeah, this is a good point.  I'm willing to concede that we are not
> close to having a uniform API that could be used for checker functions,
> so maybe what we should do for now is just invent
> plpgsql_check_function(regprocedure).  I'd still like to see the
> question revisited sometime in the future, but it would be appropriate
> to have a few working examples of popular checker functions for
> different languages before we try to invent a common API.
>

here is draft

I removed all generic structures.

Regards

Pavel

>                        regards, tom lane

>>
>
> here is draft

and there some cleaned version

Regards

Pavel Stehule

>
>
>>                        regards, tom lane

On 03/10/2012 04:36 PM, Pavel Stehule wrote:
> and there some cleaned version
>
>

Reran my tests and adjusted docs a bit, tbh I considered the previous
versions way more useful but even in this form it's nice and useful
functionality.

Regards
Petr Jelinek

2012/3/11 Petr Jelinek <pjmodos(at)pjmodos(dot)net>:
> On 03/10/2012 04:36 PM, Pavel Stehule wrote:
>>
>> and there some cleaned version
>>
>>
>
>
> Reran my tests and adjusted docs a bit, tbh I considered the previous
> versions way more useful but even in this form it's nice and useful
> functionality.

thank you - this version is base, and it doesn't block any enhancement
in future.

Regards

Pavel
>
> Regards
> Petr Jelinek

2012/3/11 Petr Jelinek <pjmodos(at)pjmodos(dot)net>:
> On 03/10/2012 04:36 PM, Pavel Stehule wrote:
>>
>> and there some cleaned version
>>
>>
>
>
> Reran my tests and adjusted docs a bit, tbh I considered the previous
> versions way more useful but even in this form it's nice and useful
> functionality.
>

remove two lines of death code

Regards

Pavel

> Regards
> Petr Jelinek

Ok, seems that the API issue is settled, so I'm now looking at the code
actually doing the checking. My first impression is that this is a lot
of code. Can we simplify it?

Since this is deeply integrated into the PL/pgSQL interpreter, I was
expecting that this would run through the normal interpreter, in a
special mode that skips all the actual execution of queries, and
shortcuts all loops and other control statements so that all code is
executed only once. That would mean sprinkling some "if (check_only)"
code into the normal exec_* functions. I'm not sure how invasive that
would be, but it's worth considering. I think you would be able to more
easily catch more errors that way, and the check code would stay better
in sync with the execution code.

Another thought is that check_stmt() and all its subroutines are very
similar to the plpgsql_dumptree() code. Would it make sense to merge
those? You could have an output mode, in addition to the xml and
plain-text formats, that would just dump the whole tree like
plpgsql_dumptree() does.

In prepare_expr(), you use a subtransaction to catch any ERRORs that
happen during parsing the expression. That's a good idea, and I think
many of the check_* functions could be greatly simplified by adopting a
similar approach. Just ereport() any errors you find, and catch them at
the appropriate level, appending the error to the output string. Your
current approach of returning true/false depending on whether there was
any errors seems tedious.

If you create a function with an invalid body (ie. set
check_function_bodies=off; create function ... $$ bogus $$;) ,
plpgsql_check_function() still throws an error. It's understandable that
it cannot do in-depth analysis if the function cannot be parsed, but I
would expect the syntax error to be returned as a return value like
other errors that it complains about, not thrown as a hard ERROR. That
would make it more useful to bulk-check all functions in a database with
something like "select plpgsql_check_function(oid) from pg_class". As it
is, the checking stops at the first invalid function with an error.

PS. I think plpgsql_check_function() belongs in pl_handler.c

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Hello

2012/3/28 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> Ok, seems that the API issue is settled, so I'm now looking at the code
> actually doing the checking. My first impression is that this is a lot of
> code. Can we simplify it?
>

I am afraid so there are not a big space for simplification :(

> Since this is deeply integrated into the PL/pgSQL interpreter, I was
> expecting that this would run through the normal interpreter, in a special
> mode that skips all the actual execution of queries, and shortcuts all loops
> and other control statements so that all code is executed only once. That
> would mean sprinkling some "if (check_only)" code into the normal exec_*
> functions. I'm not sure how invasive that would be, but it's worth
> considering. I think you would be able to more easily catch more errors that
> way, and the check code would stay better in sync with the execution code.
>

This can mess current code - it can works, but some important
fragments can be less readable after. Almost all "eval" routines
should supports fake mode, and it can be little bit slower and less
readable.

> Another thought is that check_stmt() and all its subroutines are very
> similar to the plpgsql_dumptree() code. Would it make sense to merge those?
> You could have an output mode, in addition to the xml and plain-text
> formats, that would just dump the whole tree like plpgsql_dumptree() does.
>

yes, it is possible - first implementation was via walker, and it was
reused for dump. Current code is more readable, but there is not
reuse. I am able to redesign code to this direction.

> In prepare_expr(), you use a subtransaction to catch any ERRORs that happen
> during parsing the expression. That's a good idea, and I think many of the
> check_* functions could be greatly simplified by adopting a similar
> approach. Just ereport() any errors you find, and catch them at the
> appropriate level, appending the error to the output string. Your current
> approach of returning true/false depending on whether there was any errors
> seems tedious.

This is not possible, when we would to enable "fatal_errors = false"
checking. I can do subtransaction in prepare_expr, because it is the
most deep level, but I cannot to use it elsewhere, because I cannot
handle exception and continue with other parts of statement.

>
> If you create a function with an invalid body (ie. set
> check_function_bodies=off; create function ... $$ bogus $$;) ,
> plpgsql_check_function() still throws an error. It's understandable that it
> cannot do in-depth analysis if the function cannot be parsed, but I would
> expect the syntax error to be returned as a return value like other errors
> that it complains about, not thrown as a hard ERROR. That would make it more
> useful to bulk-check all functions in a database with something like "select
> plpgsql_check_function(oid) from pg_class". As it is, the checking stops at
> the first invalid function with an error.
>

it is good idea

> PS. I think plpgsql_check_function() belongs in pl_handler.c

can be - why not.

Regards

Pavel

>
> --
>  Heikki Linnakangas
>  EnterpriseDB   http://www.enterprisedb.com

On 28.03.2012 23:54, Pavel Stehule wrote:
> 2012/3/28 Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com>:
>> In prepare_expr(), you use a subtransaction to catch any ERRORs that happen
>> during parsing the expression. That's a good idea, and I think many of the
>> check_* functions could be greatly simplified by adopting a similar
>> approach. Just ereport() any errors you find, and catch them at the
>> appropriate level, appending the error to the output string. Your current
>> approach of returning true/false depending on whether there was any errors
>> seems tedious.
>
> This is not possible, when we would to enable "fatal_errors = false"
> checking. I can do subtransaction in prepare_expr, because it is the
> most deep level, but I cannot to use it elsewhere, because I cannot
> handle exception and continue with other parts of statement.

Well, you can continue on the next statement. That's almost as good. In
practice, if there's one error in a statement, it seems unlikely that
you would correctly diagnose other errors on the same line. They're more
likely to be fallout of the same mistake.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

2012/3/29 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> On 28.03.2012 23:54, Pavel Stehule wrote:
>>
>> 2012/3/28 Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com>:
>>
>>> In prepare_expr(), you use a subtransaction to catch any ERRORs that
>>> happen
>>> during parsing the expression. That's a good idea, and I think many of
>>> the
>>> check_* functions could be greatly simplified by adopting a similar
>>> approach. Just ereport() any errors you find, and catch them at the
>>> appropriate level, appending the error to the output string. Your current
>>> approach of returning true/false depending on whether there was any
>>> errors
>>> seems tedious.
>>
>>
>> This is not possible, when we would to enable "fatal_errors = false"
>> checking. I can do subtransaction in prepare_expr, because it is the
>> most deep level, but I cannot to use it elsewhere, because I cannot
>> handle exception and continue with other parts of statement.
>
>
> Well, you can continue on the next statement. That's almost as good. In
> practice, if there's one error in a statement, it seems unlikely that you
> would correctly diagnose other errors on the same line. They're more likely
> to be fallout of the same mistake.

no, for example, it means, if I found error in "if_then" statements,
still I would to check "else" statements.

Regards

Pavel

>
>
> --
>  Heikki Linnakangas
>  EnterpriseDB   http://www.enterprisedb.com

Hello

2012/3/28 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> Ok, seems that the API issue is settled, so I'm now looking at the code
> actually doing the checking. My first impression is that this is a lot of
> code. Can we simplify it?
>

I played with this and It is not be reduced without darkening current
code of pl_exec.c. So I moved code related to checking from files to
new file pl_check.c. This code is relative large - about 50 kb, but it
is relative simple and I hope it is readable. I afraid so this cannot
be reduced by reuse with other pl_func.c (significantly). Recursive
iteration over nodes is relative not big part of this patch (~25%) and
general stmt walker doesn't help too much.

> Since this is deeply integrated into the PL/pgSQL interpreter, I was
> expecting that this would run through the normal interpreter, in a special
> mode that skips all the actual execution of queries, and shortcuts all loops
> and other control statements so that all code is executed only once. That
> would mean sprinkling some "if (check_only)" code into the normal exec_*
> functions. I'm not sure how invasive that would be, but it's worth
> considering. I think you would be able to more easily catch more errors that
> way, and the check code would stay better in sync with the execution code.

-1

there are a few places, that are very difficult: basic block with
exception handling - exception handlers, CASE stmt, .... Other issue
is increasing of complexity some routines like exec_eval*

>
> Another thought is that check_stmt() and all its subroutines are very
> similar to the plpgsql_dumptree() code. Would it make sense to merge those?
> You could have an output mode, in addition to the xml and plain-text
> formats, that would just dump the whole tree like plpgsql_dumptree() does.
>

It is difficult now - without changes in plpgsql_stmt_if,
plpgsql_stmt_case and plpgsql_stmt_block is not possible to write
general walker that is usable for checking and dumptree. It needs
redesign of these nodes first.

> In prepare_expr(), you use a subtransaction to catch any ERRORs that happen
> during parsing the expression. That's a good idea, and I think many of the
> check_* functions could be greatly simplified by adopting a similar
> approach. Just ereport() any errors you find, and catch them at the
> appropriate level, appending the error to the output string. Your current
> approach of returning true/false depending on whether there was any errors
> seems tedious.

It cannot be implemented in AST interpret. Without removing some
requested functionality - fatal_errors.

>
> If you create a function with an invalid body (ie. set
> check_function_bodies=off; create function ... $$ bogus $$;) ,
> plpgsql_check_function() still throws an error. It's understandable that it
> cannot do in-depth analysis if the function cannot be parsed, but I would
> expect the syntax error to be returned as a return value like other errors
> that it complains about, not thrown as a hard ERROR. That would make it more
> useful to bulk-check all functions in a database with something like "select
> plpgsql_check_function(oid) from pg_class". As it is, the checking stops at
> the first invalid function with an error.

done

postgres=> select plpgsql_check_function('sss'::regproc, 0);
plpgsql_check_function
-------------------------------------------------------------
error:42601:syntax error at or near "adasdfsadf"
Query: adasdfsadf
-- ^
Context: compilation of PL/pgSQL function "sss" near line 1
(4 rows)

>
> PS. I think plpgsql_check_function() belongs in pl_handler.c

done

Regards

Pavel Stehule

>
> --
>  Heikki Linnakangas
>  EnterpriseDB   http://www.enterprisedb.com

On 30.03.2012 12:36, Pavel Stehule wrote:
> 2012/3/28 Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com>:
>> In prepare_expr(), you use a subtransaction to catch any ERRORs that happen
>> during parsing the expression. That's a good idea, and I think many of the
>> check_* functions could be greatly simplified by adopting a similar
>> approach. Just ereport() any errors you find, and catch them at the
>> appropriate level, appending the error to the output string. Your current
>> approach of returning true/false depending on whether there was any errors
>> seems tedious.
>
> It cannot be implemented in AST interpret. Without removing some
> requested functionality - fatal_errors.

I don't think I'm getting my point across by explaining, so here's a
modified version of the patch that does what I was trying to say. The
general pattern of the checker functions has been changed. Instead of
returning a boolean indicating error or no error, with checks for
fatal_errors scattered around them, the internal checker functions now
return nothing. Any errors are reported with ereport(), and there is a
PG_TRY/CATCH block in a couple of carefully chosen places: in
check_stmt(), so that if you get an error while checking a statement,
you continue checking on the next statement, and in check_assignment()
which is now used by check_expr() and a few other helper functions to
basically check all expressions and SQL statements.

IMHO this makes the code much more readable, now that the control logic
of when to return and when to continue is largely gone. A lot of other
cleanup still needs to be done, I just wanted to demonstrate this
ereport+try/catch idea with this patch.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> I don't think I'm getting my point across by explaining, so here's a
> modified version of the patch that does what I was trying to say.

Minor side point: some of the diff noise in this patch comes from
s/copy_plpgsql_datum/plpgsql_copy_plpgsql_datum/, which seems entirely
useless. The name already contains "plpgsql", and even if it didn't,
there is no particular reason for plpgsql to worry about polluting
global symbol namespace. Nothing else resolves against its symbols
anyway, at least not on any platform we claim to support. I would
therefore also argue against the other renamings like
s/exec_move_row/plpgsql_exec_move_row/.

regards, tom lane

On 04.04.2012 19:32, Tom Lane wrote:
> Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
>> I don't think I'm getting my point across by explaining, so here's a
>> modified version of the patch that does what I was trying to say.
>
> Minor side point: some of the diff noise in this patch comes from
> s/copy_plpgsql_datum/plpgsql_copy_plpgsql_datum/, which seems entirely
> useless. The name already contains "plpgsql", and even if it didn't,
> there is no particular reason for plpgsql to worry about polluting
> global symbol namespace. Nothing else resolves against its symbols
> anyway, at least not on any platform we claim to support. I would
> therefore also argue against the other renamings like
> s/exec_move_row/plpgsql_exec_move_row/.

Agreed. Looking closer, I'm not sure we even need to expose
exec_move_row() to pl_check.c. It's only used to initialize row-type
function arguments to NULL. But variables that are not explicitly
initialized are NULL anyway, and the checker shouldn't use the values
stored in variables for anything, so I believe that initialization in
function_check() can be replaced with something much simpler or removed
altogether.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

2012/4/4 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> On 04.04.2012 19:32, Tom Lane wrote:
>>
>> Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com>  writes:
>>>
>>> I don't think I'm getting my point across by explaining, so here's a
>>> modified version of the patch that does what I was trying to say.
>>
>>
>> Minor side point: some of the diff noise in this patch comes from
>> s/copy_plpgsql_datum/plpgsql_copy_plpgsql_datum/, which seems entirely
>> useless.  The name already contains "plpgsql", and even if it didn't,
>> there is no particular reason for plpgsql to worry about polluting
>> global symbol namespace.  Nothing else resolves against its symbols
>> anyway, at least not on any platform we claim to support.  I would
>> therefore also argue against the other renamings like
>> s/exec_move_row/plpgsql_exec_move_row/.
>
>
> Agreed. Looking closer, I'm not sure we even need to expose exec_move_row()
> to pl_check.c. It's only used to initialize row-type function arguments to
> NULL. But variables that are not explicitly initialized are NULL anyway, and
> the checker shouldn't use the values stored in variables for anything, so I
> believe that initialization in function_check() can be replaced with
> something much simpler or removed altogether.

+1

Pavel

>
>
> --
>  Heikki Linnakangas
>  EnterpriseDB   http://www.enterprisedb.com

2012/4/4 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> On 30.03.2012 12:36, Pavel Stehule wrote:
>>
>> 2012/3/28 Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com>:
>>>
>>> In prepare_expr(), you use a subtransaction to catch any ERRORs that
>>> happen
>>>
>>> during parsing the expression. That's a good idea, and I think many of
>>> the
>>> check_* functions could be greatly simplified by adopting a similar
>>> approach. Just ereport() any errors you find, and catch them at the
>>> appropriate level, appending the error to the output string. Your current
>>> approach of returning true/false depending on whether there was any
>>> errors
>>> seems tedious.
>>
>>
>> It cannot be implemented in AST interpret. Without removing some
>> requested functionality - fatal_errors.
>
>
> I don't think I'm getting my point across by explaining, so here's a
> modified version of the patch that does what I was trying to say. The
> general pattern of the checker functions has been changed. Instead of
> returning a boolean indicating error or no error, with checks for
> fatal_errors scattered around them, the internal checker functions now
> return nothing. Any errors are reported with ereport(), and there is a
> PG_TRY/CATCH block in a couple of carefully chosen places: in check_stmt(),
> so that if you get an error while checking a statement, you continue
> checking on the next statement, and in check_assignment() which is now used
> by check_expr() and a few other helper functions to basically check all
> expressions and SQL statements.
>
> IMHO this makes the code much more readable, now that the control logic of
> when to return and when to continue is largely gone. A lot of other cleanup
> still needs to be done, I just wanted to demonstrate this ereport+try/catch
> idea with this patch.

I checked your idea and it should to work.

What other cleanup (without mentioned in previous mails) do you think?

Regards

Pavel

>
>
> --
>  Heikki Linnakangas
>  EnterpriseDB   http://www.enterprisedb.com

Hello

2012/4/4 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> On 04.04.2012 19:32, Tom Lane wrote:
>>
>> Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com>  writes:
>>>
>>> I don't think I'm getting my point across by explaining, so here's a
>>> modified version of the patch that does what I was trying to say.
>>
>>
>> Minor side point: some of the diff noise in this patch comes from
>> s/copy_plpgsql_datum/plpgsql_copy_plpgsql_datum/, which seems entirely
>> useless.  The name already contains "plpgsql", and even if it didn't,
>> there is no particular reason for plpgsql to worry about polluting
>> global symbol namespace.  Nothing else resolves against its symbols
>> anyway, at least not on any platform we claim to support.  I would
>> therefore also argue against the other renamings like
>> s/exec_move_row/plpgsql_exec_move_row/.
>
>
> Agreed. Looking closer, I'm not sure we even need to expose exec_move_row()
> to pl_check.c. It's only used to initialize row-type function arguments to
> NULL. But variables that are not explicitly initialized are NULL anyway, and
> the checker shouldn't use the values stored in variables for anything, so I
> believe that initialization in function_check() can be replaced with
> something much simpler or removed altogether.
>

I returned back original names and removed exec_move_row from pl_check.c

This is little bit cleaned Heikki version

Regards

Pavel

>
> --
>  Heikki Linnakangas
>  EnterpriseDB   http://www.enterprisedb.com