Re: [HACKERS] poll: CHECK TRIGGER?
Hello 2012/4/4 Heikki Linnakangas heikki.linnakan...@enterprisedb.com: On 04.04.2012 19:32, Tom Lane wrote: Heikki Linnakangasheikki.linnakan...@enterprisedb.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 plpgsql_check_function-2012-04-05-1.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Heikki Linnakangas heikki.linnakan...@enterprisedb.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
On 04.04.2012 19:32, Tom Lane wrote: Heikki Linnakangasheikki.linnakan...@enterprisedb.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/4/4 Heikki Linnakangas heikki.linnakan...@enterprisedb.com: On 04.04.2012 19:32, Tom Lane wrote: Heikki Linnakangasheikki.linnakan...@enterprisedb.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/4/4 Heikki Linnakangas heikki.linnakan...@enterprisedb.com: On 30.03.2012 12:36, Pavel Stehule wrote: 2012/3/28 Heikki Linnakangasheikki.linnakan...@enterprisedb.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Hello
2012/3/28 Heikki Linnakangas heikki.linnakan...@enterprisedb.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
plpgsql_check_function2-2012-03-30.diff.gz
Description: GNU Zip compressed data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
On 28.03.2012 23:54, Pavel Stehule wrote: 2012/3/28 Heikki Linnakangasheikki.linnakan...@enterprisedb.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/29 Heikki Linnakangas heikki.linnakan...@enterprisedb.com: On 28.03.2012 23:54, Pavel Stehule wrote: 2012/3/28 Heikki Linnakangasheikki.linnakan...@enterprisedb.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Hello 2012/3/28 Heikki Linnakangas heikki.linnakan...@enterprisedb.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/11 Petr Jelinek pjmo...@pjmodos.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 plpgsql_check_function2-2012-03-12.diff.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
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 plpgsql_check_function2.diff.gz Description: application/gzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/11 Petr Jelinek pjmo...@pjmodos.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Hello 2012/3/10 Tom Lane t...@sss.pgh.pa.us: Peter Eisentraut pete...@gmx.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 plpgsql_check_function.diff.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
here is draft and there some cleaned version Regards Pavel Stehule regards, tom lane plpgsql_check_function.diff.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
On tor, 2012-03-08 at 19:19 -0500, Robert Haas wrote: On Thu, Mar 8, 2012 at 4:54 PM, Peter Eisentraut pete...@gmx.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) 21 | 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
On Fri, Mar 9, 2012 at 3:15 PM, Peter Eisentraut pete...@gmx.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 pete...@gmx.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) 21 | 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Robert Haas robertmh...@gmail.com writes: On Fri, Mar 9, 2012 at 3:15 PM, Peter Eisentraut pete...@gmx.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) 21 | 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/9 Peter Eisentraut pete...@gmx.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Pavel Stehule pavel.steh...@gmail.com writes: 2012/3/9 Peter Eisentraut pete...@gmx.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
On Fri, Mar 9, 2012 at 3:33 PM, Tom Lane t...@sss.pgh.pa.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/9 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: 2012/3/9 Peter Eisentraut pete...@gmx.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
On Fri, Mar 9, 2012 at 5:09 PM, Pavel Stehule pavel.steh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/9 Robert Haas robertmh...@gmail.com: On Fri, Mar 9, 2012 at 5:09 PM, Pavel Stehule pavel.steh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
On Fri, Mar 9, 2012 at 5:31 PM, Pavel Stehule pavel.steh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Peter Eisentraut pete...@gmx.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/10 Tom Lane t...@sss.pgh.pa.us: Peter Eisentraut pete...@gmx.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
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
reduced_pl_checker_2012-03-08_3.patch.gz
Description: application/gzip
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/8 Peter Eisentraut pete...@gmx.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/8 Peter Eisentraut pete...@gmx.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
On Thu, Mar 8, 2012 at 4:54 PM, Peter Eisentraut pete...@gmx.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
On Wed, Mar 7, 2012 at 12:17 AM, Pavel Stehule pavel.steh...@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Robert Haas robertmh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/7 Robert Haas robertmh...@gmail.com:
On Wed, Mar 7, 2012 at 12:17 AM, Pavel Stehule pavel.steh...@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your
Re: [HACKERS] poll: CHECK TRIGGER?
On Wed, Mar 7, 2012 at 12:04 PM, Tom Lane t...@sss.pgh.pa.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/7 Tom Lane t...@sss.pgh.pa.us: Robert Haas robertmh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Robert Haas robertmh...@gmail.com writes: On Wed, Mar 7, 2012 at 12:04 PM, Tom Lane t...@sss.pgh.pa.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/7 Tom Lane t...@sss.pgh.pa.us: Robert Haas robertmh...@gmail.com writes: On Wed, Mar 7, 2012 at 12:04 PM, Tom Lane t...@sss.pgh.pa.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
On Wed, Mar 7, 2012 at 2:03 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Wed, Mar 7, 2012 at 12:04 PM, Tom Lane t...@sss.pgh.pa.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/7 Robert Haas robertmh...@gmail.com: On Wed, Mar 7, 2012 at 2:03 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Wed, Mar 7, 2012 at 12:04 PM, Tom Lane t...@sss.pgh.pa.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Robert Haas robertmh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Hello
2012/3/7 Tom Lane t...@sss.pgh.pa.us:
Robert Haas robertmh...@gmail.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
reduced_pl_checker_2012-03-07.patch.gz
Description: GNU Zip compressed data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Hello
2012/3/7 Tom Lane t...@sss.pgh.pa.us:
Robert Haas robertmh...@gmail.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
reduced_pl_checker_2012-03-08_1.patch.gz
Description: GNU Zip compressed data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
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.steh...@gmail.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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
On Sat, Mar 3, 2012 at 9:23 PM, Alvaro Herrera alvhe...@commandprompt.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/5 Robert Haas robertmh...@gmail.com: On Sat, Mar 3, 2012 at 9:23 PM, Alvaro Herrera alvhe...@commandprompt.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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Robert Haas robertmh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
On Mon, Mar 5, 2012 at 11:41 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/5 Robert Haas robertmh...@gmail.com: On Mon, Mar 5, 2012 at 11:41 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
2012/3/5 Robert Haas robertmh...@gmail.com: On Sat, Mar 3, 2012 at 9:23 PM, Alvaro Herrera alvhe...@commandprompt.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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 alvhe...@alvh.no-ip.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Alvaro Herrera alvhe...@alvh.no-ip.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] poll: CHECK TRIGGER?
Excerpts from Tom Lane's message of sáb mar 03 23:00:26 -0300 2012: Alvaro Herrera alvhe...@alvh.no-ip.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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
