Hi,
After my previous suggestion for adding a STRICT keyword got shot
down[1], I've been thinking about an idea Andrew Gierth tossed out:
adding a new "strict mode" into PL/PgSQL. In this mode, any query which
processes more than one row will raise an exception. This is a bit
similar to specifying INTO STRICT .. for every statement, except
processing no rows does not trigger the exception. The need for this
mode comes from a few observations I make almost every day:
1) The majority of statements only deal with exactly 0 or 1 rows.
2) Checking row_count for a statement is ugly and cumbersome, so
often it just isn't checked. I often use RETURNING TRUE INTO STRICT _OK
for DML, but that a) requires an extra variable, and b) isn't possible
if 0 rows affected is not an error in the application logic.
3) SELECT .. INTO only fetches one row and ignores the rest. Even
row_count is always set to 0 or 1, so there's no way to fetch a value
*and* to check that there would not have been more rows. This creates
bugs which make your queries return wrong results and which could go
undetected for a long time.
Attached is a proof-of-concept patch (no docs, probably some actual code
problems too) to implement this as a compile option:
=# create or replace function footest() returns void as $$
$# #strict_mode strict
$# begin
$# -- not allowed to delete more than one row
$# delete from foo where f1 < 100;
$# end$$ language plpgsql;
CREATE FUNCTION
=# select footest();
ERROR: query processed more than one row
CONTEXT: PL/pgSQL function footest() line 5 at SQL statement
Now while I think this is a step into the right direction, I do have a
couple of problems with this patch:
1) I'm not sure what would be the correct behaviour with EXECUTE.
I'm tempted to just leave EXECUTE alone, as it has slightly different
rules anyway.
2) If you're running in strict mode and you want to
insert/update/delete more than one row, things get a bit uglier; a wCTE
would work for some cases. If strict mode doesn't affect EXECUTE (see
point 1 above), that could work too. Or maybe there could be a new
command which runs a query, discards the results and ignores the number
of rows processed.
I'll be adding this to the open commitfest in hopes of getting some
feedback on this idea (I'm prepared to hear a lot of "you're crazy!"),
but feel free to comment away any time you please.
Regards,
Marko Tiikkaja
[1]: http://www.postgresql.org/message-id/510bf731.5020...@gmx.net
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
***************
*** 351,356 **** do_compile(FunctionCallInfo fcinfo,
--- 351,357 ----
function->fn_cxt = func_cxt;
function->out_param_varno = -1; /* set up for no OUT param */
function->resolve_option = plpgsql_variable_conflict;
+ function->strict_mode = plpgsql_strict_mode;
if (is_dml_trigger)
function->fn_is_trigger = PLPGSQL_DML_TRIGGER;
***************
*** 847,852 **** plpgsql_compile_inline(char *proc_source)
--- 848,854 ----
function->fn_cxt = func_cxt;
function->out_param_varno = -1; /* set up for no OUT param */
function->resolve_option = plpgsql_variable_conflict;
+ function->strict_mode = plpgsql_strict_mode;
plpgsql_ns_init();
plpgsql_ns_push(func_name);
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
***************
*** 3238,3243 **** exec_stmt_execsql(PLpgSQL_execstate *estate,
--- 3238,3244 ----
ParamListInfo paramLI;
long tcount;
int rc;
+ bool strict_mode = estate->func->strict_mode;
PLpgSQL_expr *expr = stmt->sqlstmt;
/*
***************
*** 3278,3293 **** exec_stmt_execsql(PLpgSQL_execstate *estate,
/*
* If we have INTO, then we only need one row back ... but if we have
INTO
! * STRICT, ask for two rows, so that we can verify the statement returns
! * only one. INSERT/UPDATE/DELETE are always treated strictly. Without
! * INTO, just run the statement to completion (tcount = 0).
*
* We could just ask for two rows always when using INTO, but there are
* some cases where demanding the extra row costs significant time, eg
by
* forcing completion of a sequential scan. So don't do it unless we
need
* to enforce strictness.
*/
! if (stmt->into)
{
if (stmt->strict || stmt->mod_stmt)
tcount = 2;
--- 3279,3297 ----
/*
* If we have INTO, then we only need one row back ... but if we have
INTO
! * STRICT or we're in strict mode, ask for two rows, so that we can
verify
! * the statement returns only one. INSERT/UPDATE/DELETE are always
treated
! * strictly. Without INTO, just run the statement to completion
! * (tcount = 0).
*
* We could just ask for two rows always when using INTO, but there are
* some cases where demanding the extra row costs significant time, eg
by
* forcing completion of a sequential scan. So don't do it unless we
need
* to enforce strictness.
*/
! if (strict_mode)
! tcount = 2;
! else if (stmt->into)
{
if (stmt->strict || stmt->mod_stmt)
tcount = 2;
***************
*** 3399,3405 **** exec_stmt_execsql(PLpgSQL_execstate *estate,
}
else
{
! if (n > 1 && (stmt->strict || stmt->mod_stmt))
ereport(ERROR,
(errcode(ERRCODE_TOO_MANY_ROWS),
errmsg("query returned more
than one row")));
--- 3403,3409 ----
}
else
{
! if (n > 1 && (strict_mode || stmt->strict ||
stmt->mod_stmt))
ereport(ERROR,
(errcode(ERRCODE_TOO_MANY_ROWS),
errmsg("query returned more
than one row")));
***************
*** 3419,3424 **** exec_stmt_execsql(PLpgSQL_execstate *estate,
--- 3423,3433 ----
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("query has no destination for
result data"),
(rc == SPI_OK_SELECT) ? errhint("If
you want to discard the results of a SELECT, use PERFORM instead.") : 0));
+
+ if (SPI_processed > 1 && strict_mode)
+ ereport(ERROR,
+ (errcode(ERRCODE_TOO_MANY_ROWS),
+ errmsg("query processed more than one
row")));
}
if (paramLI)
***************
*** 3442,3447 **** exec_stmt_dynexecute(PLpgSQL_execstate *estate,
--- 3451,3457 ----
Oid restype;
char *querystr;
int exec_res;
+ bool strict_mode = estate->func->strict_mode;
/*
* First we evaluate the string expression after the EXECUTE keyword.
Its
***************
*** 3559,3566 **** exec_stmt_dynexecute(PLpgSQL_execstate *estate,
/*
* If SELECT ... INTO specified STRICT, and the query didn't
find
! * exactly one row, throw an error. If STRICT was not
specified, then
! * allow the query to find any number of rows.
*/
if (n == 0)
{
--- 3569,3578 ----
/*
* If SELECT ... INTO specified STRICT, and the query didn't
find
! * exactly one row, throw an error. Also if we're in strict
mode and
! * the query found more than one row, we need to report a
problem. If
! * STRICT was not specified, then allow the query to find any
number of
! * rows.
*/
if (n == 0)
{
***************
*** 3573,3579 **** exec_stmt_dynexecute(PLpgSQL_execstate *estate,
}
else
{
! if (n > 1 && stmt->strict)
ereport(ERROR,
(errcode(ERRCODE_TOO_MANY_ROWS),
errmsg("query returned more
than one row")));
--- 3585,3591 ----
}
else
{
! if (n > 1 && (strict_mode || stmt->strict))
ereport(ERROR,
(errcode(ERRCODE_TOO_MANY_ROWS),
errmsg("query returned more
than one row")));
***************
*** 3590,3595 **** exec_stmt_dynexecute(PLpgSQL_execstate *estate,
--- 3602,3611 ----
* tuples that are being ignored, but historically we have not
done
* that.
*/
+ if (SPI_processed > 1 && strict_mode)
+ ereport(ERROR,
+ (errcode(ERRCODE_TOO_MANY_ROWS),
+ errmsg("query processed more than one
row")));
}
/* Release any result from SPI_execute, as well as the querystring */
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
***************
*** 296,301 **** static List *read_raise_options(void);
--- 296,302 ----
%token <keyword> K_MOVE
%token <keyword> K_NEXT
%token <keyword> K_NO
+ %token <keyword> K_NONSTRICT
%token <keyword> K_NOT
%token <keyword> K_NOTICE
%token <keyword> K_NULL
***************
*** 325,330 **** static List *read_raise_options(void);
--- 326,332 ----
%token <keyword> K_SQLSTATE
%token <keyword> K_STACKED
%token <keyword> K_STRICT
+ %token <keyword> K_STRICT_MODE
%token <keyword> K_TABLE
%token <keyword> K_TABLE_NAME
%token <keyword> K_THEN
***************
*** 354,359 **** comp_option : '#' K_OPTION K_DUMP
--- 356,369 ----
{
plpgsql_DumpExecTree = true;
}
+ | '#' K_STRICT_MODE K_STRICT
+ {
+
plpgsql_curr_compile->strict_mode = true;
+ }
+ | '#' K_STRICT_MODE K_NONSTRICT
+ {
+
plpgsql_curr_compile->strict_mode = false;
+ }
| '#' K_VARIABLE_CONFLICT K_ERROR
{
plpgsql_curr_compile->resolve_option = PLPGSQL_RESOLVE_ERROR;
***************
*** 2313,2318 **** unreserved_keyword :
--- 2323,2329 ----
| K_SLICE
| K_SQLSTATE
| K_STACKED
+ | K_STRICT_MODE
| K_TABLE
| K_TABLE_NAME
| K_TYPE
*** a/src/pl/plpgsql/src/pl_handler.c
--- b/src/pl/plpgsql/src/pl_handler.c
***************
*** 37,42 **** static const struct config_enum_entry
variable_conflict_options[] = {
--- 37,44 ----
int plpgsql_variable_conflict = PLPGSQL_RESOLVE_ERROR;
+ bool plpgsql_strict_mode = false;
+
/* Hook for plugins */
PLpgSQL_plugin **plugin_ptr = NULL;
*** a/src/pl/plpgsql/src/pl_scanner.c
--- b/src/pl/plpgsql/src/pl_scanner.c
***************
*** 133,138 **** static const ScanKeyword unreserved_keywords[] = {
--- 133,139 ----
PG_KEYWORD("message_text", K_MESSAGE_TEXT, UNRESERVED_KEYWORD)
PG_KEYWORD("next", K_NEXT, UNRESERVED_KEYWORD)
PG_KEYWORD("no", K_NO, UNRESERVED_KEYWORD)
+ PG_KEYWORD("nonstrict", K_NONSTRICT, UNRESERVED_KEYWORD)
PG_KEYWORD("notice", K_NOTICE, UNRESERVED_KEYWORD)
PG_KEYWORD("option", K_OPTION, UNRESERVED_KEYWORD)
PG_KEYWORD("pg_context", K_PG_CONTEXT, UNRESERVED_KEYWORD)
***************
*** 154,159 **** static const ScanKeyword unreserved_keywords[] = {
--- 155,161 ----
PG_KEYWORD("slice", K_SLICE, UNRESERVED_KEYWORD)
PG_KEYWORD("sqlstate", K_SQLSTATE, UNRESERVED_KEYWORD)
PG_KEYWORD("stacked", K_STACKED, UNRESERVED_KEYWORD)
+ PG_KEYWORD("strict_mode", K_STRICT_MODE, UNRESERVED_KEYWORD)
PG_KEYWORD("table", K_TABLE, UNRESERVED_KEYWORD)
PG_KEYWORD("table_name", K_TABLE_NAME, UNRESERVED_KEYWORD)
PG_KEYWORD("type", K_TYPE, UNRESERVED_KEYWORD)
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
***************
*** 737,742 **** typedef struct PLpgSQL_function
--- 737,744 ----
PLpgSQL_resolve_option resolve_option;
+ bool strict_mode;
+
int ndatums;
PLpgSQL_datum **datums;
PLpgSQL_stmt_block *action;
***************
*** 873,878 **** extern IdentifierLookup plpgsql_IdentifierLookup;
--- 875,882 ----
extern int plpgsql_variable_conflict;
+ extern bool plpgsql_strict_mode;
+
extern bool plpgsql_check_syntax;
extern bool plpgsql_DumpExecTree;
*** a/src/test/regress/expected/plpgsql.out
--- b/src/test/regress/expected/plpgsql.out
***************
*** 3104,3109 **** select footest();
--- 3104,3170 ----
ERROR: query returned more than one row
CONTEXT: PL/pgSQL function footest() line 5 at EXECUTE statement
drop function footest();
+ -- Test "strict mode" where no query should return or process more
+ -- than one row.
+ create or replace function footest() returns void as $$
+ #strict_mode strict
+ declare x record;
+ begin
+ -- only one row, should work
+ insert into foo values(5,6) returning * into x;
+ -- no rows, should work
+ insert into foo select 1,1 where false;
+ -- perform is unaffected
+ perform 1 from foo;
+ -- only one row
+ select * from foo into x limit 1;
+ raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
+ end$$ language plpgsql;
+ select footest();
+ NOTICE: x.f1 = 1, x.f2 = 2
+ footest
+ ---------
+
+ (1 row)
+
+ create or replace function footest() returns void as $$
+ #strict_mode strict
+ declare x record;
+ begin
+ -- not allowed to return more than one row
+ select * from foo into x where f1 < 100;
+ raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
+ end$$ language plpgsql;
+ select footest();
+ ERROR: query returned more than one row
+ CONTEXT: PL/pgSQL function footest() line 6 at SQL statement
+ create or replace function footest() returns void as $$
+ #strict_mode strict
+ begin
+ -- not allowed to insert more than one row
+ insert into foo values (1,1), (2,2);
+ end$$ language plpgsql;
+ select footest();
+ ERROR: query processed more than one row
+ CONTEXT: PL/pgSQL function footest() line 5 at SQL statement
+ create or replace function footest() returns void as $$
+ #strict_mode strict
+ begin
+ -- not allowed to update more than one row
+ update foo set f1=0 where f1 < 100;
+ end$$ language plpgsql;
+ select footest();
+ ERROR: query processed more than one row
+ CONTEXT: PL/pgSQL function footest() line 5 at SQL statement
+ create or replace function footest() returns void as $$
+ #strict_mode strict
+ begin
+ -- not allowed to delete more than one row
+ delete from foo where f1 < 100;
+ end$$ language plpgsql;
+ select footest();
+ ERROR: query processed more than one row
+ CONTEXT: PL/pgSQL function footest() line 5 at SQL statement
-- test scrollable cursor support
create function sc_test() returns setof integer as $$
declare
*** a/src/test/regress/sql/plpgsql.sql
--- b/src/test/regress/sql/plpgsql.sql
***************
*** 2587,2592 **** select footest();
--- 2587,2650 ----
drop function footest();
+ -- Test "strict mode" where no query should return or process more
+ -- than one row.
+
+ create or replace function footest() returns void as $$
+ #strict_mode strict
+ declare x record;
+ begin
+ -- only one row, should work
+ insert into foo values(5,6) returning * into x;
+ -- no rows, should work
+ insert into foo select 1,1 where false;
+ -- perform is unaffected
+ perform 1 from foo;
+ -- only one row
+ select * from foo into x limit 1;
+ raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
+ end$$ language plpgsql;
+
+ select footest();
+
+ create or replace function footest() returns void as $$
+ #strict_mode strict
+ declare x record;
+ begin
+ -- not allowed to return more than one row
+ select * from foo into x where f1 < 100;
+ raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
+ end$$ language plpgsql;
+
+ select footest();
+
+ create or replace function footest() returns void as $$
+ #strict_mode strict
+ begin
+ -- not allowed to insert more than one row
+ insert into foo values (1,1), (2,2);
+ end$$ language plpgsql;
+
+ select footest();
+
+ create or replace function footest() returns void as $$
+ #strict_mode strict
+ begin
+ -- not allowed to update more than one row
+ update foo set f1=0 where f1 < 100;
+ end$$ language plpgsql;
+
+ select footest();
+
+ create or replace function footest() returns void as $$
+ #strict_mode strict
+ begin
+ -- not allowed to delete more than one row
+ delete from foo where f1 < 100;
+ end$$ language plpgsql;
+
+ select footest();
+
-- test scrollable cursor support
create function sc_test() returns setof integer as $$
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
