Re: [HACKERS] review: CHECK FUNCTION statement
Hello multicheck for triggers are supported now CHECK TRIGGER ALL; CHECK TRIGGER ALL IN SCHEMA xxx FOR ROLE yyy; Regards Pavel Stehule check_function-2012-03-07-2.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] review: CHECK FUNCTION statement
Excerpts from Pavel Stehule's message of mar mar 06 03:43:06 -0300 2012: Hello * I refreshed regress tests and appended tests for multi lines query * There are enhanced checking of SELECT INTO statement * I fixed showing details and hints Oh, I forgot to remove the do_tup_output_slot() function which I added in some previous version but is no longer necessary. Also, there are two includes that I put separately in functioncmds.c that are only necessary for the CHECK FUNCTION stuff; those should be merged in with the other includes there. (I was toying with the idea of moving all that code to a new file). -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
2012/3/6 Alvaro Herrera alvhe...@commandprompt.com: Excerpts from Pavel Stehule's message of mar mar 06 10:44:09 -0300 2012: 2012/3/6 Alvaro Herrera alvhe...@commandprompt.com: Excerpts from Pavel Stehule's message of mar mar 06 03:43:06 -0300 2012: Hello * I refreshed regress tests and appended tests for multi lines query * There are enhanced checking of SELECT INTO statement * I fixed showing details and hints Oh, I forgot to remove the do_tup_output_slot() function which I added in some previous version but is no longer necessary. Also, there are two includes that I put separately in functioncmds.c that are only necessary for the CHECK FUNCTION stuff; those should be merged in with the other includes there. (I was toying with the idea of moving all that code to a new file). I am not sure, what you did do. Can you remove this useless code, please? It's just a three line function that's not called anywhere. ok fixed patch Pavel -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support check_function-2012-03-06-2.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] review: CHECK FUNCTION statement
Hello there is new version * fixed small formatting issues related to drop SPI call * long functions was divided * CREATE TRIGGER ALL ON table support Regards Pavel check_function-2012-03-06-3.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] review: CHECK FUNCTION statement
Hello I found one small issue where Query was not forwarded when trigger record was broken. I had to append context forwarding. Regards Pavel 2012/3/6 Pavel Stehule pavel.steh...@gmail.com: Hello there is new version * fixed small formatting issues related to drop SPI call * long functions was divided * CREATE TRIGGER ALL ON table support Regards Pavel check_function-2012-03-07-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] review: CHECK FUNCTION statement
Hello
2012/3/5 Alvaro Herrera alvhe...@commandprompt.com:
Excerpts from Pavel Stehule's message of dom mar 04 16:33:08 -0300 2012:
Hello
2012/3/4 Alvaro Herrera alvhe...@commandprompt.com:
CHECK FUNCTION
-
In function: 'f()'
error:42P01:2:sentencia SQL:no existe la relación «foo»
query:select +
var +
from +
foo
^
(4 filas)
this should be fixed. I checked expressions, that works (I expect)
correctly. Caret helps - (really). Sometimes man is blind :).
Agreed.
I don't have your last version, so I am sending just part of
CheckFunctionById function - this fragment ensures a output
or please, send me your last patch and I'll do merge
now result is better
postgres= create function f() returns int language plpgsql as $$
postgres$ begin select
postgres$ var
postgres$ from
postgres$ foo; end; $$;
CREATE FUNCTION
postgres= check function f();
CHECK FUNCTION
---
In function: f()
error:42P01:2:SQL statement:relation foo does not exist
query:select
var
from
foo
^
(7 rows)
and some utf8 fce
postgres= check function fx(int);
CHECK FUNCTION
--
In function: fx(integer)
error:42703:3:RETURN:column ýšý does not exist
query:SELECT (select žlutý
from jj
/* ýšý */
where /*ýšýšý8*/ ýšý = 10)
^
(7 rows)
postgres= check function fx(int);
CHECK FUNCTION
-
In function: fx(integer)
error:42703:3:RETURN:column xx does not exist
query:SELECT (select t.a
from t
/* ýšý */
where /*ýšýšý8*/ xx = 10)
^
(7 rows)
caret is ok
regards
Pavel
--
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
else
{
resetStringInfo(sinfo);
appendStringInfo(sinfo, In function: %s, funcname);
do_text_output_oneline(tstate, sinfo.data);
for (i = 0; i SPI_processed; i++)
{
char *query;
resetStringInfo(sinfo);
appendStringInfo(sinfo, %s:%s:%s:%s:%s,
SPI_getvalue(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 8),
SPI_getvalue(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 4),
SPI_getvalue(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 2),
SPI_getvalue(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 3),
SPI_getvalue(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 5));
do_text_output_oneline(tstate, sinfo.data);
resetStringInfo(sinfo);
query = SPI_getvalue(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 10);
if (query != NULL)
{
bool isnull;
char *query_line; /* pointer to begin of processed line */
int query_line_caret;
int caret;
bool is_first_line = true;
/*
* put any query line to separate output line. And append
* a curet, when is defined and related to processed rows.
*/
caret = SPI_getbinval(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 9, isnull);
if (isnull)
caret = -1;
query_line = query;
query_line_caret = caret;
while (*query != '\0')
{
int len;
if (*query == '\n')
{
/* now we found end of line */
*query = '\0';
if (is_first_line)
{
appendStringInfo(sinfo, query:%s, query_line);
is_first_line = false;
}
else
appendStringInfo(sinfo, %s, query_line);
do_text_output_oneline(tstate, sinfo.data);
resetStringInfo(sinfo);
if (query_line_caret 0 caret == 0)
{
appendStringInfo(sinfo, %*s,
query_line_caret, ^);
do_text_output_oneline(tstate, sinfo.data);
resetStringInfo(sinfo);
query_line_caret = 0;
}
/* store caret offset for next line */
if (caret 0)
query_line_caret = caret - 1;
/* go to next line */
query_line = query + 1;
}
len = pg_mblen(query);
query += len;
if (caret 0)
caret--;
}
/* last line output */
if (query_line != NULL)
{
if (is_first_line)
{
appendStringInfo(sinfo, query:%s, query_line);
}
else
appendStringInfo(sinfo, %s, query_line);
do_text_output_oneline(tstate, sinfo.data);
resetStringInfo(sinfo);
if (query_line_caret 0 caret == 0)
{
appendStringInfo(sinfo, %*s,
query_line_caret, ^);
do_text_output_oneline(tstate, sinfo.data);
resetStringInfo(sinfo);
}
}
}
}
}
--
Re: [HACKERS] review: CHECK FUNCTION statement
small fix of CheckFunctionById function
Regards
p.s. Alvaro, please, send your patch and I'll merge it
/*
* Connect to SPI manager
*/
if (SPI_connect() != SPI_OK_CONNECT)
elog(ERROR, SPI_connect failed);
values[0] = ObjectIdGetDatum(funcOid);
values[1] = ObjectIdGetDatum(relid);
values[2] = PointerGetDatum(options);
values[3] = BoolGetDatum(fatal_errors);
SPI_execute_with_args(sinfo.data,
4, argtypes,
values, nulls,
true, 0);
result = SPI_processed == 0;
if (result)
{
resetStringInfo(sinfo);
appendStringInfo(sinfo, Function is valid: '%s', funcname);
do_text_output_oneline(tstate, sinfo.data);
}
else
{
resetStringInfo(sinfo);
appendStringInfo(sinfo, In function: %s, funcname);
do_text_output_oneline(tstate, sinfo.data);
for (i = 0; i SPI_processed; i++)
{
char *query;
resetStringInfo(sinfo);
appendStringInfo(sinfo, %s:%s:%s:%s:%s,
SPI_getvalue(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 8),
SPI_getvalue(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 4),
SPI_getvalue(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 2),
SPI_getvalue(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 3),
SPI_getvalue(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 5));
do_text_output_oneline(tstate, sinfo.data);
resetStringInfo(sinfo);
query = SPI_getvalue(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 10);
if (query != NULL)
{
bool isnull;
char *query_line; /* pointer to begin of processed line */
int query_line_caret;
int caret;
bool is_first_line = true;
/*
* put any query line to separate output line. And append
* a curet, when is defined and related to processed rows.
*/
caret = SPI_getbinval(SPI_tuptable-vals[i], SPI_tuptable-tupdesc, 9, isnull);
if (isnull)
caret = -1;
else
caret;
query_line = query;
query_line_caret = caret;
while (*query != '\0')
{
int len;
if (*query == '\n')
{
/* now we found end of line */
*query = '\0';
if (is_first_line)
{
appendStringInfo(sinfo, query:%s, query_line);
is_first_line = false;
}
else
appendStringInfo(sinfo, %s, query_line);
do_text_output_oneline(tstate, sinfo.data);
resetStringInfo(sinfo);
if (query_line_caret 0 caret == 0)
{
appendStringInfo(sinfo, --%*s,
query_line_caret, ^);
do_text_output_oneline(tstate, sinfo.data);
resetStringInfo(sinfo);
query_line_caret = 0;
}
/* store caret offset for next line */
if (caret 1)
query_line_caret = caret - 1;
/* go to next line */
query_line = query + 1;
}
len = pg_mblen(query);
query += len;
if (caret 0)
caret--;
}
/* last line output */
if (query_line != NULL)
{
if (is_first_line)
{
appendStringInfo(sinfo, query:%s, query_line);
}
else
appendStringInfo(sinfo, %s, query_line);
do_text_output_oneline(tstate, sinfo.data);
resetStringInfo(sinfo);
if (query_line_caret 0 caret == 0)
{
appendStringInfo(sinfo, --%*s,
query_line_caret, ^);
do_text_output_oneline(tstate, sinfo.data);
resetStringInfo(sinfo);
}
}
}
}
}
--
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] review: CHECK FUNCTION statement
Excerpts from Pavel Stehule's message of lun mar 05 13:02:50 -0300 2012: small fix of CheckFunctionById function Regards p.s. Alvaro, please, send your patch and I'll merge it Here it is, with your changes already merged. I also added back the new reference doc files which were dropped after the 2012-01-01 version. Note I haven't touched or read the plpgsql checker code at all (only some automatic indentation changes IIRC). I haven't verified the regression tests either. FWIW I'm not going to participate in the other thread; neither I am going to work any more on this patch until the other thread sees some reasonable conclusion. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support check_function-2012-03-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] review: CHECK FUNCTION statement
Hello * I refreshed regress tests and appended tests for multi lines query * There are enhanced checking of SELECT INTO statement * I fixed showing details and hints Regards Pavel Stehule 2012/3/5 Alvaro Herrera alvhe...@commandprompt.com: Excerpts from Pavel Stehule's message of lun mar 05 13:02:50 -0300 2012: small fix of CheckFunctionById function Regards p.s. Alvaro, please, send your patch and I'll merge it Here it is, with your changes already merged. I also added back the new reference doc files which were dropped after the 2012-01-01 version. Note I haven't touched or read the plpgsql checker code at all (only some automatic indentation changes IIRC). I haven't verified the regression tests either. FWIW I'm not going to participate in the other thread; neither I am going to work any more on this patch until the other thread sees some reasonable conclusion. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support check_function-2012-03-06-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] review: CHECK FUNCTION statement
Hello 2012/3/4 Alvaro Herrera alvhe...@commandprompt.com: Excerpts from Alvaro Herrera's message of sáb mar 03 17:56:23 -0300 2012: Excerpts from Alvaro Herrera's message of sáb mar 03 16:54:19 -0300 2012: Excerpts from Pavel Stehule's message of sáb mar 03 02:25:52 -0300 2012: 3. THE ARE NOT CARET - this is really important I am not sure about the caret thingy -- mainly because I don't think it works all that well. It doesn't work correctly with your patch; see sample below. Note the caret is pointing to an entirely nonsensical position. I'm not sure about duplicating the libpq line-counting logic in the backend. Also note i18n seems to be working well, except for the In function header, query, and the error level. That seems easily fixable. I remain unconvinced that this is the best possible output. alvherre=# create function f() returns int language plpgsql as $$ begin select var from foo; end; $$; CREATE FUNCTION alvherre=# check function f(); CHECK FUNCTION - In function: 'f()' error:42P01:2:sentencia SQL:no existe la relación «foo» query:select + var + from + foo ^ (4 filas) this should be fixed. I checked expressions, that works (I expect) correctly. Caret helps - (really). Sometimes man is blind :). I'll look on this topic tomorrow and I hope this will be solvable simply. Regards Pavel -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Excerpts from Pavel Stehule's message of dom mar 04 16:33:08 -0300 2012: Hello 2012/3/4 Alvaro Herrera alvhe...@commandprompt.com: CHECK FUNCTION - In function: 'f()' error:42P01:2:sentencia SQL:no existe la relación «foo» query:select + var + from + foo ^ (4 filas) this should be fixed. I checked expressions, that works (I expect) correctly. Caret helps - (really). Sometimes man is blind :). Agreed. -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
On 03/03/2012 02:24 AM, Alvaro Herrera wrote: question: how attached you are to the current return format for CHECK FUNCTION? check function f1(); CHECK FUNCTION - In function: 'f1()' error:42804:5:assignment:subscripted object is not an array (2 rows) It seems to me that it'd be trivial to make it look like this instead: check function f1(); function | lineno | statement | sqlstate | message | detail | hint | level | position | query -+++--+++--+---+--+--- f1() | 5 | assignment | 42804| subscripted object is not an array || | error | | (1 row) This looks much nicer to me. One thing we lose is the caret marking the position of the error -- but I'm wondering if that really works well. I didn't test it but from the code it looks to me like it'd misbehave if you had a multiline statement. Opinions? Well, if you want nicely formated table you can always call the checker function directly, I think the statement returning something that is more human and less machine is more consistent approach with the rest of the utility commands. In other words I don't really see the point of it. Petr -- 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] review: CHECK FUNCTION statement
Excerpts from Petr Jelínek's message of sáb mar 03 10:26:04 -0300 2012: On 03/03/2012 02:24 AM, Alvaro Herrera wrote: question: how attached you are to the current return format for CHECK FUNCTION? check function f1(); CHECK FUNCTION - In function: 'f1()' error:42804:5:assignment:subscripted object is not an array (2 rows) Well, if you want nicely formated table you can always call the checker function directly, I think the statement returning something that is more human and less machine is more consistent approach with the rest of the utility commands. In other words I don't really see the point of it. I am not against having some more human readable output than plain tabular. In particular the idea that we need to have all fields is of course open to discussion. But is the output as proposed above really all that human friendly? I disagree that it cannot be improved. BTW one thing that's missing in this feature so far is some translatability of the returned output. -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Excerpts from Pavel Stehule's message of sáb mar 03 02:25:52 -0300 2012: Hello It wasn't all that difficult -- see below. While at this, I have a question: how attached you are to the current return format for CHECK FUNCTION? TupleDescr is created by language creator. This ensure exactly expected format, because there are no possible registry check function with other output tuple descriptor. I'm not sure what you're saying. What TupDesc are you talking about? The tupdesc returned by the checker is certainly hardcoded by the core support; the language creator cannot deviate from it. check function f1(); function | lineno | statement | sqlstate | message | detail | hint | level | position | query -+++--+++--+---+--+--- f1() | 5 | assignment | 42804 | subscripted object is not an array | | | error | | (1 row) This looks much nicer to me. I am strongly disagree. 1. This format is not consistent with other commands, 2. This format is difficult for copy/paste 3. THE ARE NOT CARET - this is really important 5. This form is bad for terminals - there are long rows, and with \x outout, there are lot of garbage for multicommand 4. When you would to this form, you can to directly call SRF PL check functions. I am not sure that consistency is the most important thing here; I think what we care about is that it's usable. So yeah, it might be hard to cut and paste, and also too wide. Maybe we can run some of the fields together, and omit others. I am not sure about the caret thingy -- mainly because I don't think it works all that well. I don't know how psql does it, but I notice that it shows a single line in a multiline query -- so it's not just a matter of adding some number of spaces. Given the negative feedback, I'm going to leave this output format unchanged; we can tweak it later. -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Excerpts from Alvaro Herrera's message of sáb mar 03 16:54:19 -0300 2012: Excerpts from Pavel Stehule's message of sáb mar 03 02:25:52 -0300 2012: 3. THE ARE NOT CARET - this is really important I am not sure about the caret thingy -- mainly because I don't think it works all that well. I don't know how psql does it, but I notice that it shows a single line in a multiline query -- so it's not just a matter of adding some number of spaces. I checked how this works in psql. It is upwards of 200 lines of code -- see reportErrorPosition in libpq/fe-protocol3.c. I'm not sure this can be made to work sensibly here. -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Excerpts from Alvaro Herrera's message of sáb mar 03 17:56:23 -0300 2012: Excerpts from Alvaro Herrera's message of sáb mar 03 16:54:19 -0300 2012: Excerpts from Pavel Stehule's message of sáb mar 03 02:25:52 -0300 2012: 3. THE ARE NOT CARET - this is really important I am not sure about the caret thingy -- mainly because I don't think it works all that well. It doesn't work correctly with your patch; see sample below. Note the caret is pointing to an entirely nonsensical position. I'm not sure about duplicating the libpq line-counting logic in the backend. Also note i18n seems to be working well, except for the In function header, query, and the error level. That seems easily fixable. I remain unconvinced that this is the best possible output. alvherre=# create function f() returns int language plpgsql as $$ begin select var from foo; end; $$; CREATE FUNCTION alvherre=# check function f(); CHECK FUNCTION - In function: 'f()' error:42P01:2:sentencia SQL:no existe la relación «foo» query:select + var+ from + foo ^ (4 filas) -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Hello 2012/3/1 Alvaro Herrera alvhe...@commandprompt.com: Why does CollectCheckedFunctions skip trigger functions? My only guess is that at one point the checker was not supposed to know how to check them, and a later version learned about it and this bit wasn't updated; but maybe there's another reason? you cannot to check trigger function without assigned relation - TupleDescription should be assigned to NEW and OLD variables. Regards Pavel -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
2012/3/2 Alvaro Herrera alvhe...@commandprompt.com: I've cleaned up the backend code a bit -- see attached. More yet to go through; I'm mainly sending it out for you (and everyone, really) to give your opinion on my changes so far. (I split out the plpgsql checker for the time being into a separate branch; I'll get on it after I get this part committed.) it looks well Regards Pavel Stěhule -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Excerpts from Pavel Stehule's message of vie mar 02 05:29:26 -0300 2012: you cannot to check trigger function without assigned relation - TupleDescription should be assigned to NEW and OLD variables. Oh, I see, that makes sense. After mulling over this a bit, I'm dubious about having two separate commands, one which checks triggers and another that checks non-trigger functions. Wouldn't it make more sense to have some options into CHECK FUNCTION so that it receives the trigger and corresponding relation name to check? For example check function foo() trigger on tab or something like that? I also wonder if it would make sense to have grammar for check all triggers on table xyz or some such, and even check all triggers on all functions. Another thing is that CHECK FUNCTION ALL FOR ROLE foo seems a bit strange to me. What about CHECK FUNCTION ALL OWNED BY foo instead? (CHECK FUNCTION ALL seems strange as a whole, but I'm not sure that we can improve that ... still, if anyone has ideas I'm sure we can discuss) As a reminder: we also have CHECK FUNCTION ALL IN SCHEMA f and CHECK FUNCTION ALL IN LANGUAGE f (and combinations thereof) Thoughts? -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Hello 2012/3/2 Alvaro Herrera alvhe...@commandprompt.com: Excerpts from Pavel Stehule's message of vie mar 02 05:29:26 -0300 2012: you cannot to check trigger function without assigned relation - TupleDescription should be assigned to NEW and OLD variables. Oh, I see, that makes sense. After mulling over this a bit, I'm dubious about having two separate commands, one which checks triggers and another that checks non-trigger functions. Wouldn't it make more sense to have some options into CHECK FUNCTION so that it receives the trigger and corresponding relation name to check? For example check function foo() trigger on tab or something like that? I don't like it - check trigger is simple - and consistent I also wonder if it would make sense to have grammar for check all triggers on table xyz or some such, and even check all triggers on all functions. this is good idea - and I like it CHECK TRIGGER ALL ON TABLE X there are possible some combination like CHECK TRIGGER ALL IN SCHEMA ...; and similar. But I am not sure, if this is necessary - for some more complex usage developer can use a direct PL check function. Another thing is that CHECK FUNCTION ALL FOR ROLE foo seems a bit strange to me. What about CHECK FUNCTION ALL OWNED BY foo instead? (CHECK FUNCTION ALL seems strange as a whole, but I'm not sure that we can improve that ... still, if anyone has ideas I'm sure we can discuss) this should not be nice from language view, but it doesn't need new keywords pattern FOR ROLE, IN SCHEMA are used ALTER DEFAULT PRIVILEGES FOR ROLE but OWNED BY is used too (SeqOptElem:) I agree so OWNED BY sounds better (can be changed) As a reminder: we also have CHECK FUNCTION ALL IN SCHEMA f it is ok and CHECK FUNCTION ALL IN LANGUAGE f (and combinations thereof) it is ok too Regards Pavel Thoughts? -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Excerpts from Pavel Stehule's message of mar feb 28 16:30:58 -0300 2012:
Hello
Dne 28. února 2012 17:48 Alvaro Herrera alvhe...@commandprompt.com
napsal(a):
I have a few comments about this patch:
I didn't like the fact that the checker calling infrastructure uses
SPI instead of just a FunctionCallN to call the checker function. I
think this should be easily avoidable.
It is not possible - or it has not simple solution (I don't how to do
it). PLpgSQL_checker is SRF function. SPI is used for processing
returned resultset. I looked to pg source code, and I didn't find any
other pattern than using SPI for SRF function call. It is probably
possible, but it means some code duplication too. I invite any ideas.
It wasn't all that difficult -- see below. While at this, I have a
question: how attached you are to the current return format for CHECK
FUNCTION?
check function f1();
CHECK FUNCTION
-
In function: 'f1()'
error:42804:5:assignment:subscripted object is not an array
(2 rows)
It seems to me that it'd be trivial to make it look like this instead:
check function f1();
function | lineno | statement | sqlstate | message
| detail | hint | level | position | query
-+++--+++--+---+--+---
f1() | 5 | assignment | 42804| subscripted object is not an array
|| | error | |
(1 row)
This looks much nicer to me.
One thing we lose is the caret marking the position of the error -- but
I'm wondering if that really works well. I didn't test it but from the
code it looks to me like it'd misbehave if you had a multiline statement.
Opinions?
/*
* Search and execute the checker function.
*
* returns true, when checked function is valid
*/
static bool
CheckFunctionById(Oid funcOid, Oid relid, ArrayType *options,
bool fatal_errors, TupOutputState *tstate)
{
HeapTuple tup;
Form_pg_procproc;
HeapTuple languageTuple;
Form_pg_language lanForm;
Oid languageChecker;
char *funcname;
int result;
tup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcOid));
if (!HeapTupleIsValid(tup)) /* should not happen */
elog(ERROR, cache lookup failed for function %u, funcOid);
proc = (Form_pg_proc) GETSTRUCT(tup);
languageTuple = SearchSysCache1(LANGOID,
ObjectIdGetDatum(proc-prolang));
Assert(HeapTupleIsValid(languageTuple));
lanForm = (Form_pg_language) GETSTRUCT(languageTuple);
languageChecker = lanForm-lanchecker;
funcname = format_procedure(funcOid);
/* We're all set to call the checker */
if (OidIsValid(languageChecker))
{
TupleDesc tupdesc;
Datum checkret;
FmgrInfoflinfo;
ReturnSetInfo rsinfo;
FunctionCallInfoData fcinfo;
/* create the tuple descriptor that the checker is supposed to
return */
tupdesc = CreateTemplateTupleDesc(10, false);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, functionid,
REGPROCOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, lineno, INT4OID,
-1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 3, statement,
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 4, sqlstate,
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, message, TEXTOID,
-1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 6, detail, TEXTOID,
-1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 7, hint, TEXTOID,
-1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 8, level, TEXTOID,
-1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 9, position,
INT4OID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 10, query, TEXTOID,
-1, 0);
fmgr_info(languageChecker, flinfo);
rsinfo.type = T_ReturnSetInfo;
rsinfo.econtext = CreateStandaloneExprContext();
rsinfo.expectedDesc = tupdesc;
rsinfo.allowedModes = (int) SFRM_Materialize;
/* returnMode is set by the checker, hopefully ... */
/* isDone is not relevant, since not using ValuePerCall */
rsinfo.setResult = NULL;
rsinfo.setDesc = NULL;
InitFunctionCallInfoData(fcinfo, flinfo, 4, InvalidOid, NULL,
(Node *) rsinfo);
fcinfo.arg[0] = ObjectIdGetDatum(funcOid);
Re: [HACKERS] review: CHECK FUNCTION statement
Hello
It wasn't all that difficult -- see below. While at this, I have a
question: how attached you are to the current return format for CHECK
FUNCTION?
TupleDescr is created by language creator. This ensure exactly
expected format, because there are no possible registry check function
with other output tuple descriptor.
check function f1();
CHECK FUNCTION
-
In function: 'f1()'
error:42804:5:assignment:subscripted object is not an array
(2 rows)
It seems to me that it'd be trivial to make it look like this instead:
check function f1();
function | lineno | statement | sqlstate | message
| detail | hint | level | position | query
-+++--+++--+---+--+---
f1() | 5 | assignment | 42804 | subscripted object is not an
array | | | error | |
(1 row)
This looks much nicer to me.
I am strongly disagree.
1. This format is not consistent with other commands,
2. This format is difficult for copy/paste
3. THE ARE NOT CARET - this is really important
5. This form is bad for terminals - there are long rows, and with \x
outout, there are lot of garbage for multicommand
4. When you would to this form, you can to directly call SRF PL check
functions.
One thing we lose is the caret marking the position of the error -- but
I'm wondering if that really works well. I didn't test it but from the
code it looks to me like it'd misbehave if you had a multiline statement.
Opinions?
-1
/*
* Search and execute the checker function.
*
* returns true, when checked function is valid
*/
static bool
CheckFunctionById(Oid funcOid, Oid relid, ArrayType *options,
bool fatal_errors, TupOutputState *tstate)
{
HeapTuple tup;
Form_pg_proc proc;
HeapTuple languageTuple;
Form_pg_language lanForm;
Oid languageChecker;
char *funcname;
int result;
tup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcOid));
if (!HeapTupleIsValid(tup)) /* should not happen */
elog(ERROR, cache lookup failed for function %u, funcOid);
proc = (Form_pg_proc) GETSTRUCT(tup);
languageTuple = SearchSysCache1(LANGOID,
ObjectIdGetDatum(proc-prolang));
Assert(HeapTupleIsValid(languageTuple));
lanForm = (Form_pg_language) GETSTRUCT(languageTuple);
languageChecker = lanForm-lanchecker;
funcname = format_procedure(funcOid);
/* We're all set to call the checker */
if (OidIsValid(languageChecker))
{
TupleDesc tupdesc;
Datum checkret;
FmgrInfo flinfo;
ReturnSetInfo rsinfo;
FunctionCallInfoData fcinfo;
/* create the tuple descriptor that the checker is supposed to
return */
tupdesc = CreateTemplateTupleDesc(10, false);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, functionid,
REGPROCOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, lineno, INT4OID,
-1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 3, statement,
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 4, sqlstate,
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, message,
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 6, detail, TEXTOID,
-1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 7, hint, TEXTOID,
-1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 8, level, TEXTOID,
-1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 9, position,
INT4OID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 10, query, TEXTOID,
-1, 0);
fmgr_info(languageChecker, flinfo);
rsinfo.type = T_ReturnSetInfo;
rsinfo.econtext = CreateStandaloneExprContext();
rsinfo.expectedDesc = tupdesc;
rsinfo.allowedModes = (int) SFRM_Materialize;
/* returnMode is set by the checker, hopefully ... */
/* isDone is not relevant, since not using ValuePerCall */
rsinfo.setResult = NULL;
rsinfo.setDesc = NULL;
InitFunctionCallInfoData(fcinfo, flinfo, 4, InvalidOid, NULL,
(Node *) rsinfo);
fcinfo.arg[0] = ObjectIdGetDatum(funcOid);
fcinfo.arg[1] = ObjectIdGetDatum(relid);
fcinfo.arg[2] = PointerGetDatum(options);
fcinfo.arg[3] = BoolGetDatum(fatal_errors);
fcinfo.argnull[0]
Re: [HACKERS] review: CHECK FUNCTION statement
2012/3/3 Pavel Stehule pavel.steh...@gmail.com:
Hello
It wasn't all that difficult -- see below. While at this, I have a
question: how attached you are to the current return format for CHECK
FUNCTION?
TupleDescr is created by language creator. This ensure exactly
expected format, because there are no possible registry check function
with other output tuple descriptor.
check function f1();
CHECK FUNCTION
-
In function: 'f1()'
error:42804:5:assignment:subscripted object is not an array
(2 rows)
It seems to me that it'd be trivial to make it look like this instead:
check function f1();
function | lineno | statement | sqlstate | message
| detail | hint | level | position | query
-+++--+++--+---+--+---
f1() | 5 | assignment | 42804 | subscripted object is not an
array | | | error | |
(1 row)
This looks much nicer to me.
This was similar to first design - it is near to result of direct PL
check function call. But Tom and Albe had different opinion - check a
thread three months ago, and I had to agree with them - they proposed
better behave for using in psql console - and result is more similar
to usual output when exception is raised.
I am strongly disagree.
1. This format is not consistent with other commands,
2. This format is difficult for copy/paste
3. THE ARE NOT CARET - this is really important
5. This form is bad for terminals - there are long rows, and with \x
outout, there are lot of garbage for multicommand
4. When you would to this form, you can to directly call SRF PL check
functions.
One thing we lose is the caret marking the position of the error -- but
I'm wondering if that really works well. I didn't test it but from the
code it looks to me like it'd misbehave if you had a multiline statement.
Opinions?
/*
* Search and execute the checker function.
*
* returns true, when checked function is valid
*/
static bool
CheckFunctionById(Oid funcOid, Oid relid, ArrayType *options,
bool fatal_errors, TupOutputState *tstate)
{
HeapTuple tup;
Form_pg_proc proc;
HeapTuple languageTuple;
Form_pg_language lanForm;
Oid languageChecker;
char *funcname;
int result;
tup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcOid));
if (!HeapTupleIsValid(tup)) /* should not happen */
elog(ERROR, cache lookup failed for function %u, funcOid);
proc = (Form_pg_proc) GETSTRUCT(tup);
languageTuple = SearchSysCache1(LANGOID,
ObjectIdGetDatum(proc-prolang));
Assert(HeapTupleIsValid(languageTuple));
lanForm = (Form_pg_language) GETSTRUCT(languageTuple);
languageChecker = lanForm-lanchecker;
funcname = format_procedure(funcOid);
/* We're all set to call the checker */
if (OidIsValid(languageChecker))
{
TupleDesc tupdesc;
Datum checkret;
FmgrInfo flinfo;
ReturnSetInfo rsinfo;
FunctionCallInfoData fcinfo;
/* create the tuple descriptor that the checker is supposed
to return */
tupdesc = CreateTemplateTupleDesc(10, false);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, functionid,
REGPROCOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, lineno,
INT4OID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 3, statement,
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 4, sqlstate,
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, message,
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 6, detail,
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 7, hint, TEXTOID,
-1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 8, level, TEXTOID,
-1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 9, position,
INT4OID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 10, query,
TEXTOID, -1, 0);
fmgr_info(languageChecker, flinfo);
rsinfo.type = T_ReturnSetInfo;
rsinfo.econtext = CreateStandaloneExprContext();
rsinfo.expectedDesc = tupdesc;
rsinfo.allowedModes = (int) SFRM_Materialize;
/* returnMode is set by the checker, hopefully ... */
/* isDone is not relevant, since not using ValuePerCall */
rsinfo.setResult = NULL;
Re: [HACKERS] review: CHECK FUNCTION statement
Excerpts from Pavel Stehule's message of sáb mar 03 02:45:06 -0300 2012: Without correct registration you cannot to call PL check function directly simply. I don't thing so this is good price for removing a few SPI lines. I don't understand why you don't like SPI. I don't dislike SPI in general. I just dislike using it internally in the backend. Other than RI, it's not used anywhere. It is used more times in code for similar purpose. this disallow direct PL check function call - so any more complex situation cannot be solved by SQL, but must be solved by PL/pgSQL with dynamic SQL Nonsense. Where did you get this idea? I did not touch the plpgsql code at all, it'd still work exactly as in your original patch. -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
2012/3/3 Alvaro Herrera alvhe...@commandprompt.com: Excerpts from Pavel Stehule's message of sáb mar 03 02:45:06 -0300 2012: Without correct registration you cannot to call PL check function directly simply. I don't thing so this is good price for removing a few SPI lines. I don't understand why you don't like SPI. I don't dislike SPI in general. I just dislike using it internally in the backend. Other than RI, it's not used anywhere. It is used more times in code for similar purpose. this disallow direct PL check function call - so any more complex situation cannot be solved by SQL, but must be solved by PL/pgSQL with dynamic SQL Nonsense. Where did you get this idea? I did not touch the plpgsql code at all, it'd still work exactly as in your original patch. ok I am sorry Pavel -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Why does CollectCheckedFunctions skip trigger functions? My only guess is that at one point the checker was not supposed to know how to check them, and a later version learned about it and this bit wasn't updated; but maybe there's another reason? -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Hello 2012/2/28 Alvaro Herrera alvhe...@commandprompt.com: In gram.y we have a new check_option_list nonterminal. This is mostly identical to explain_option_list, except that the option args do not take a NumericOnly (only opt_boolean_or_string and empty). I wonder if it's really worthwhile having a bunch of separate productions for this; how about we just use the existing explain_option_list instead and get rid of those extra productions? elog() is used in many user-facing messages (errors and notices). Full ereport() calls should be used there, so that messages are marked for translations and so on. I replaced elog by ereport for all not internal errors Does the patched pg_dump work with older servers? it should to do I don't like CheckFunction being declared in defrem.h. It seems completely out of place there. I don't see any better place though, so I'm thinking maybe we should have a new header file for it (say commands/functions.h; but we already have executor/functions.h so perhaps it's better to find another name). This addition means that there's a distressingly large number of .c files that are now getting dest.h, which was previously pretty confined. please, fix it like you wish Regards Pavel -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support check_function-2012-02-29-1.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] review: CHECK FUNCTION statement
I think the way we're passing down the options to the checker is a bit of a mess. The way it is formulated, it seems to me that we'd need to add support code in the core CheckFunction for each option we might want to accept in the PL-specific checkers -- including what type of value the option receives. As an example, right now we have all but one option taking a string argument (judging from usage of defGetString()); however, option fatal_errors takes a boolean value, and it converts to string on or off which is supposed to be passed down to the checker. This doesn't seem very future-proof. (Also, the patch seems to be passing the fatal_errors value twice: first in the options array, where it is ignored by the plpgsql checker, and a second time as a separate boolean option. This needs a cleanup). I don't see any good way to pass down generic options in a generic way. Maybe we can just state that all option values are going to be passed as strings -- is that good enough? The other option would be to pass them using something like pg_node_tree, but then it wouldn't be possible to call the checker directly instead of through CHECK FUNCTION, which I think was a requirement. And it'd be a stronger argument against usage of SPI to call the checker function from CHECK FUNCTION, but that's an unsolved problem. -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Hello 2012/2/29 Alvaro Herrera alvhe...@commandprompt.com: I think the way we're passing down the options to the checker is a bit of a mess. The way it is formulated, it seems to me that we'd need to add support code in the core CheckFunction for each option we might want to accept in the PL-specific checkers -- including what type of value the option receives. As an example, right now we have all but one option taking a string argument (judging from usage of defGetString()); however, option fatal_errors takes a boolean value, and it converts to string on or off which is supposed to be passed down to the checker. This doesn't seem very future-proof. I don't agree - we has not any datatype that can hold key/value list - and can be used via SQL interface. So two dimensional text array is simple and generic data type. Theoretically we can use JSON type now, but I think so array is more generic and better checked then JSON now (and it require less code - and JSON is still string too). We don't plan to modify parser to better support of JSON, so text array is more user friendly. I think so pl checker function can be simply called with used concept. But I am not against to your proposals. Actually concept of generic options was required in initial discuss and then there is implemented, but it not used. But cannot be removed, because probably don't would to change API in next version. (Also, the patch seems to be passing the fatal_errors value twice: first in the options array, where it is ignored by the plpgsql checker, and a second time as a separate boolean option. This needs a cleanup). This is by design. One request for checker function (and check function statement) was generic support for some optional data. This can has sense for plperl or plpython, and it are not used now. Fatal_errors is only proof concept and can be removed. I plan to use these options in 9.3 for checking of inline blocks. I don't see any good way to pass down generic options in a generic way. Maybe we can just state that all option values are going to be passed as strings -- is that good enough? The other option would be to pass them using something like pg_node_tree, but then it wouldn't be possible to call the checker directly instead of through CHECK FUNCTION, which I think was a requirement. And it'd be a stronger argument against usage of SPI to call the checker function from CHECK FUNCTION, but that's an unsolved problem. Using just string needs more complex parsing, and I don't like some like pg_node_tree too, because it cannot be simple created by hands for direct call of checker function. Please, accept fact, so we would to call directly PL checker function - and there all params of this function should be simple created - and using two dimensional array is really simple: ARRAY [['source',$$$$]]. I don't understand why you don't like pass generic options by generic way. Why? Regards Pavel Stehule -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
I have a few comments about this patch: I didn't like the fact that the checker calling infrastructure uses SPI instead of just a FunctionCallN to call the checker function. I think this should be easily avoidable. Second, I see that functioncmds.c gets a lot into trigger internals just to be able to figure out the function starting from a trigger name. I think it'd be saner to have a new function in trigger.c that returns the required function OID. I think CheckFunction would be clearer if the code to check multiple objects is split out into a separate subroutine. After CheckFunction there is a leftover function comment without any following function. There are other spurious hunks that add or remove single lines too (once in an otherwise untouched file). -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Hello Dne 28. února 2012 17:48 Alvaro Herrera alvhe...@commandprompt.com napsal(a): I have a few comments about this patch: I didn't like the fact that the checker calling infrastructure uses SPI instead of just a FunctionCallN to call the checker function. I think this should be easily avoidable. It is not possible - or it has not simple solution (I don't how to do it). PLpgSQL_checker is SRF function. SPI is used for processing returned resultset. I looked to pg source code, and I didn't find any other pattern than using SPI for SRF function call. It is probably possible, but it means some code duplication too. I invite any ideas. Second, I see that functioncmds.c gets a lot into trigger internals just to be able to figure out the function starting from a trigger name. I think it'd be saner to have a new function in trigger.c that returns the required function OID. done I think CheckFunction would be clearer if the code to check multiple objects is split out into a separate subroutine. done After CheckFunction there is a leftover function comment without any following function. There are other spurious hunks that add or remove single lines too (once in an otherwise untouched file). fixed I refreshed patch for current git repository. Regards Pavel -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support check_function-2012-02-28-2.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] review: CHECK FUNCTION statement
Excerpts from Pavel Stehule's message of mar feb 28 16:30:58 -0300 2012: I refreshed patch for current git repository. Thanks, I'll have a look. Oh, another thing -- you shouldn't patch the 1.0 version of the plpgsql extension. Rather I think you should produce a 1.1 version. -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
2012/2/28 Alvaro Herrera alvhe...@commandprompt.com: Excerpts from Pavel Stehule's message of mar feb 28 16:30:58 -0300 2012: I refreshed patch for current git repository. Thanks, I'll have a look. Oh, another thing -- you shouldn't patch the 1.0 version of the plpgsql extension. Rather I think you should produce a 1.1 version. there is patch with updated tag. I am not sure if there are some changes in pg_upgrade are necessary?? Regards and thank you Pavel -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support check_function-2012-02-28-3.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] review: CHECK FUNCTION statement
In gram.y we have a new check_option_list nonterminal. This is mostly identical to explain_option_list, except that the option args do not take a NumericOnly (only opt_boolean_or_string and empty). I wonder if it's really worthwhile having a bunch of separate productions for this; how about we just use the existing explain_option_list instead and get rid of those extra productions? elog() is used in many user-facing messages (errors and notices). Full ereport() calls should be used there, so that messages are marked for translations and so on. Does the patched pg_dump work with older servers? I don't like CheckFunction being declared in defrem.h. It seems completely out of place there. I don't see any better place though, so I'm thinking maybe we should have a new header file for it (say commands/functions.h; but we already have executor/functions.h so perhaps it's better to find another name). This addition means that there's a distressingly large number of .c files that are now getting dest.h, which was previously pretty confined. -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
2012/2/28 Alvaro Herrera alvhe...@commandprompt.com: In gram.y we have a new check_option_list nonterminal. This is mostly identical to explain_option_list, except that the option args do not take a NumericOnly (only opt_boolean_or_string and empty). I wonder if it's really worthwhile having a bunch of separate productions for this; how about we just use the existing explain_option_list instead and get rid of those extra productions? I have to look on it elog() is used in many user-facing messages (errors and notices). Full ereport() calls should be used there, so that messages are marked for translations and so on. yes, I'll fix it Does the patched pg_dump work with older servers? yes, It should to do I don't like CheckFunction being declared in defrem.h. It seems completely out of place there. I don't see any better place though, so I'm thinking maybe we should have a new header file for it (say commands/functions.h; but we already have executor/functions.h so perhaps it's better to find another name). This addition means that there's a distressingly large number of .c files that are now getting dest.h, which was previously pretty confined. you have much better knowledge about headers then me, so, please, propose solution. Regards Pavel -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Pavel Stehule wrote: here is new version of CHECK FUNCTION patch I changed implementation of interface: * checked functions returns table instead raising exceptions - it necessary for describing more issues inside one function - and it allow to use better structured data then ExceptionData [...] * result of CHECK FUNCTION is simple table (like EXPLAIN - based on Tom proposition) I don't have the time for a complete review, but I tried the patch and found: It is in context diff and applies to current master (there is fuzz 1 in one hunk). It contains documentation and regression tests. Compiles without warnings and passes regression tests. The two or three CHECK FUNCTIONs I ran worked ok. The documentation (that I wrote) will need to get updated: currently it states in two places that the checker function should throw a warning if it encounters a problem. 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] review: CHECK FUNCTION statement
Pavel Stehule wrote: here is new version of CHECK FUNCTION patch I won't be able to review that one because I'll be in California from Jan 6 to Jan 29. 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] review: CHECK FUNCTION statement
Hello all
here is new version of CHECK FUNCTION patch
I changed implementation of interface:
* checked functions returns table instead raising exceptions - it
necessary for describing more issues inside one function - and it
allow to use better structured data then ExceptionData
postgres=# select lineno, statement, sqlstate, message, detail, hint,
level, position, query from plpgsql_checker('f1()', 0, '{}', false);
lineno | statement | sqlstate | message
| detail | hint | level | position |query
+---+--++++---+--+--
4 | SQL statement | 42703| column c of relation t1 does
not exist | [null] | [null] | error | 15 | update t1 set c = 30
7 | RAISE | 42P01| missing FROM-clause entry for
table r| [null] | [null] | error |8 | SELECT r.c
7 | RAISE | 42601| too few parameters specified for
RAISE | [null] | [null] | error | [null] | [null]
(3 rows)
* result of CHECK FUNCTION is simple table (like EXPLAIN - based on
Tom proposition)
postgres=# check function f1();
CHECK FUNCTION
In function: 'f1()'
error:42703:4:SQL statement:column c of relation t1 does not exist
query:update t1 set c = 30
^
error:42P01:7:RAISE:missing FROM-clause entry for table r
query:SELECT r.c
^
error:42601:7:RAISE:too few parameters specified for RAISE
(8 rows)
This change allow a more playing with output
postgres=# check function all in schema public;
CHECK FUNCTION
In function: 'bubu(integer)'
error:42703:2:assignment:column v does not exist
query:SELECT a + v
^
error:42601:3:RETURN:query SELECT 1,1 returned 2 columns
query:SELECT 1,1
In function: 'f1()'
error:42703:4:SQL statement:column c of relation t1 does not exist
query:update t1 set c = 30
^
error:42P01:7:RAISE:missing FROM-clause entry for table r
query:SELECT r.c
^
error:42601:7:RAISE:too few parameters specified for RAISE
Function is valid: 'ff(integer)'
Function is valid: 'fff(integer)'
(18 rows)
Regards
Pavel Stehule
check_function-2012-01-01-1.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] review: CHECK FUNCTION statement
On 12/17/2011 04:00 PM, Pavel Stehule wrote: I use it for checking of my most large plpgsql project - it is about 300KB plpgsql procedures - but this code is not free - and this module helps to find lot of bugs. Great. If you continue to check against that regularly, that makes me feel better. I was guessing you had a large body of such source code around, and knowing it executed correctly against all of it improves my confidence here. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- 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] review: CHECK FUNCTION statement
2011/12/19 Greg Smith g...@2ndquadrant.com: On 12/17/2011 04:00 PM, Pavel Stehule wrote: I use it for checking of my most large plpgsql project - it is about 300KB plpgsql procedures - but this code is not free - and this module helps to find lot of bugs. Great. If you continue to check against that regularly, that makes me feel better. I was guessing you had a large body of such source code around, and knowing it executed correctly against all of it improves my confidence here. I am not alone a subset is used in plpgsql_lint and I know about some commercial subjects that use it too. https://github.com/okbob/plpgsql_lint but code in check function is little newer. It can interesting test some code that is wroted by person with background from other db, because they use a different patterns I don't use a explicit cursors for example - on other hand, I use exception intensively in my last project. We can ask people from LedgerSMB about testing if somebody has contact Regards Pavel -- Greg Smith 2ndQuadrant US g...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- 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] review: CHECK FUNCTION statement
2011/12/16 Albe Laurenz laurenz.a...@wien.gv.at: Pavel Stehule wrote: one small update - better emulation of environment for security definer functions Patch applies and compiles fine, core functionality works fine. I found a little bug: In backend/commands/functioncmds.c, function CheckFunction(CheckFunctionStmt *stmt), while you perform the table scan for CHECK FUNCTION ALL, you use the variable funcOid to hold the OID of the current function in the loop. If no appropriate function is found in the loop, the check immediately after the table scan will not succeed because funcOid holds the OID of the last function scanned in the loop. As a consequence, CheckFunctionById is called for this function. Here is a demonstration: test= CHECK FUNCTION ALL IN SCHEMA pg_catalog; [...] NOTICE: skip check function plpgsql_checker(oid,regclass,text[]), uses C language NOTICE: skip check function plperl_call_handler(), uses C language NOTICE: skip check function plperl_inline_handler(internal), uses C language NOTICE: skip check function plperl_validator(oid), uses C language NOTICE: skip check function plperl_validator(oid), language c hasn't checker function CHECK FUNCTION when it should be: test= CHECK FUNCTION ALL IN SCHEMA pg_catalog; [...] NOTICE: skip check function plpgsql_checker(oid,regclass,text[]), uses C language NOTICE: skip check function plperl_call_handler(), uses C language NOTICE: skip check function plperl_inline_handler(internal), uses C language NOTICE: skip check function plperl_validator(oid), uses C language NOTICE: nothing to check CHECK FUNCTION I'll fix it Another thing struck me as odd: You have the option fatal_errors for the checker function, but you special case it in CheckFunction(CheckFunctionStmt *stmt) and turn errors to warnings if it is not set. Wouldn't it be better to have the checker function ereport a WARNING or an ERROR depending on the setting? Options should be handled by the checker function. The behave that I use, is more rubust and there is only a few lines of code more. a) It ensure expectable behave for third party checker function - exception in checker function doesn't break a multi statement check function b) It can ensure same format of error message - because it is transformed on top level Regards Pavel 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] review: CHECK FUNCTION statement
2011/12/16 Greg Smith g...@2ndquadrant.com: I just poked at this a bit myself to see how the patch looked. There's just over 4000 lines in the diff. Even though 1/4 of that is tests, which is itself encouraging, that's still a good sized feature. The rate at which code here has still been changing regularly here has me nervous about considering this a commit candidate right now though. It seems like it still needs a bit more time to have problems squeezed out still. Two ideas I was thinking about here: -If you take a step back and look at where the problem parts of the code have been recently, are there any new tests or assertions you might add to try and detect problems like that in the future? I haven't been following this closely enough to have any suggestions where, and there is a lot of error checking aimed at logging already; maybe there's nothing new to chase there. last bug was based on wrong untoasting of function parameters - and it was hang on assertion - so it was ok I can recheck code where I can add asserts There is known issue - I cannot to check multi check statement in regress tests, because results (notices, errors) can be in random order. We don't sort functions by oid in check_functions_lookup. -Can we find some larger functions you haven't tested this against yet to throw at it? It seems able to consume all the cases you've constructed for it; it would be nice to find some brand new ones it's never seen before to check. I use it for checking of my most large plpgsql project - it is about 300KB plpgsql procedures - but this code is not free - and this module helps to find lot of bugs. I have plan to check a more functions from regress tests - but these tests are no bugs - I checked almost all four months ago - dynamic sql based and temp table based cannot check. This has made a lot of progress and seems it will be a good commit candidate for the next CF. I think it justs a bit more time than we have left in this CommitFest for it right now, particularly given the size of the patch. I'm turning this one into returned with feedback, but as a mediocre pl/pgsql author I'm hoping to see more updates still. I'll send update early Regards Pavel -- Greg Smith 2ndQuadrant US g...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- 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] review: CHECK FUNCTION statement
Hello You have the option fatal_errors for the checker function, but you special case it in CheckFunction(CheckFunctionStmt *stmt) and turn errors to warnings if it is not set. Wouldn't it be better to have the checker function ereport a WARNING or an ERROR depending on the setting? Options should be handled by the checker function. A would to process fatal_errors out of checker function - just it is more robust. This flag has not too sense in plpgsql - but can have a more sense in other languages. But I'll think again about flags note about warnings and errors. Warnings are useless on checker function level, because they are just shown, but they cannot be trapped. maybe result based on tuplestore can be better - I have to look on it. Regards Pavel 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] review: CHECK FUNCTION statement
Pavel Stehule wrote: one small update - better emulation of environment for security definer functions Patch applies and compiles fine, core functionality works fine. I found a little bug: In backend/commands/functioncmds.c, function CheckFunction(CheckFunctionStmt *stmt), while you perform the table scan for CHECK FUNCTION ALL, you use the variable funcOid to hold the OID of the current function in the loop. If no appropriate function is found in the loop, the check immediately after the table scan will not succeed because funcOid holds the OID of the last function scanned in the loop. As a consequence, CheckFunctionById is called for this function. Here is a demonstration: test= CHECK FUNCTION ALL IN SCHEMA pg_catalog; [...] NOTICE: skip check function plpgsql_checker(oid,regclass,text[]), uses C language NOTICE: skip check function plperl_call_handler(), uses C language NOTICE: skip check function plperl_inline_handler(internal), uses C language NOTICE: skip check function plperl_validator(oid), uses C language NOTICE: skip check function plperl_validator(oid), language c hasn't checker function CHECK FUNCTION when it should be: test= CHECK FUNCTION ALL IN SCHEMA pg_catalog; [...] NOTICE: skip check function plpgsql_checker(oid,regclass,text[]), uses C language NOTICE: skip check function plperl_call_handler(), uses C language NOTICE: skip check function plperl_inline_handler(internal), uses C language NOTICE: skip check function plperl_validator(oid), uses C language NOTICE: nothing to check CHECK FUNCTION Another thing struck me as odd: You have the option fatal_errors for the checker function, but you special case it in CheckFunction(CheckFunctionStmt *stmt) and turn errors to warnings if it is not set. Wouldn't it be better to have the checker function ereport a WARNING or an ERROR depending on the setting? Options should be handled by the checker function. 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] review: CHECK FUNCTION statement
I just poked at this a bit myself to see how the patch looked. There's just over 4000 lines in the diff. Even though 1/4 of that is tests, which is itself encouraging, that's still a good sized feature. The rate at which code here has still been changing regularly here has me nervous about considering this a commit candidate right now though. It seems like it still needs a bit more time to have problems squeezed out still. Two ideas I was thinking about here: -If you take a step back and look at where the problem parts of the code have been recently, are there any new tests or assertions you might add to try and detect problems like that in the future? I haven't been following this closely enough to have any suggestions where, and there is a lot of error checking aimed at logging already; maybe there's nothing new to chase there. -Can we find some larger functions you haven't tested this against yet to throw at it? It seems able to consume all the cases you've constructed for it; it would be nice to find some brand new ones it's never seen before to check. This has made a lot of progress and seems it will be a good commit candidate for the next CF. I think it justs a bit more time than we have left in this CommitFest for it right now, particularly given the size of the patch. I'm turning this one into returned with feedback, but as a mediocre pl/pgsql author I'm hoping to see more updates still. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- 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] review: CHECK FUNCTION statement
Pavel Stehule wrote: so removed quite option and removed multiple check regression tests also - there is missing explicit order of function checking, so regress tests can fail :( There seems to be a problem with the SET clause of CREATE FUNCTION: ftest=# CREATE OR REPLACE FUNCTION a(integer) RETURNS integer LANGUAGE plpgsql AS 'BEGIN RETURN 2*$1; END'; CREATE FUNCTION ftest=# CHECK FUNCTION a(integer); NOTICE: checked function a(integer) CHECK FUNCTION ftest=# CREATE OR REPLACE FUNCTION a(integer) RETURNS integer LANGUAGE plpgsql SET search_path=public AS 'BEGIN RETURN 2*$1; END'; CREATE FUNCTION ftest=# CHECK FUNCTION a(integer); The connection to the server was lost. Attempting reset: Failed. ! 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] review: CHECK FUNCTION statement
Hello 2011/12/15 Albe Laurenz laurenz.a...@wien.gv.at: Pavel Stehule wrote: so removed quite option and removed multiple check regression tests also - there is missing explicit order of function checking, so regress tests can fail :( There seems to be a problem with the SET clause of CREATE FUNCTION: ftest=# CREATE OR REPLACE FUNCTION a(integer) RETURNS integer LANGUAGE plpgsql AS 'BEGIN RETURN 2*$1; END'; CREATE FUNCTION ftest=# CHECK FUNCTION a(integer); NOTICE: checked function a(integer) CHECK FUNCTION ftest=# CREATE OR REPLACE FUNCTION a(integer) RETURNS integer LANGUAGE plpgsql SET search_path=public AS 'BEGIN RETURN 2*$1; END'; CREATE FUNCTION ftest=# CHECK FUNCTION a(integer); The connection to the server was lost. Attempting reset: Failed. ! There was bug - missing detoast call fixed Regards Pavel Yours, Laurenz Albe check_function-2011-12-15-1.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] review: CHECK FUNCTION statement
Hello one small update - better emulation of environment for security definer functions Regards Pavel check_function-2011-12-15-2.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] review: CHECK FUNCTION statement
Pavel Stehule wrote:
changes:
* fixed warnings
* support for options - actually only two options are supported -
quite and fatal_errors
these options are +/- useful - main reason for their existence is
testing of support of options - processing on CHECK ... stmt side and
processing on checker function side.
options are send as 2d text array - some like
'{{quite,on},{fatal_errors,on}} - so direct call of checker function
is possible
* regress test for multi check
First of all: It should be quiet and not quite.
The patch applies and builds fine.
It fails one of ist own regression tests, here is the diff:
*** /postgres/cvs/postgresql/src/test/regress/expected/plpgsql.out
2011-12-14 11:50:44.0 +0100
--- /postgres/cvs/postgresql/src/test/regress/results/plpgsql.out
2011-12-14 16:19:45.0 +0100
***
*** 4975,4991
end;
$$ language plpgsql;
check function all in schema plpgsql_check;
- NOTICE: checked function plpgsql_check.fce1()
WARNING: error in function plpgsql_check.fce2()
DETAIL: too few parameters specified for RAISE
CONTEXT: line 3 at RAISE
NOTICE: checked function plpgsql_check.fce3()
check function all in schema plpgsql_check with (quite);
WARNING: error in function plpgsql_check.fce2()
DETAIL: too few parameters specified for RAISE
CONTEXT: line 3 at RAISE
check function all in schema plpgsql_check with (fatal_errors);
- NOTICE: checked function plpgsql_check.fce1()
ERROR: too few parameters specified for RAISE
CONTEXT: PL/pgSQL function fce2 line 3 at RAISE
check function all in schema plpgsql_check with (quite, fatal_errors on);
--- 4975,4990
end;
$$ language plpgsql;
check function all in schema plpgsql_check;
WARNING: error in function plpgsql_check.fce2()
DETAIL: too few parameters specified for RAISE
CONTEXT: line 3 at RAISE
NOTICE: checked function plpgsql_check.fce3()
+ NOTICE: checked function plpgsql_check.fce1()
check function all in schema plpgsql_check with (quite);
WARNING: error in function plpgsql_check.fce2()
DETAIL: too few parameters specified for RAISE
CONTEXT: line 3 at RAISE
check function all in schema plpgsql_check with (fatal_errors);
ERROR: too few parameters specified for RAISE
CONTEXT: PL/pgSQL function fce2 line 3 at RAISE
check function all in schema plpgsql_check with (quite, fatal_errors on);
==
The quiet option is not very intuitive:
test= CHECK FUNCTION ALL WITH (quite 'off');
NOTICE: skip check function atrig(), it is trigger function
NOTICE: skip check function perl_max(integer,integer), language plperl
hasn't checker function
NOTICE: checked function ok()
NOTICE: checked function newstyle(integer)
CHECK FUNCTION
test= CHECK FUNCTION ALL WITH (quite 'on');
NOTICE: skip check function atrig(), it is trigger function
CHECK FUNCTION
I understand that quiet cannot silence this message, nor
skip ..., uses C language and skip ..., it uses internal language,
but that means that it is not very useful as it is.
If all we need is a sample option, I think that fatal_errors is
enough, and I think that is an option that can be useful.
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] review: CHECK FUNCTION statement
Hello
so removed quite option
and removed multiple check regression tests also - there is missing
explicit order of function checking, so regress tests can fail :(
Regards
Pavel
2011/12/14 Albe Laurenz laurenz.a...@wien.gv.at:
Pavel Stehule wrote:
changes:
* fixed warnings
* support for options - actually only two options are supported -
quite and fatal_errors
these options are +/- useful - main reason for their existence is
testing of support of options - processing on CHECK ... stmt side and
processing on checker function side.
options are send as 2d text array - some like
'{{quite,on},{fatal_errors,on}} - so direct call of checker function
is possible
* regress test for multi check
First of all: It should be quiet and not quite.
The patch applies and builds fine.
It fails one of ist own regression tests, here is the diff:
*** /postgres/cvs/postgresql/src/test/regress/expected/plpgsql.out
2011-12-14 11:50:44.0 +0100
--- /postgres/cvs/postgresql/src/test/regress/results/plpgsql.out
2011-12-14 16:19:45.0 +0100
***
*** 4975,4991
end;
$$ language plpgsql;
check function all in schema plpgsql_check;
- NOTICE: checked function plpgsql_check.fce1()
WARNING: error in function plpgsql_check.fce2()
DETAIL: too few parameters specified for RAISE
CONTEXT: line 3 at RAISE
NOTICE: checked function plpgsql_check.fce3()
check function all in schema plpgsql_check with (quite);
WARNING: error in function plpgsql_check.fce2()
DETAIL: too few parameters specified for RAISE
CONTEXT: line 3 at RAISE
check function all in schema plpgsql_check with (fatal_errors);
- NOTICE: checked function plpgsql_check.fce1()
ERROR: too few parameters specified for RAISE
CONTEXT: PL/pgSQL function fce2 line 3 at RAISE
check function all in schema plpgsql_check with (quite, fatal_errors on);
--- 4975,4990
end;
$$ language plpgsql;
check function all in schema plpgsql_check;
WARNING: error in function plpgsql_check.fce2()
DETAIL: too few parameters specified for RAISE
CONTEXT: line 3 at RAISE
NOTICE: checked function plpgsql_check.fce3()
+ NOTICE: checked function plpgsql_check.fce1()
check function all in schema plpgsql_check with (quite);
WARNING: error in function plpgsql_check.fce2()
DETAIL: too few parameters specified for RAISE
CONTEXT: line 3 at RAISE
check function all in schema plpgsql_check with (fatal_errors);
ERROR: too few parameters specified for RAISE
CONTEXT: PL/pgSQL function fce2 line 3 at RAISE
check function all in schema plpgsql_check with (quite, fatal_errors on);
==
The quiet option is not very intuitive:
test= CHECK FUNCTION ALL WITH (quite 'off');
NOTICE: skip check function atrig(), it is trigger function
NOTICE: skip check function perl_max(integer,integer), language plperl
hasn't checker function
NOTICE: checked function ok()
NOTICE: checked function newstyle(integer)
CHECK FUNCTION
test= CHECK FUNCTION ALL WITH (quite 'on');
NOTICE: skip check function atrig(), it is trigger function
CHECK FUNCTION
I understand that quiet cannot silence this message, nor
skip ..., uses C language and skip ..., it uses internal language,
but that means that it is not very useful as it is.
If all we need is a sample option, I think that fatal_errors is
enough, and I think that is an option that can be useful.
Yours,
Laurenz Albe
check_function-2011-12-14-3.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] review: CHECK FUNCTION statement
Pavel Stehule wrote: One thing I forgot to mention: I thought there was a consensus to add a WITH() or OPTIONS() clause to pass options to the checker function: http://archives.postgresql.org/message-id/12568.1322669...@sss.pgh.pa.us I think this should be there so that the API does not have to be changed in the future. there is just one question - how propagate options to check functions I am thinking about third parameter - probably text array Either that, or couldn't you pass an option List as data type internal? I don't know what is most natural or convenient. 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] review: CHECK FUNCTION statement
2011/12/13 Albe Laurenz laurenz.a...@wien.gv.at: Pavel Stehule wrote: One thing I forgot to mention: I thought there was a consensus to add a WITH() or OPTIONS() clause to pass options to the checker function: http://archives.postgresql.org/message-id/12568.1322669...@sss.pgh.pa.us I think this should be there so that the API does not have to be changed in the future. there is just one question - how propagate options to check functions I am thinking about third parameter - probably text array Either that, or couldn't you pass an option List as data type internal? this is question - internal is most simply solution, but then we cannot to call check function directly Regards Pavel I don't know what is most natural or convenient. 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] review: CHECK FUNCTION statement
Pavel Stehule pavel.steh...@gmail.com writes: 2011/12/13 Albe Laurenz laurenz.a...@wien.gv.at: Either that, or couldn't you pass an option List as data type internal? this is question - internal is most simply solution, but then we cannot to call check function directly Yeah, one of the proposals for allowing people to specify complicated conditions about what to check was to tell them to do select checker(oid) from pg_proc where any-random-condition; If the checker isn't user-callable then we lose that escape hatch, and the only selection conditions that will ever be possible are the ones we take the trouble to shoehorn into the CHECK FUNCTION statement. Doesn't seem like a good thing to me. 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] review: CHECK FUNCTION statement
2011/12/13 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: 2011/12/13 Albe Laurenz laurenz.a...@wien.gv.at: Either that, or couldn't you pass an option List as data type internal? this is question - internal is most simply solution, but then we cannot to call check function directly Yeah, one of the proposals for allowing people to specify complicated conditions about what to check was to tell them to do select checker(oid) from pg_proc where any-random-condition; If the checker isn't user-callable then we lose that escape hatch, and the only selection conditions that will ever be possible are the ones we take the trouble to shoehorn into the CHECK FUNCTION statement. Doesn't seem like a good thing to me. yes, it is reason why I thinking just about string array. I have not idea about other PL, but options for plpgsql can be one word and checker function can simply parse two or more words options. Now I would to implement flags quite - ignore NOTIFY messages and fatal_errors to stop on first error. 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] review: CHECK FUNCTION statement
Hello
2011/12/12 Albe Laurenz laurenz.a...@wien.gv.at:
Pavel Stehule wrote:
there is merged patch
Works fine, except that there are still missing const qualifiers
in copyfuncs.c and equalfuncs.c that lead to compiler warnings.
One thing I forgot to mention:
I thought there was a consensus to add a WITH() or OPTIONS() clause
to pass options to the checker function:
http://archives.postgresql.org/message-id/12568.1322669...@sss.pgh.pa.us
I think this should be there so that the API does not have to be
changed in the future.
changes:
* fixed warnings
* support for options - actually only two options are supported -
quite and fatal_errors
these options are +/- useful - main reason for their existence is
testing of support of options - processing on CHECK ... stmt side and
processing on checker function side.
options are send as 2d text array - some like
'{{quite,on},{fatal_errors,on}} - so direct call of checker function
is possible
* regress test for multi check
Regards
Pavel
Yours,
Laurenz Albe
check_function-2011-12-14-1.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] review: CHECK FUNCTION statement
Pavel Stehule wrote: there is merged patch Works fine, except that there are still missing const qualifiers in copyfuncs.c and equalfuncs.c that lead to compiler warnings. One thing I forgot to mention: I thought there was a consensus to add a WITH() or OPTIONS() clause to pass options to the checker function: http://archives.postgresql.org/message-id/12568.1322669...@sss.pgh.pa.us I think this should be there so that the API does not have to be changed in the future. 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] review: CHECK FUNCTION statement
hello 2011/12/12 Albe Laurenz laurenz.a...@wien.gv.at: Pavel Stehule wrote: there is merged patch Works fine, except that there are still missing const qualifiers in copyfuncs.c and equalfuncs.c that lead to compiler warnings. One thing I forgot to mention: I thought there was a consensus to add a WITH() or OPTIONS() clause to pass options to the checker function: http://archives.postgresql.org/message-id/12568.1322669...@sss.pgh.pa.us I think this should be there so that the API does not have to be changed in the future. there is just one question - how propagate options to check functions I am thinking about third parameter - probably text array ?? Regards Pavel 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] review: CHECK FUNCTION statement
Pavel Stehule wrote: updated version changes: * CHECK FUNCTION ALL; is enabled - in this case functions from pg_catalog schema are ignored I looked on parser, and I didn't other changes there - IN SCHEMA, FOR ROLE are used more time there, so our usage will be consistent a small addition * don't check SQL functions - are checked well now * don't check functions from information_schema too One hunk in the patch fails due to conflict with commit d5f23af6bfbc454e86dd16e5c7a0bfc0cf6189d0 (Peter Eisentraut's const patch). There are also compiler warnings about discarded const qualifiers in backend/nodes/copyfuncs.c, backend/nodes/equalfuncs.c and backend/parser/gram.y. There is a bug when ALL IN SCHEMA or ALL IN LANGUAGE is used: test= CHECK FUNCTION ALL IN LANGUAGE plpgsql; ERROR: language language does not exist test= CHECK FUNCTION ALL IN SCHEMA laurenz; ERROR: schema schema does not exist Something gets mixed up here. I like the idea that CHECK FUNCTION ALL without additional clauses works and ignores pg_catalog and information_schema! I'm working on some documentation, but it won't be final before the functionality is agreed upon. 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] review: CHECK FUNCTION statement
Pavel Stehule wrote: there is fixed version Here is my attempt at a doc patch. Could you add it to your patch so that all is in a single patch? Yours, Laurenz Albe check_function_docs.patch Description: check_function_docs.patch -- 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] review: CHECK FUNCTION statement
Hello 2011/12/9 Albe Laurenz laurenz.a...@wien.gv.at: Pavel Stehule wrote: there is fixed version Here is my attempt at a doc patch. Could you add it to your patch so that all is in a single patch? there is merged patch Thank you Regards Pavel Yours, Laurenz Albe check_function-2011-12-09-3.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] review: CHECK FUNCTION statement
Pavel Stehule wrote: there is a updated patch. it support multi check, options and custom check functions are not supported yet. I don't plan to implement custom check functions in this round - I has not any example of usage - but we have agreement on syntax and behave, so this should not be problem. I changed reporting - from exception to warnings. The patch applies and builds cleanly. The syntax error messages are still inadequate; all I can get is 'syntax error at or near %s'. They should be more detailed. Many other messages and code comments are in bad English. It might be a good idea to add some regression tests for the CHECK FUNCTION ALL variants. Functionality: -- I noticed an oddity: postgres=# CHECK FUNCTION ALL; ERROR: syntax error at or near ; LINE 1: CHECK FUNCTION ALL; ^ postgres=# CHECK FUNCTION ALL IN LANGUAGE plpgsql; NOTICE: nothing to check postgres=# CHECK FUNCTION ALL IN SCHEMA pg_catalog; [prints lots of NOTICEs] According to the syntax diagram and my intuition CHECK FUNCTION ALL without additional clauses should work. Regarding the syntax: I know I suggested it myself, but after several times of typing IN LANGUAGE plpgsql I think that omitting the IN would be better and more like other commands (e.g. CREATE FUNCTION). It is a pity that the CHECK FUNCTION ALL variants will not check trigger functions, but I understand the difficulty -- it would require checking all trigger functions on all tables where they occur in a trigger. I think that the checker function should be shown in psql's \dL+ output. Barring these little gripes, the functionality seems ready for committer from my point of view. Code review: I do not feel competent for a thorough code review. Documentation: -- This is where I see the greatest shortcomings. - The documentation for the system catalog pg_pltemplate should be extended to include tmplchecker. - The documentation patch for CREATE LANGUAGE is plain wrong and contains a syntax error. - CHECK FUNCTION and CHECK TRIGGER should be treated as different SQL statements. It is misleading to have CHECK TRIGGER listed under CHECK FUNCTION. If they have to be together, the statement should be called CHECK and not CHECK TRIGGER, but I think they should be separate. - There is still no documentation patch for plhandler.sgml. I think that at least the documentation should be improved before I am ready to set this as ready for committer. 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] review: CHECK FUNCTION statement
2011/12/7 Albe Laurenz laurenz.a...@wien.gv.at: Pavel Stehule wrote: there is a updated patch. it support multi check, options and custom check functions are not supported yet. I don't plan to implement custom check functions in this round - I has not any example of usage - but we have agreement on syntax and behave, so this should not be problem. I changed reporting - from exception to warnings. The patch applies and builds cleanly. The syntax error messages are still inadequate; all I can get is 'syntax error at or near %s'. They should be more detailed. this system is based on error messages that generates a plpgsql engine or bison engine. I can correct only a few percent from these messages :( internally I didn't wrote a compiler or plpgsql checker - this is just tool that can emit some plpgsql interpret subprocess - and when these subprocesses raises exceptions, then takes their messages. Many other messages and code comments are in bad English. It might be a good idea to add some regression tests for the CHECK FUNCTION ALL variants. Functionality: -- I noticed an oddity: postgres=# CHECK FUNCTION ALL; ERROR: syntax error at or near ; LINE 1: CHECK FUNCTION ALL; ^ postgres=# CHECK FUNCTION ALL IN LANGUAGE plpgsql; NOTICE: nothing to check postgres=# CHECK FUNCTION ALL IN SCHEMA pg_catalog; [prints lots of NOTICEs] According to the syntax diagram and my intuition CHECK FUNCTION ALL without additional clauses should work. this is question - this will check all functions in postgres.It's 2421 function, so one criterium as minimum should be good idea. We can remove buildin functions from list - so it will check all function in database. Regarding the syntax: I know I suggested it myself, but after several times of typing IN LANGUAGE plpgsql I think that omitting the IN would be better and more like other commands (e.g. CREATE FUNCTION). IN should be syntactic sugar It is a pity that the CHECK FUNCTION ALL variants will not check trigger functions, but I understand the difficulty -- it would require checking all trigger functions on all tables where they occur in a trigger. I think that the checker function should be shown in psql's \dL+ output. Barring these little gripes, the functionality seems ready for committer from my point of view. Code review: I do not feel competent for a thorough code review. Documentation: -- This is where I see the greatest shortcomings. - The documentation for the system catalog pg_pltemplate should be extended to include tmplchecker. - The documentation patch for CREATE LANGUAGE is plain wrong and contains a syntax error. - CHECK FUNCTION and CHECK TRIGGER should be treated as different SQL statements. It is misleading to have CHECK TRIGGER listed under CHECK FUNCTION. If they have to be together, the statement should be called CHECK and not CHECK TRIGGER, but I think they should be separate. - There is still no documentation patch for plhandler.sgml. I think that at least the documentation should be improved before I am ready to set this as ready for committer. please, can you send a correction to documentation or error messages? I am not able to write documentation Regards Pavel 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] review: CHECK FUNCTION statement
Pavel Stehule wrote: The syntax error messages are still inadequate; all I can get is 'syntax error at or near %s'. They should be more detailed. this system is based on error messages that generates a plpgsql engine or bison engine. I can correct only a few percent from these messages :( internally I didn't wrote a compiler or plpgsql checker - this is just tool that can emit some plpgsql interpret subprocess - and when these subprocesses raises exceptions, then takes their messages. I see. I think that at least the documentation should be improved before I am ready to set this as ready for committer. please, can you send a correction to documentation or error messages? I am not able to write documentation I'll give it a try. 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] review: CHECK FUNCTION statement
Pavel Stehule wrote:
My attempt at a syntax that could also cover Peter's wish for multiple
checker functions:
CHECK FUNCTION { func(args) | ALL [IN SCHEMA schema] [FOR ROLE user] }
[ USING check_function ] OPTIONS (optname optarg [, ...])
check_function should be related to one language, so you have to
specify language if you would to specify check_function (if we would
to have more check functions for one language).
Right, I forgot LANGUAGE:
CHECK FUNCTION { func(args) | ALL IN LANGUAGE pl [IN SCHEMA schema] [FOR ROLE
user] }
[ USING check_function ] OPTIONS (optname optarg [, ...])
If func(args) is given, the language can be inferred.
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] review: CHECK FUNCTION statement
On ons, 2011-11-30 at 10:53 -0500, Tom Lane wrote: I think the important point here is that we need to support more than one level of validation, and that the higher levels can't really be applied by default in CREATE FUNCTION because they may fail on perfectly valid code. How would this work with anything other than PL/pgSQL in practice? Here is an additional use case: There are a bunch of syntax and style checkers for Python: pylint, pyflakes, pep8, pychecker, and maybe more. I would like to have a way to use these for PL/Python. Right now I use a tool I wrote called plpylint (https://github.com/petere/plpylint), which pulls the source code out of the database and runs pylint on the client, which works well enough, but what is being discussed here could lead to a better solution. So what I'd like to have is some way to say check all plpythonu functions [in this schema or whatever] using checker pylint where pylint was previously defined as a checker associated with the plpythonu language that actually invokes some user-defined function. Also, what kind of report does this generate? -- 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] review: CHECK FUNCTION statement
Tom Lane wrote:
Do I understand right that the reason why the check function is
different from the validator function is that it would be more
difficult
to add the checks to the validator function?
Is that a good enough argument? From a user's perspective it is
difficult to see why some checks are performed at function creation
time, while others have to be explicitly checked with CHECK FUNCTION.
I think it would be much more intuitive if CHECK FUNCTION does
the same as function validation with check_function_bodies on.
I think the important point here is that we need to support more than
one level of validation, and that the higher levels can't really be
applied by default in CREATE FUNCTION because they may fail on
perfectly
valid code.
I understand now.
There are three levels of checking:
1) Validation with check_function_bodies = off (checks nothing).
2) Validation with check_function_bodies = on (checks syntax).
3) CHECK FUNCTION (checks RAISE and objects referenced in the function).
As long as 3) implies 2) (which I think it does), that makes sense.
I guess I was led astray by the documentation in plhandler.sgml:
Validator functions should typically honor the check_function_bodies
parameter: [...] this parameter is turned off by pg_dump so that it
can load procedural language functions without worrying about possible
dependencies of the function bodies on other database objects.
Dependencyies on other database objects seems more like a description
of CHECK FUNCTION.
But I guess that this documentation should be changed anyway to describe
the check function.
A bigger issue is that once you think about more than one kind of
check,
it becomes apparent that we might need some user-specifiable options
to
control which checks are applied. And I see no provision for that
here.
My attempt at a syntax that could also cover Peter's wish for multiple
checker functions:
CHECK FUNCTION { func(args) | ALL [IN SCHEMA schema] [FOR ROLE user] }
[ USING check_function ] OPTIONS (optname optarg [, ...])
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] review: CHECK FUNCTION statement
Peter Eisentraut pete...@gmx.net writes: On ons, 2011-11-30 at 10:53 -0500, Tom Lane wrote: I think the important point here is that we need to support more than one level of validation, and that the higher levels can't really be applied by default in CREATE FUNCTION because they may fail on perfectly valid code. How would this work with anything other than PL/pgSQL in practice? Well, that's TBD by the individual PL authors, but it hardly seems implausible that there might be lint-like checks applicable in many PLs. As long as we have the functionality pushed out to a PL-specific checker function, the details can be worked out later. So what I'd like to have is some way to say check all plpythonu functions [in this schema or whatever] using checker pylint where pylint was previously defined as a checker associated with the plpythonu language that actually invokes some user-defined function. That sounds like a language-specific option to me. Also, what kind of report does this generate? Good question. I suspect what Pavel has now will raise errors, but that doesn't scale very nicely to checking more than one function, or even to finding more than one bug in a single function. My first instinct is to say that it should work like plain EXPLAIN, ie, deliver a textual report that we send as if it were a query result. 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] review: CHECK FUNCTION statement
Hello Also, what kind of report does this generate? Good question. I suspect what Pavel has now will raise errors, but that doesn't scale very nicely to checking more than one function, or even to finding more than one bug in a single function. I stop on first error now. Reason is reuse of functionality that can to mark a critical point (bug) of embedded query in console. Continuous checking is possible (plpgsql), but there are a few critical bugs, that does stop. For example - any buggy assign to record var causes uninitialized variable and any later checks are affected. This is possible when ASSIGN, FOR SELECT, SELECT INTO statements are used. It is small part of possible bugs but relative often pattern. So I didn't worry about it yet. My first instinct is to say that it should work like plain EXPLAIN, ie, deliver a textual report that we send as if it were a query result. can be - but EXPLAIN raises exception too, when there some error. There should be a some possibility to simply identify result. I am not sure if checking on empty result is good way. A raising exception should be option. When we return text, then we have to think about some structured form result - line, position, message. A advance of exception on first issue is fact, so these questions are solved. so CHECK can returns lines - like EXPLAIN, but can be nice and helpful for this moment a GUC - some like check_raises_exception = on|off. Default should be off. And I have to think about some FORMAT option. is it good plan? 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] review: CHECK FUNCTION statement
Hello
My attempt at a syntax that could also cover Peter's wish for multiple
checker functions:
CHECK FUNCTION { func(args) | ALL [IN SCHEMA schema] [FOR ROLE user] }
[ USING check_function ] OPTIONS (optname optarg [, ...])
check_function should be related to one language, so you have to
specify language if you would to specify check_function (if we would
to have more check functions for one language).
Regards
Pavel Stehule
--
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] review: CHECK FUNCTION statement
2011/12/2 Pavel Stehule pavel.steh...@gmail.com:
Hello
My attempt at a syntax that could also cover Peter's wish for multiple
checker functions:
CHECK FUNCTION { func(args) | ALL [IN SCHEMA schema] [FOR ROLE user] }
[ USING check_function ] OPTIONS (optname optarg [, ...])
some other idea about other using CHECK FUNCTION
CHECK FUNCTION func(args)
RETURNS ... AS $$
$$ LANGUAGE xxx
This should to do check of function body without affect on registered
function. This is addition to previous defined syntax.
Nice a day
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] review: CHECK FUNCTION statement
Pavel Stehule wrote: updated patch: * recheck compilation and initdb * working routines moved to pl_exec.c * add entry to catalog.sgml about lanchecker field * add node's utils Documentation: -- This patch has no documentation for CHECK FUNCTION or CHECK TRIGGER. The last patch had at least something. \h check function in psql does not show anything. The patch should also add documentation about the handler function to plhandler.sgml. In particular, the difference between the validator function and the check function should be pointed out. Usability: -- Do I understand right that the reason why the check function is different from the validator function is that it would be more difficult to add the checks to the validator function? Is that a good enough argument? From a user's perspective it is difficult to see why some checks are performed at function creation time, while others have to be explicitly checked with CHECK FUNCTION. I think it would be much more intuitive if CHECK FUNCTION does the same as function validation with check_function_bodies on. Submission, Compilation, Regression tests: -- The patch applies and compiles fine and passes regression tests. The tests cover the functionality. initdb succeeds. pg_dump: pg_dump support does not work right. If I create a language like this: CREATE LANGUAGE mylang HANDLER plpgsql_call_handler INLINE plpgsql_inline_handler VALIDATOR plpgsql_validator CHECK plpgsql_checker; the dump will contain: CREATE OR REPLACE PROCEDURAL LANGUAGE mylang; This is not a problem of this patch though (same in 9.1); it seems that pg_dump support for languages without definition in pg_pltemplate is broken in general. Feature test: - CHECK FUNCTION and CHECK TRIGGER work, I couldn't crash it. Error messages could be better: CHECK TRIGGER atrigger; ERROR: syntax error at or near ; LINE 1: CHECK TRIGGER atrigger; ^ Something like expected keyword 'ON' might be nice. There are a lot of things that CHECK FUNCTION does not check, some examples: 1) CREATE OR REPLACE FUNCTION t(i integer) RETURNS integer LANGUAGE plpgsql STRICT AS $$DECLARE j integer; BEGIN IF i=1 THEN FOR I IN 1..4 BY -1 LOOP RAISE NOTICE '%', i; END LOOP; RETURN -1; ELSE RETURN 2*i; END IF; END;$$; CHECK FUNCTION t(integer); -- no error SELECT t(1); ERROR: BY value of FOR loop must be greater than zero CONTEXT: PL/pgSQL function t line 4 at FOR with integer loop variable 2) CREATE OR REPLACE FUNCTION t(i integer) RETURNS integer LANGUAGE plpgsql STRICT AS $$DECLARE j integer; BEGIN IF i=1 THEN j=999; RETURN j; ELSE RETURN 2*i; END IF; END;$$; CHECK FUNCTION t(integer); -- no error SELECT t(1); ERROR: value 999 is out of range for type integer CONTEXT: PL/pgSQL function t line 4 at assignment 3) CREATE OR REPLACE FUNCTION t(i integer) RETURNS integer LANGUAGE plpgsql STRICT AS $$DECLARE j atable; BEGIN IF i=1 THEN j=12; RETURN j; ELSE RETURN 2*i; END IF; END;$$; CHECK FUNCTION t(integer); -- no error SELECT t(1); ERROR: cannot assign non-composite value to a row variable CONTEXT: PL/pgSQL function t line 3 at assignment 4) CREATE TABLE atable( id integer PRIMARY KEY, val text NOT NULL ); INSERT INTO atable VALUES (1, 'eins'); CREATE OR REPLACE FUNCTION atrigger() RETURNS trigger LANGUAGE plpgsql STRICT AS $$BEGIN NEW.id=22; RETURN NEW; END;$$; CREATE TRIGGER atrigger AFTER DELETE ON atable FOR EACH ROW EXECUTE PROCEDURE atrigger(); CHECK TRIGGER atrigger ON atable; -- no error NOTICE: checking function atrigger() DELETE FROM atable; ERROR: record new is not assigned yet DETAIL: The tuple structure of a not-yet-assigned record is indeterminate. CONTEXT: PL/pgSQL function atrigger line 2 at assignment I can try and come up with more if desired. Maybe case 2) and 4) cannot reasonably be covered. It is probably very hard to check everything possible, but the usefulness of CHECK FUNCTION is proportional to the number of cases covered. I'll mark the patch as Waiting on Author until there is more documentation, I understand the answers to the questions raised in usability above, and until it is agreed that the things checked are sufficient. 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] review: CHECK FUNCTION statement
Albe Laurenz laurenz.a...@wien.gv.at writes: Do I understand right that the reason why the check function is different from the validator function is that it would be more difficult to add the checks to the validator function? Is that a good enough argument? From a user's perspective it is difficult to see why some checks are performed at function creation time, while others have to be explicitly checked with CHECK FUNCTION. I think it would be much more intuitive if CHECK FUNCTION does the same as function validation with check_function_bodies on. I think the important point here is that we need to support more than one level of validation, and that the higher levels can't really be applied by default in CREATE FUNCTION because they may fail on perfectly valid code. The real reason why we need a separate check function is that the API for validators doesn't include any parameter about check level. It's conceivable that instead of adding a check-function entry point, we could generalizee check_function_bodies into a more-than-2-level setting, and expect validators to pay attention to the GUC's value when deciding how aggressively to validate. However, it's far from clear to me that that's a more usable definition than having a separate CHECK FUNCTION command. An example of where a separate CHECK command could come in handy is: you just did some ALTER TABLE commands, and now you would like to check if your functions all still work with the modified table schemas. [ snip examples of some additional checks that could be made ] It is probably very hard to check everything possible, but the usefulness of CHECK FUNCTION is proportional to the number of cases covered. I don't think that the initial patch needs to check everything that could conceivably be checked. We can always add more checking later. The initial patch obviously has to create the infrastructure for optional checking, and the specific check that Pavel wants to add is to run parse analysis on each SQL statement in a plpgsql function. That seems to me to be a well-defined and useful check, so I think the scope of the patch is entirely adequate for now. A bigger issue is that once you think about more than one kind of check, it becomes apparent that we might need some user-specifiable options to control which checks are applied. And I see no provision for that here. This is not something we can add later, at least not without breaking the API for the check function --- and if we're willing to break API, why not just add some more parameters to the validator and avoid having a second function? On the whole, it might not be a bad idea to have two allowed signatures for the validator function, rather than inventing an additional column in pg_language. But the fundamental point IMHO is that there needs to be a provision to pass language-dependent validation options to the function, whether it's the existing validator or a separate checker entry point. 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] review: CHECK FUNCTION statement
Hello CREATE OR REPLACE FUNCTION t(i integer) RETURNS integer LANGUAGE plpgsql STRICT AS $$DECLARE j integer; BEGIN IF i=1 THEN FOR I IN 1..4 BY -1 LOOP RAISE NOTICE '%', i; END LOOP; RETURN -1; ELSE RETURN 2*i; END IF; END;$$; CHECK FUNCTION t(integer); -- no error SELECT t(1); ERROR: BY value of FOR loop must be greater than zero CONTEXT: PL/pgSQL function t line 4 at FOR with integer loop variable 2) CREATE OR REPLACE FUNCTION t(i integer) RETURNS integer LANGUAGE plpgsql STRICT AS $$DECLARE j integer; BEGIN IF i=1 THEN j=999; RETURN j; ELSE RETURN 2*i; END IF; END;$$; CHECK FUNCTION t(integer); -- no error SELECT t(1); ERROR: value 999 is out of range for type integer CONTEXT: PL/pgSQL function t line 4 at assignment This kind of check are little bit difficult. It is solveable but I would to have a skelet in core, and then this skelet can be enhanced step by step. Where is problem? PL/pgSQL usually don't work with numeric constant. Almost all numbers are expressions - and checking function ensure only semantic validity of expression, but don't try to evaluate expression. So isn't possible to check runtime errors now. 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] review: CHECK FUNCTION statement
On Wed, Nov 30, 2011 at 10:53 AM, Tom Lane t...@sss.pgh.pa.us wrote: On the whole, it might not be a bad idea to have two allowed signatures for the validator function, rather than inventing an additional column in pg_language. But the fundamental point IMHO is that there needs to be a provision to pass language-dependent validation options to the function, whether it's the existing validator or a separate checker entry point. Something like: CHECK FUNCTION proname(proargs) WITH (...fdw-style elastic options...) ? -- 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] review: CHECK FUNCTION statement
Excerpts from Tom Lane's message of mié nov 30 12:53:42 -0300 2011: A bigger issue is that once you think about more than one kind of check, it becomes apparent that we might need some user-specifiable options to control which checks are applied. And I see no provision for that here. This is not something we can add later, at least not without breaking the API for the check function --- and if we're willing to break API, why not just add some more parameters to the validator and avoid having a second function? How about CHECK (parse, names=off) FUNCTION foobar(a, b, c) -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Robert Haas robertmh...@gmail.com writes: On Wed, Nov 30, 2011 at 10:53 AM, Tom Lane t...@sss.pgh.pa.us wrote: On the whole, it might not be a bad idea to have two allowed signatures for the validator function, rather than inventing an additional column in pg_language. But the fundamental point IMHO is that there needs to be a provision to pass language-dependent validation options to the function, whether it's the existing validator or a separate checker entry point. Something like: CHECK FUNCTION proname(proargs) WITH (...fdw-style elastic options...) Great minds think alike ... that was pretty much exactly the syntax that was in the back of my mind. 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] review: CHECK FUNCTION statement
2011/11/30 Alvaro Herrera alvhe...@commandprompt.com: Excerpts from Tom Lane's message of mié nov 30 12:53:42 -0300 2011: A bigger issue is that once you think about more than one kind of check, it becomes apparent that we might need some user-specifiable options to control which checks are applied. And I see no provision for that here. This is not something we can add later, at least not without breaking the API for the check function --- and if we're willing to break API, why not just add some more parameters to the validator and avoid having a second function? How about CHECK (parse, names=off) FUNCTION foobar(a, b, c) this syntax is relative consistent with EXPLAIN, is it ok for all? Pavel -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
Pavel Stehule pavel.steh...@gmail.com writes: 2011/11/30 Alvaro Herrera alvhe...@commandprompt.com: How about CHECK (parse, names=off) FUNCTION foobar(a, b, c) this syntax is relative consistent with EXPLAIN, is it ok for all? It seems pretty awkward to me, particularly putting the options before the second keyword of the command --- that could bite us if we ever want some other flavors of CHECK command. I prefer Robert's suggestion of a WITH clause at the end. 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] review: CHECK FUNCTION statement
2011/11/30 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: 2011/11/30 Alvaro Herrera alvhe...@commandprompt.com: How about CHECK (parse, names=off) FUNCTION foobar(a, b, c) this syntax is relative consistent with EXPLAIN, is it ok for all? It seems pretty awkward to me, particularly putting the options before the second keyword of the command --- that could bite us if we ever want some other flavors of CHECK command. I prefer Robert's suggestion of a WITH clause at the end. we can provide both versions - as can be fine for people. Is is simple in parser. I like both variants and I am thinking so much more important is a API of checker function and behave of CHECK FUNCTION statement. Just idea - don't kill me :). Because CHECK FUNCTION is not destructive , then complete signature is not necessary, and when function name is unique, then parameters should be optional - it can be comfortable for manual work, so just CHECK FUNCTION name; can work. I see a usage for option - a entering parameter's values instead signature. When I started with overloaded functions, sometimes I had a problem with identification of function that was executed - CHECK FUNCTION can help CHECK FUNCTION name(10,20) WITH (values); Notice: checking function name(int, int, int default 20) I would to design API of checker function be friendly to direct call. There was some ideas to design CHECK FUNCTION for possibility to check all functions in schema or language. It should be, but we have a inline statement and system catalog, so anybody can write own scripts per your requests. And It was one point to decision for separate checker function from validate function. 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] review: CHECK FUNCTION statement
Pavel Stehule pavel.steh...@gmail.com writes: 2011/11/30 Tom Lane t...@sss.pgh.pa.us: It seems pretty awkward to me, particularly putting the options before the second keyword of the command --- that could bite us if we ever want some other flavors of CHECK command. I prefer Robert's suggestion of a WITH clause at the end. we can provide both versions - as can be fine for people. Is is simple in parser. I like both variants and I am thinking so much more important is a API of checker function and behave of CHECK FUNCTION statement. I think you missed my point: I don't want the options list at the front because I'm afraid it will prevent us from making good extensions in the future. Offering both syntaxes does not fix that. Just idea - don't kill me :). Because CHECK FUNCTION is not destructive , then complete signature is not necessary, and when function name is unique, then parameters should be optional - it can be comfortable for manual work, so just CHECK FUNCTION name; can work. Well, there was some discussion of having a bulk check or wildcard capability in the CHECK command, but this seems like an awfully constricted version of that. The thing I'd prefer to see in the first cut is some notation for check all functions owned by me that are in language FOO. The reason for the language restriction is that if we think the options are language-specific, there's no reason to believe that different validators would accept the same options. 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] review: CHECK FUNCTION statement
Hello I rechecked a possibility to use a validator function together with checker function. The main issue is a different interface of both functions. Validator needs just function oid and uses global variable check_function_bodies. Checker function needs function oid and relation oid (possible some other params). When we mix these two functions together we need a validator(oid) or validator(oid, oid, variadic any) one parameter function is old validator, three parameters function is checker. Question: What is a correct signature for this function? We cannot use a overloading, because we can have only one validator function per language. We can change a validator to support both forms, and we have to be carefully and correct if we will work with our validators(3 and more params) or foreign validators (1 param). We should to support both (compatibility reasons). We are not careful about validators now - there are some places, where one parameter is hardly expected - this should be changed. So using validator for checking doesn't mean smaller patch, but is true, so these functions has similar semantic - validator is usually low level checker. What is your opinion? 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] review: CHECK FUNCTION statement
Pavel Stehule pavel.steh...@gmail.com writes: I rechecked a possibility to use a validator function together with checker function. The main issue is a different interface of both functions. Validator needs just function oid and uses global variable check_function_bodies. Checker function needs function oid and relation oid (possible some other params). When we mix these two functions together we need a validator(oid) or validator(oid, oid, variadic any) Right, although if you want it to be callable from SQL I think that variadic any is too loose. What is a correct signature for this function? We cannot use a overloading, because we can have only one validator function per language. So? You have one validator function, it has either signature; if it has the old signature then CHECK isn't supported by the language. We have plenty of examples of this sort of thing already. One issue that would need to be considered is how the validator tells the difference between a CREATE FUNCTION call and a CHECK call with default parameters (no WITH clause). Those shouldn't do exactly the same thing, presumably. Maybe that's a sufficient reason to have two entry points. 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] review: CHECK FUNCTION statement
Pavel Stehule wrote: I am sending updated patch, that implements a CHECK FUNCTION and CHECK TRIGGER statements. This patch is significantly redesigned to previous version (PL/pgSQL part) - it is more readable, more accurate. There are new regress tests. Please, can some English native speaker fix doc and comments? ToDo: CHECK FUNCTION search function according to function signature - it should be changes for using a actual types - it can be solution for polymorphic types and useful tool for work with overloaded functions - when is not clean, that function was executed. check function foo(int, int); NOTICE: checking function foo(variadic anyarray) ... and maybe some support for named parameters check function foo(name text, surname text); NOTICE: checking function foo(text, text, text, text) ... I think that CHECK FUNCTION should work exactly like DROP FUNCTION in these respects. Submission review: -- The patch is context diff, applies with some offsets, contains regression tests and documentation. The documentation should be expanded, the doc for CHECK FUNCTION is only a stub. It should describe the procedure and what is checked. That would also make reviewing easier. I think that some documentation should be added to plhandler.sgml. There is a spelling error (statemnt) in the docs. Usability review: - If I understand right, the goal of CHECK FUNCTION is to find errors in the function definition without actually having to execute it. The patch tries to provide this for PL/pgSQL. There hasn't been any discussion on the list, the patch was just posted, so I can't say that we want that. Tom added it to the commitfest page, so there's one important voice against dismissing it right away :^) I don't understand the functional difference between a validator function and a check function as proposed by this patch. I am probably missing something, but why couldn't these checks be added to function validation when check_function_bodies is set? A new CHECK FUNCTION statement could simply call the validator function. I don't see any pg_dump support in this patch, and PL/pgSQL probably doesn't need that, but I think pg_dump support for CREATE LANGUAGE would have to be added for other PLs. I can't test if the functionality is complete because I can't get it to run (see below). Feature test: - I can't really test the patch because initdb fails: $ initdb -E UTF8 --locale=de_DE.UTF-8 --lc-messages=en_US.UTF-8 -U postgres /postgres/cvs/dbhome The files belonging to this database system will be owned by user laurenz. This user must also own the server process. The database cluster will be initialized with locales COLLATE: de_DE.UTF-8 CTYPE:de_DE.UTF-8 MESSAGES: en_US.UTF-8 MONETARY: de_DE.UTF-8 NUMERIC: de_DE.UTF-8 TIME: de_DE.UTF-8 The default text search configuration will be set to german. creating directory /postgres/cvs/dbhome ... ok creating subdirectories ... ok selecting default max_connections ... 100 selecting default shared_buffers ... 32MB creating configuration files ... ok creating template1 database in /postgres/cvs/dbhome/base/1 ... ok initializing pg_authid ... ok initializing dependencies ... ok creating system views ... ok loading system objects' descriptions ... ok creating collations ... ok creating conversions ... ok creating dictionaries ... ok setting privileges on built-in objects ... ok creating information schema ... ok loading PL/pgSQL server-side language ... FATAL: could not load library /postgres/cvs/pg92/lib/plpgsql.so: /postgres/cvs/pg92/lib/plpgsql.so: undefined symbol: plpgsql_delete_function STATEMENT: CREATE EXTENSION plpgsql; child process exited with exit code 1 initdb: removing data directory /postgres/cvs/dbhome Coding review: -- The patch compiles without warnings. The comments in the code should be revised, they are bad English. I can't say if there should be more of them -- I don't know this part of the code well enough to have a well-founded opinion. I don't think there are any portability issues, but I could not test it. There are a lot of small changes to pl/plpgsql/src/pl_exec.c, are they all necessary? For example, why was copy_plpgsql_datum renamed to plpgsql_copy_datum? I'll mark the patch as Waiting on Author. 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] review: CHECK FUNCTION statement
Hello 2011/11/29 Albe Laurenz laurenz.a...@wien.gv.at: Pavel Stehule wrote: I am sending updated patch, that implements a CHECK FUNCTION and CHECK TRIGGER statements. This patch is significantly redesigned to previous version (PL/pgSQL part) - it is more readable, more accurate. There are new regress tests. Please, can some English native speaker fix doc and comments? ToDo: CHECK FUNCTION search function according to function signature - it should be changes for using a actual types - it can be solution for polymorphic types and useful tool for work with overloaded functions - when is not clean, that function was executed. check function foo(int, int); NOTICE: checking function foo(variadic anyarray) ... and maybe some support for named parameters check function foo(name text, surname text); NOTICE: checking function foo(text, text, text, text) ... I think that CHECK FUNCTION should work exactly like DROP FUNCTION in these respects. Submission review: -- The patch is context diff, applies with some offsets, contains regression tests and documentation. The documentation should be expanded, the doc for CHECK FUNCTION is only a stub. It should describe the procedure and what is checked. That would also make reviewing easier. I think that some documentation should be added to plhandler.sgml. There is a spelling error (statemnt) in the docs. Usability review: - If I understand right, the goal of CHECK FUNCTION is to find errors in the function definition without actually having to execute it. The patch tries to provide this for PL/pgSQL. There hasn't been any discussion on the list, the patch was just posted, so I can't say that we want that. Tom added it to the commitfest page, so there's one important voice against dismissing it right away :^) This feature was transformed from plpgsql_lint contrib module. So there was a voises so this functionality should be in contrib module as minimum http://archives.postgresql.org/pgsql-hackers/2011-07/msg00900.php http://archives.postgresql.org/pgsql-hackers/2011-07/msg01035.php Contrib module has one disadvantage - it cannot be used in combination with other plpgsql extensions like edb debugger or edb profiler. So I rewrote it as core plpgsql patch. It was a plpgsql.prepare_plans feature. This idea was rejected and replaced by CHECK FUNCTION statement Tom propossed a syntax http://archives.postgresql.org/pgsql-hackers/2011-09/msg00549.php http://archives.postgresql.org/pgsql-hackers/2011-09/msg00563.php I don't understand the functional difference between a validator function and a check function as proposed by this patch. I am probably missing something, but why couldn't these checks be added to function validation when check_function_bodies is set? A new CHECK FUNCTION statement could simply call the validator function. A validation function is not simple now - and check feature increase a complexity. Other problem with validator function is their polymorphic interface. I don't see any pg_dump support in this patch, and PL/pgSQL probably doesn't need that, but I think pg_dump support for CREATE LANGUAGE would have to be added for other PLs. I have to recheck it I can't test if the functionality is complete because I can't get it to run (see below). sorry - I'll look on it Feature test: - I can't really test the patch because initdb fails: $ initdb -E UTF8 --locale=de_DE.UTF-8 --lc-messages=en_US.UTF-8 -U postgres /postgres/cvs/dbhome The files belonging to this database system will be owned by user laurenz. This user must also own the server process. The database cluster will be initialized with locales COLLATE: de_DE.UTF-8 CTYPE: de_DE.UTF-8 MESSAGES: en_US.UTF-8 MONETARY: de_DE.UTF-8 NUMERIC: de_DE.UTF-8 TIME: de_DE.UTF-8 The default text search configuration will be set to german. creating directory /postgres/cvs/dbhome ... ok creating subdirectories ... ok selecting default max_connections ... 100 selecting default shared_buffers ... 32MB creating configuration files ... ok creating template1 database in /postgres/cvs/dbhome/base/1 ... ok initializing pg_authid ... ok initializing dependencies ... ok creating system views ... ok loading system objects' descriptions ... ok creating collations ... ok creating conversions ... ok creating dictionaries ... ok setting privileges on built-in objects ... ok creating information schema ... ok loading PL/pgSQL server-side language ... FATAL: could not load library /postgres/cvs/pg92/lib/plpgsql.so: /postgres/cvs/pg92/lib/plpgsql.so: undefined symbol: plpgsql_delete_function STATEMENT: CREATE EXTENSION plpgsql; child process exited with exit code 1 initdb: removing data directory /postgres/cvs/dbhome Coding review: -- The patch compiles without warnings. The comments in the code should be revised, they are bad English. I
Re: [HACKERS] review: CHECK FUNCTION statement
Pavel Stehule pavel.steh...@gmail.com writes: 2011/11/29 Albe Laurenz laurenz.a...@wien.gv.at: There are a lot of small changes to pl/plpgsql/src/pl_exec.c, are they all necessary? For example, why was copy_plpgsql_datum renamed to plpgsql_copy_datum? yes, it's necessary - a implementation is in new file and there is necessary call a functions from pg_compile and pg_exec files - checking is between compilation and execution - so some functions should not be static now. All plpgsql public functions should start with plpgsql_ prefix. It is reason for renaming. I don't think renaming is necessary. plpgsql is a standalone shared library and so its symbols don't matter to anybody but itself. Possibly a larger question, though, is whether you really need a new source file. If that results in having to export functions that otherwise could stay static, maybe it's not the best choice. 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] review: CHECK FUNCTION statement
2011/11/29 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: 2011/11/29 Albe Laurenz laurenz.a...@wien.gv.at: There are a lot of small changes to pl/plpgsql/src/pl_exec.c, are they all necessary? For example, why was copy_plpgsql_datum renamed to plpgsql_copy_datum? yes, it's necessary - a implementation is in new file and there is necessary call a functions from pg_compile and pg_exec files - checking is between compilation and execution - so some functions should not be static now. All plpgsql public functions should start with plpgsql_ prefix. It is reason for renaming. I don't think renaming is necessary. plpgsql is a standalone shared library and so its symbols don't matter to anybody but itself. Possibly a larger question, though, is whether you really need a new source file. If that results in having to export functions that otherwise could stay static, maybe it's not the best choice. This patch was originally in pl_exec.c but this file has a 6170 lines and checking adds 1092 lines - so I moved it to new file It has little bit different semantics, but it is true, so checking hardly depends on routines from pl_exec - routines for variable's management. I have no problem to move it back. I reduces original patch little bit. Some refactoring of pl_exec should be nice - a management of row, record variables and array fields is part that can be shared with SQL/PSM interpret. But I have not idea how it realize. 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] review: CHECK FUNCTION statement
Excerpts from Pavel Stehule's message of mar nov 29 14:37:24 -0300 2011: 2011/11/29 Tom Lane t...@sss.pgh.pa.us: I don't think renaming is necessary. plpgsql is a standalone shared library and so its symbols don't matter to anybody but itself. Possibly a larger question, though, is whether you really need a new source file. If that results in having to export functions that otherwise could stay static, maybe it's not the best choice. Some refactoring of pl_exec should be nice - a management of row, record variables and array fields is part that can be shared with SQL/PSM interpret. But I have not idea how it realize. I proposed at the PL summit that perhaps we should have some sort of PL lib that would be shared by plpgsql and plpsm, to reduce code duplication. -- Á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
Re: [HACKERS] review: CHECK FUNCTION statement
2011/11/29 Pavel Stehule pavel.steh...@gmail.com: Hello updated patch: * recheck compilation and initdb * working routines moved to pl_exec.c * add entry to catalog.sgml about lanchecker field * add node's utils + pg_dump support Pavel Regards Pavel Stehule 2011/11/29 Albe Laurenz laurenz.a...@wien.gv.at: Pavel Stehule wrote: I am sending updated patch, that implements a CHECK FUNCTION and CHECK TRIGGER statements. This patch is significantly redesigned to previous version (PL/pgSQL part) - it is more readable, more accurate. There are new regress tests. Please, can some English native speaker fix doc and comments? ToDo: CHECK FUNCTION search function according to function signature - it should be changes for using a actual types - it can be solution for polymorphic types and useful tool for work with overloaded functions - when is not clean, that function was executed. check function foo(int, int); NOTICE: checking function foo(variadic anyarray) ... and maybe some support for named parameters check function foo(name text, surname text); NOTICE: checking function foo(text, text, text, text) ... I think that CHECK FUNCTION should work exactly like DROP FUNCTION in these respects. Submission review: -- The patch is context diff, applies with some offsets, contains regression tests and documentation. The documentation should be expanded, the doc for CHECK FUNCTION is only a stub. It should describe the procedure and what is checked. That would also make reviewing easier. I think that some documentation should be added to plhandler.sgml. There is a spelling error (statemnt) in the docs. Usability review: - If I understand right, the goal of CHECK FUNCTION is to find errors in the function definition without actually having to execute it. The patch tries to provide this for PL/pgSQL. There hasn't been any discussion on the list, the patch was just posted, so I can't say that we want that. Tom added it to the commitfest page, so there's one important voice against dismissing it right away :^) I don't understand the functional difference between a validator function and a check function as proposed by this patch. I am probably missing something, but why couldn't these checks be added to function validation when check_function_bodies is set? A new CHECK FUNCTION statement could simply call the validator function. I don't see any pg_dump support in this patch, and PL/pgSQL probably doesn't need that, but I think pg_dump support for CREATE LANGUAGE would have to be added for other PLs. I can't test if the functionality is complete because I can't get it to run (see below). Feature test: - I can't really test the patch because initdb fails: $ initdb -E UTF8 --locale=de_DE.UTF-8 --lc-messages=en_US.UTF-8 -U postgres /postgres/cvs/dbhome The files belonging to this database system will be owned by user laurenz. This user must also own the server process. The database cluster will be initialized with locales COLLATE: de_DE.UTF-8 CTYPE: de_DE.UTF-8 MESSAGES: en_US.UTF-8 MONETARY: de_DE.UTF-8 NUMERIC: de_DE.UTF-8 TIME: de_DE.UTF-8 The default text search configuration will be set to german. creating directory /postgres/cvs/dbhome ... ok creating subdirectories ... ok selecting default max_connections ... 100 selecting default shared_buffers ... 32MB creating configuration files ... ok creating template1 database in /postgres/cvs/dbhome/base/1 ... ok initializing pg_authid ... ok initializing dependencies ... ok creating system views ... ok loading system objects' descriptions ... ok creating collations ... ok creating conversions ... ok creating dictionaries ... ok setting privileges on built-in objects ... ok creating information schema ... ok loading PL/pgSQL server-side language ... FATAL: could not load library /postgres/cvs/pg92/lib/plpgsql.so: /postgres/cvs/pg92/lib/plpgsql.so: undefined symbol: plpgsql_delete_function STATEMENT: CREATE EXTENSION plpgsql; child process exited with exit code 1 initdb: removing data directory /postgres/cvs/dbhome Coding review: -- The patch compiles without warnings. The comments in the code should be revised, they are bad English. I can't say if there should be more of them -- I don't know this part of the code well enough to have a well-founded opinion. I don't think there are any portability issues, but I could not test it. There are a lot of small changes to pl/plpgsql/src/pl_exec.c, are they all necessary? For example, why was copy_plpgsql_datum renamed to plpgsql_copy_datum? I'll mark the patch as Waiting on Author. 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] Review: CHECK FUNCTION statement
2011/11/22 Albe Laurenz laurenz.a...@wien.gv.at: I tried to look at the patch, but it does not apply to current master, probably because of bit rot. Can you submit an updated version? The patch contains docs and regression tests and is context diff. I'll mark it waiting for author. please wait, I plan to work on this topic at thursday. Regards Pavel 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
