Re: [HACKERS] PL/PgSQL STRICT
Hi, If I'm counting correctly, we have four votes for this patch and two votes against it. Any other opinions? Regards, Marko Tiikkaja -- 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] PL/PgSQL STRICT
Marko Tiikkaja ma...@joh.to writes: If I'm counting correctly, we have four votes for this patch and two votes against it. Any other opinions? FWIW, I share Peter's poor opinion of this syntax. I can see the appeal of not having to write an explicit check of the rowcount afterwards, but that appeal is greatly weakened by the strange syntax. (IOW, if you were counting me as a + vote, that was only a vote for the concept --- on reflection I don't much like this implementation.) 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] PL/PgSQL STRICT
FWIW, I share Peter's poor opinion of this syntax. I can see the appeal of not having to write an explicit check of the rowcount afterwards, but that appeal is greatly weakened by the strange syntax. (IOW, if you were counting me as a + vote, that was only a vote for the concept --- on reflection I don't much like this implementation.) I agree with other commentators that it would be useful, but that the word STRICT should be near the word INTO, making it clear that the STRICTness is tied to the variable assignment. I do think we can deal with the more than one case once PL/pgSQL INTO actually supports assigning more than one row to a variable, which currently it doesn't. At that time, we'll just have to remember to update the code for STRICT as well. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/PgSQL STRICT
On 1/26/13 11:28 AM, Marko Tiikkaja wrote: On Fri, 21 Dec 2012 16:14:19 +0100, I wrote: I wrote a patch which allows you to add STRICT into PERFORM and INSERT/UPDATE/DELETE without specifying an INTO clause. Here's the second version of the patch, addressing the syntax issues. I think the new syntax is horribly ugly. The actual command name should always come first, not options. What will happen if people add more options along this line? I also couldn't make the grammar work with PERFORM STRICT, so I allowed STRICT SELECT instead. I don't quite understand the reason for distinguishing PERFORM and SELECT, but what you are proposing will make this even more confusing. That said, I don't quite believe in the premise for this patch to begin with. The supposed analogy is with INTO STRICT. But that is effectively a variable assignment and checks whether that assignment was performed correctly. So for scalar variables, this checks that exactly one value was returned. I'd imagine if we allowed a syntax like ... INTO (a, b, c), (d, e, f) it would check that exactly two rows were returned. So this clause basically just ensures that the run-time behavior is consistent with the appearance of the source code. What you are now proposing is that STRICT means one, but that's just an opinion. The SQL standard recognizes that updating or deleting nothing is a noteworthy condition, so I could get partially behind this patch if it just errored out when zero rows are affected, but insisting on one is just arbitrary. (Note also that elsewhere STRICT means something like not null, so the term is getting a bit overloaded.) -- 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] PL/PgSQL STRICT
On Fri, 01 Feb 2013 18:11:13 +0100, Peter Eisentraut pete...@gmx.net wrote: On 1/26/13 11:28 AM, Marko Tiikkaja wrote: Here's the second version of the patch, addressing the syntax issues. I think the new syntax is horribly ugly. The actual command name should always come first, not options. What will happen if people add more options along this line? WITH foo AS (..) SELECT ..; doesn't have the command first either. I don't really see what other plpgsql-specific options we would add.. I also couldn't make the grammar work with PERFORM STRICT, so I allowed STRICT SELECT instead. I don't quite understand the reason for distinguishing PERFORM and SELECT, but what you are proposing will make this even more confusing. That said, I don't quite believe in the premise for this patch to begin with. The supposed analogy is with INTO STRICT. But that is effectively a variable assignment and checks whether that assignment was performed correctly. So for scalar variables, this checks that exactly one value was returned. I'd imagine if we allowed a syntax like ... INTO (a, b, c), (d, e, f) it would check that exactly two rows were returned. So this clause basically just ensures that the run-time behavior is consistent with the appearance of the source code. Fine, I can see why you see it that way. What you are now proposing is that STRICT means one, but that's just an opinion. The SQL standard recognizes that updating or deleting nothing is a noteworthy condition, so I could get partially behind this patch if it just errored out when zero rows are affected, but insisting on one is just arbitrary. *shrug* To me, this makes the most sense. In my experience if you know something should be there, it's exactly one row and not one or more. Not throwing an error on more than one would make this feature a lot less useful in practice. Regards, Marko Tiikkaja -- 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] PL/PgSQL STRICT
On Fri, 21 Dec 2012 16:14:19 +0100, I wrote: I wrote a patch which allows you to add STRICT into PERFORM and INSERT/UPDATE/DELETE without specifying an INTO clause. Here's the second version of the patch, addressing the syntax issues. I also couldn't make the grammar work with PERFORM STRICT, so I allowed STRICT SELECT instead. Any feedback welcome. Regards, Marko Tiikkaja plpgsql_strict2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PL/PgSQL STRICT
Hi,
Courtesy of me, Christmas comes a bit early this year. I wrote a patch
which allows you to add STRICT into PERFORM and INSERT/UPDATE/DELETE
without specifying an INTO clause. Observe:
=# create table foo(a int);
CREATE TABLE
=# create function foof() returns void as $$ begin update strict foo set
a=a+1; end $$ language plpgsql;
CREATE FUNCTION
=# select foof();
ERROR: query returned no rows
I know everyone obviously wants this, so I will be sending another
version with regression tests and documentation later. The code is a
bit ugly at places, but I'm going to work on that too.
Regards,
Marko Tiikkaja
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
***
*** 1500,1508 static int
exec_stmt_perform(PLpgSQL_execstate *estate, PLpgSQL_stmt_perform *stmt)
{
PLpgSQL_expr *expr = stmt-expr;
(void) exec_run_select(estate, expr, 0, NULL);
! exec_set_found(estate, (estate-eval_processed != 0));
exec_eval_cleanup(estate);
return PLPGSQL_RC_OK;
--- 1500,1519
exec_stmt_perform(PLpgSQL_execstate *estate, PLpgSQL_stmt_perform *stmt)
{
PLpgSQL_expr *expr = stmt-expr;
+ uint32 n;
(void) exec_run_select(estate, expr, 0, NULL);
! n = estate-eval_processed;
! if (stmt-strict n == 0)
! ereport(ERROR,
! (errcode(ERRCODE_NO_DATA_FOUND),
!errmsg(query returned no rows)));
! else if (stmt-strict n 1)
! ereport(ERROR,
! (errcode(ERRCODE_TOO_MANY_ROWS),
!errmsg(query returned more than one row)));
!
! exec_set_found(estate, (n != 0));
exec_eval_cleanup(estate);
return PLPGSQL_RC_OK;
***
*** 3211,3217 exec_stmt_execsql(PLpgSQL_execstate *estate,
* 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;
--- 3222,3228
* forcing completion of a sequential scan. So don't do it unless we
need
* to enforce strictness.
*/
! if (stmt-into || stmt-strict)
{
if (stmt-strict || stmt-mod_stmt)
tcount = 2;
***
*** 3335,3340 exec_stmt_execsql(PLpgSQL_execstate *estate,
--- 3346,3366
exec_eval_cleanup(estate);
SPI_freetuptable(SPI_tuptable);
}
+ else if (stmt-strict)
+ {
+ /*
+* If a mod stmt specified STRICT, and the query didn't find
+* exactly one row, throw an error.
+*/
+ if (SPI_processed == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_NO_DATA_FOUND),
+errmsg(query returned no rows)));
+ else if (SPI_processed 1)
+ ereport(ERROR,
+ (errcode(ERRCODE_TOO_MANY_ROWS),
+errmsg(query returned more than one
row)));
+ }
else
{
/* If the statement returned a tuple table, complain */
*** a/src/pl/plpgsql/src/pl_funcs.c
--- b/src/pl/plpgsql/src/pl_funcs.c
***
*** 1187,1193 static void
dump_perform(PLpgSQL_stmt_perform *stmt)
{
dump_ind();
! printf(PERFORM expr = );
dump_expr(stmt-expr);
printf(\n);
}
--- 1187,1193
dump_perform(PLpgSQL_stmt_perform *stmt)
{
dump_ind();
! printf(PERFORM%s expr = , stmt-strict ? STRICT : );
dump_expr(stmt-expr);
printf(\n);
}
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
***
*** 178,183 static List*read_raise_options(void);
--- 178,184
%type expr expr_until_semi expr_until_rightbracket
%type expr expr_until_then expr_until_loop opt_expr_until_when
%type expr opt_exitcond
+ %type boolean opt_strict
%type ival assign_var foreach_slice
%type var cursor_variable
***
*** 834,847 proc_stmt : pl_block ';'
{ $$ = $1; }
;
! stmt_perform : K_PERFORM expr_until_semi
{
PLpgSQL_stmt_perform *new;
new =
palloc0(sizeof(PLpgSQL_stmt_perform));
new-cmd_type =
PLPGSQL_STMT_PERFORM;
new-lineno =
plpgsql_location_to_lineno(@1);
! new-expr = $2;
Re: [HACKERS] PL/PgSQL STRICT
Marko Tiikkaja pgm...@joh.to writes: Courtesy of me, Christmas comes a bit early this year. I wrote a patch which allows you to add STRICT into PERFORM and INSERT/UPDATE/DELETE without specifying an INTO clause. What is the use-case for this? Won't this result in the word STRICT becoming effectively reserved in contexts where it currently is not? (IOW, even if the feature is useful, I've got considerable doubts about this syntax for it. The INTO clause is an ugly, badly designed kluge already --- let's not make another one just like it.) 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] PL/PgSQL STRICT
On 12/21/12 4:39 PM, Tom Lane wrote: What is the use-case for this? Currently, the way to do this would be something like: DECLARE _ok bool; BEGIN UPDATE foo .. RETURNING TRUE INTO STRICT _ok; We have a lot of code like this, and I bet others do as well. Won't this result in the word STRICT becoming effectively reserved in contexts where it currently is not? It will, which probably is not ideal if it can be avoided. I also considered syntax like INTO STRICT NULL, but that felt a bit ugly. It would be great if someone had any smart ideas about the syntax. Regards, Marko Tiikkaja -- 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] PL/PgSQL STRICT
On 12/21/12 4:49 PM, I wrote: On 12/21/12 4:39 PM, Tom Lane wrote: What is the use-case for this? Currently, the way to do this would be something like: I realize I didn't really answer the question. The use case is when you're UPDATEing or DELETEing a row and you want to quickly assert that there should be exactly one row. For example, if you've previously locked a row with SELECT .. FOR UPDATE, and now you want to UPDATE or DELETE it, it better be there (or you have a bug somewhere). Regards, Marko Tiikkaja -- 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] PL/PgSQL STRICT
On 12/21/12 4:49 PM, I wrote: Won't this result in the word STRICT becoming effectively reserved in contexts where it currently is not? It will, which probably is not ideal if it can be avoided. I also considered syntax like INTO STRICT NULL, but that felt a bit ugly. It would be great if someone had any smart ideas about the syntax. Another idea would be to force the STRICT to be immediately after INSERT, UPDATE or DELETE. Regards, Marko Tiikkaja -- 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] PL/PgSQL STRICT
2012/12/21 Marko Tiikkaja pgm...@joh.to: On 12/21/12 4:49 PM, I wrote: On 12/21/12 4:39 PM, Tom Lane wrote: What is the use-case for this? Currently, the way to do this would be something like: I realize I didn't really answer the question. The use case is when you're UPDATEing or DELETEing a row and you want to quickly assert that there should be exactly one row. For example, if you've previously locked a row with SELECT .. FOR UPDATE, and now you want to UPDATE or DELETE it, it better be there (or you have a bug somewhere). yes, it has sense probably only after keyword it should be simple implementable Regards Pavel Regards, Marko Tiikkaja -- 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] PL/PgSQL STRICT
On Fri, Dec 21, 2012 at 10:39 AM, Tom Lane t...@sss.pgh.pa.us wrote: Marko Tiikkaja pgm...@joh.to writes: Courtesy of me, Christmas comes a bit early this year. I wrote a patch which allows you to add STRICT into PERFORM and INSERT/UPDATE/DELETE without specifying an INTO clause. What is the use-case for this? Won't this result in the word STRICT becoming effectively reserved in contexts where it currently is not? (IOW, even if the feature is useful, I've got considerable doubts about this syntax for it. The INTO clause is an ugly, badly designed kluge already --- let's not make another one just like it.) Yep, the use case for this seems mighty narrow to me. I could use GET DIAGNOSTICS to determine if nothing got altered, and it seems likely to me that expressly doing this via IF/ELSE/END IF would be easier to read in function code than a somewhat magic STRICT side-effect. I certainly appreciate that brevity can make things more readable, it's just that I'm not sure that is much of a help here. This is adding specific syntax for what seems like an unusual case to me, which seems like an unworthwhile complication. -- When confronted by a difficult problem, solve it by reducing it to the question, How would the Lone Ranger handle this?
Re: [HACKERS] PL/PgSQL STRICT
-Original Message- From: pgsql-hackers-ow...@postgresql.org [mailto:pgsql-hackers- ow...@postgresql.org] On Behalf Of Marko Tiikkaja Sent: Friday, December 21, 2012 10:53 AM To: Tom Lane Cc: PostgreSQL-development Subject: Re: [HACKERS] PL/PgSQL STRICT On 12/21/12 4:49 PM, I wrote: On 12/21/12 4:39 PM, Tom Lane wrote: What is the use-case for this? Currently, the way to do this would be something like: I realize I didn't really answer the question. The use case is when you're UPDATEing or DELETEing a row and you want to quickly assert that there should be exactly one row. For example, if you've previously locked a row with SELECT .. FOR UPDATE, and now you want to UPDATE or DELETE it, it better be there (or you have a bug somewhere). There had better be exactly one row - but who cares whether that is the row we were actually expecting to delete/update... I've recently had the experience of missing a WHERE pk = ... clause in an UPDATE statement inside a function so I do see the value in having an easy to implement safety idiom along these lines. Along the lines of EXPLAIN (options) CMD would something like UPDATE|DELETE (STRICT) identifier work? David J. -- 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] PL/PgSQL STRICT
Marko Tiikkaja pgm...@joh.to writes: Another idea would be to force the STRICT to be immediately after INSERT, UPDATE or DELETE. What about before it, ie STRICT UPDATE ... This should dodge the problem of possible conflict with table names, and it seems to me to read more naturally too. 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] PL/PgSQL STRICT
2012/12/21 Tom Lane t...@sss.pgh.pa.us: Marko Tiikkaja pgm...@joh.to writes: Another idea would be to force the STRICT to be immediately after INSERT, UPDATE or DELETE. What about before it, ie STRICT UPDATE ... This should dodge the problem of possible conflict with table names, and it seems to me to read more naturally too. +1 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 -- 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] PL/PgSQL STRICT
On 12/21/12 5:09 PM, Christopher Browne wrote: I could use GET DIAGNOSTICS to determine if nothing got altered, and it seems likely to me that expressly doing this via IF/ELSE/END IF would be easier to read in function code than a somewhat magic STRICT side-effect. STRICT is used in INTO, so PL/PgSQL users should already have an idea what it's going to do outside of INTO. I certainly appreciate that brevity can make things more readable, it's just that I'm not sure that is much of a help here. This is adding specific syntax for what seems like an unusual case to me, which seems like an unworthwhile complication. A quick grep suggests that our (the company I work for) code base has 160 occurrences of INSERT/UPDATE/DELETE followed by IF NOT FOUND THEN RAISE EXCEPTION. So it doesn't seem like an unusual case to me. Of course, some of them couldn't use STRICT because they are expected to happen (in which case they can send a more descriptive error message), but most of them could. Regards, Marko Tiikkaja -- 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] PL/PgSQL STRICT
On 12/21/12 5:22 PM, Tom Lane wrote: Marko Tiikkaja pgm...@joh.to writes: Another idea would be to force the STRICT to be immediately after INSERT, UPDATE or DELETE. What about before it, ie STRICT UPDATE ... This should dodge the problem of possible conflict with table names, and it seems to me to read more naturally too. Yeah, putting STRICT after the command wouldn't work for UPDATE. I like this one best so far, so I'm going with this syntax for the next version of the patch. Regards, Marko Tiikkaja -- 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] PL/PgSQL STRICT
On Fri, Dec 21, 2012 at 4:22 PM, Tom Lane t...@sss.pgh.pa.us wrote: What about before it, ie STRICT UPDATE ... +1 from me too. This feature would be awesome. -- 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] PL/PgSQL STRICT
Christopher Browne cbbro...@gmail.com writes: This is adding specific syntax for what seems like an unusual case to me, which seems like an unworthwhile complication. That was my first reaction too, but Marko's followon examples seem to make a reasonable case for it. There are many situations where you expect an UPDATE or DELETE to hit exactly one row. Often, programmers won't bother to add code to check that it did ... but if a one-word addition to the command can provide such a check, it seems more likely that they would add the check. 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] PL/PgSQL STRICT
2012/12/21 Tom Lane t...@sss.pgh.pa.us: Christopher Browne cbbro...@gmail.com writes: This is adding specific syntax for what seems like an unusual case to me, which seems like an unworthwhile complication. That was my first reaction too, but Marko's followon examples seem to make a reasonable case for it. There are many situations where you expect an UPDATE or DELETE to hit exactly one row. Often, programmers won't bother to add code to check that it did ... but if a one-word addition to the command can provide such a check, it seems more likely that they would add the check. and it can be used for optimization - it can be optimized for fast first row 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 -- 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] PL/PgSQL STRICT
On Fri, Dec 21, 2012 at 4:53 PM, Tom Lane t...@sss.pgh.pa.us wrote: That was my first reaction too, but Marko's followon examples seem to make a reasonable case for it. There are many situations where you expect an UPDATE or DELETE to hit exactly one row. Often, programmers won't bother to add code to check that it did ... but if a one-word addition to the command can provide such a check, it seems more likely that they would add the check. Very true. When I was a PL/PgSQL beginner a few years ago I did exactly that, I didn't check if the update actually updated any row, I didn't know it could fail, and felt extremely worried and stupid when I realised this. I spent an entire day going through all functions fixing this problem at all places. The fix was not beautiful and it bugged me there was not a prettier way to fix it. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
