Message-ID: <4E71B4C4.3010004@gmail.com>
Date: Thu, 15 Sep 2011 10:18:12 +0200
From: Yeb Havinga <yebhavinga@gmail.com>
User-Agent: Mozilla/5.0 (Windows NT 5.1;
	rv:6.0.2) Gecko/20110902 Thunderbird/6.0.2
MIME-Version: 1.0
To: pgsql-hackers@postgreSQL.org
Subject: Patch for cursor calling with named parameters
Content-Type: multipart/mixed; boundary="------------080009040102060304040000"

This is a multi-part message in MIME format.
--------------080009040102060304040000
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hello list,

The following patch implements cursor calling with named parameters in 
addition to the standard positional argument lists.

c1 cursor (param1 int, param2 int) for select * from rc_test where a > 
param1 and b > param2;
open c1($1, $2);                     -- this is currently possible
open c1(param2 := $2, param1 := $1); -- this is the new feature

Especially for cursors with a lot of arguments, this increases 
readability of code. This was discussed previously in 
http://archives.postgresql.org/pgsql-hackers/2010-09/msg01433.php. We 
actually made two patches: one with => and then one with := notation. 
Attached is the patch with := notation.

Is it ok to add it to the next commitfest?

regards,
Yeb Havinga, Willem Dijkstra

-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data


--------------080009040102060304040000
Content-Type: text/x-patch;
 name="cursornamedparameter-v1.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="cursornamedparameter-v1.patch"

diff --git a/src/pl/plpgsql/src/gram.y b/src/pl/plpgsql/src/gram.y
new file mode 100644
index 92b54dd..192f278
*** a/src/pl/plpgsql/src/gram.y
--- b/src/pl/plpgsql/src/gram.y
*************** read_sql_expression(int until, const cha
*** 2335,2340 ****
--- 2335,2352 ----
  							  "SELECT ", true, true, NULL, NULL);
  }
  
+ /*
+  * Convenience routine to read a single unchecked expression with two possible
+  * terminators, returning an expression with an empty sql prefix.
+  */
+ static PLpgSQL_expr *
+ read_sql_one_expression(int until, int until2, const char *expected,
+ 						int *endtoken)
+ {
+ 	return read_sql_construct(until, until2, 0, expected,
+ 							  "", true, false, NULL, endtoken);
+ }
+ 
  /* Convenience routine to read an expression with two possible terminators */
  static PLpgSQL_expr *
  read_sql_expression2(int until, int until2, const char *expected,
*************** check_labels(const char *start_label, co
*** 3384,3399 ****
  /*
   * Read the arguments (if any) for a cursor, followed by the until token
   *
!  * If cursor has no args, just swallow the until token and return NULL.
!  * If it does have args, we expect to see "( expr [, expr ...] )" followed
!  * by the until token.  Consume all that and return a SELECT query that
!  * evaluates the expression(s) (without the outer parens).
   */
  static PLpgSQL_expr *
  read_cursor_args(PLpgSQL_var *cursor, int until, const char *expected)
  {
  	PLpgSQL_expr *expr;
! 	int			tok;
  
  	tok = yylex();
  	if (cursor->cursor_explicit_argrow < 0)
--- 3396,3418 ----
  /*
   * Read the arguments (if any) for a cursor, followed by the until token
   *
!  * If cursor has no args, just swallow the until token and return NULL.  If it
!  * does have args, we expect to see "( expr [, expr ...] )" followed by the
!  * until token, where expr may be a plain expression, or a named parameter
!  * assignment of the form IDENT := expr. Consume all that and return a SELECT
!  * query that evaluates the expression(s) (without the outer parens).
   */
  static PLpgSQL_expr *
  read_cursor_args(PLpgSQL_var *cursor, int until, const char *expected)
  {
  	PLpgSQL_expr *expr;
! 	PLpgSQL_row *row;
! 	int tok;
! 	int argc = 0;
! 	char **argv;
! 	StringInfoData ds;
! 	char *sqlstart = "SELECT ";
! 	int startlocation = yylloc;
  
  	tok = yylex();
  	if (cursor->cursor_explicit_argrow < 0)
*************** read_cursor_args(PLpgSQL_var *cursor, in
*** 3412,3417 ****
--- 3431,3439 ----
  		return NULL;
  	}
  
+ 	row = (PLpgSQL_row *) plpgsql_Datums[cursor->cursor_explicit_argrow];
+ 	argv = (char **) palloc0(sizeof(char *) * row->nfields);
+ 
  	/* Else better provide arguments */
  	if (tok != '(')
  		ereport(ERROR,
*************** read_cursor_args(PLpgSQL_var *cursor, in
*** 3420,3429 ****
  						cursor->refname),
  				 parser_errposition(yylloc)));
  
! 	/*
! 	 * Read expressions until the matching ')'.
! 	 */
! 	expr = read_sql_expression(')', ")");
  
  	/* Next we'd better find the until token */
  	tok = yylex();
--- 3442,3527 ----
  						cursor->refname),
  				 parser_errposition(yylloc)));
  
! 	for (argc = 0; argc < row->nfields; argc++)
! 	{
! 		int argpos;
! 		int endtoken;
! 		PLpgSQL_expr *item;
! 
! 		if (plpgsql_isidentassign())
! 		{
! 			/* Named parameter assignment */
! 			for (argpos = 0; argpos < row->nfields; argpos++)
! 				if (strncmp(row->fieldnames[argpos], yylval.str, strlen(row->fieldnames[argpos])) == 0)
! 					break;
! 
! 			if (argpos == row->nfields)
! 				ereport(ERROR,
! 						(errcode(ERRCODE_SYNTAX_ERROR),
! 						 errmsg("cursor \"%s\" has no argument named \"%s\"",
! 								cursor->refname, yylval.str),
! 						 parser_errposition(yylloc)));
! 		}
! 		else
! 		{
! 			/* Positional parameter assignment */
! 			argpos = argc;
! 		}
! 
! 		/*
! 		 * Read one expression at a time until the matching endtoken. Checking
! 		 * the expressions is postponed until the positional argument list is
! 		 * made.
! 		 */
! 		item = read_sql_one_expression(',', ')', ",\" or \")", &endtoken);
! 
! 		if (endtoken == ')' && !(argc == row->nfields - 1))
! 			ereport(ERROR,
! 					(errcode(ERRCODE_SYNTAX_ERROR),
! 					 errmsg("not enough arguments for cursor \"%s\"",
! 							cursor->refname),
! 					 parser_errposition(yylloc)));
! 
! 		if (endtoken == ',' && (argc == row->nfields - 1))
! 			ereport(ERROR,
! 					(errcode(ERRCODE_SYNTAX_ERROR),
! 					 errmsg("too many arguments for cursor \"%s\"",
! 							cursor->refname),
! 					 parser_errposition(yylloc)));
! 
! 		if (argv[argpos] != NULL)
! 			ereport(ERROR,
! 					(errcode(ERRCODE_SYNTAX_ERROR),
! 					 errmsg("cursor \"%s\" argument %d \"%s\" provided multiple times",
! 							cursor->refname, argpos + 1, row->fieldnames[argpos]),
! 					 parser_errposition(yylloc)));
! 
! 		argv[argpos] = item->query;
! 	}
! 
! 	/* Make positional argument list */
! 	initStringInfo(&ds);
! 	appendStringInfoString(&ds, sqlstart);
! 	for (argc = 0; argc < row->nfields; argc++)
! 	{
! 		Assert(argv[argc] != NULL);
! 		appendStringInfoString(&ds, argv[argc]);
! 
! 		if (argc < row->nfields - 1)
! 			appendStringInfoString(&ds, "\n,"); /* use newline to end possible -- comment in arg */
! 	}
! 	appendStringInfoChar(&ds, ';');
! 
! 	expr = palloc0(sizeof(PLpgSQL_expr));
! 	expr->dtype			= PLPGSQL_DTYPE_EXPR;
! 	expr->query			= pstrdup(ds.data);
! 	expr->plan			= NULL;
! 	expr->paramnos		= NULL;
! 	expr->ns            = plpgsql_ns_top();
! 	pfree(ds.data);
! 
! 	/* Check if sql is valid */
! 	check_sql_expr(expr->query, startlocation, strlen(sqlstart));
  
  	/* Next we'd better find the until token */
  	tok = yylex();
diff --git a/src/pl/plpgsql/src/pl_scanner.c b/src/pl/plpgsql/src/pl_scanner.c
new file mode 100644
index 76e8436..9c233c4
*** a/src/pl/plpgsql/src/pl_scanner.c
--- b/src/pl/plpgsql/src/pl_scanner.c
*************** plpgsql_scanner_finish(void)
*** 583,585 ****
--- 583,617 ----
  	yyscanner = NULL;
  	scanorig = NULL;
  }
+ 
+ /*
+  * Return true if 'IDENT' ':=' are the next two tokens
+  */
+ bool
+ plpgsql_isidentassign(void)
+ {
+ 	int tok1, tok2;
+ 	TokenAuxData aux1, aux2;
+ 	bool result = false;
+ 
+ 	tok1 = internal_yylex(&aux1);
+ 	if (tok1 == IDENT)
+ 	{
+ 		tok2 = internal_yylex(&aux2);
+ 
+ 		if (tok2 == COLON_EQUALS)
+ 			result = true;
+ 		else
+ 			push_back_token(tok2, &aux2);
+ 	}
+ 
+ 	if (!result)
+ 		push_back_token(tok1, &aux1);
+ 
+ 	plpgsql_yylval = aux1.lval;
+ 	plpgsql_yylloc = aux1.lloc;
+ 	plpgsql_yyleng = aux1.leng;
+ 
+ 	return result;
+ }
+ 
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
new file mode 100644
index ed8fcad..2ecb82f
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
*************** extern int	plpgsql_location_to_lineno(in
*** 950,955 ****
--- 950,956 ----
  extern int	plpgsql_latest_lineno(void);
  extern void plpgsql_scanner_init(const char *str);
  extern void plpgsql_scanner_finish(void);
+ extern bool plpgsql_isidentassign(void);
  
  /* ----------
   * Externs in gram.y
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
new file mode 100644
index bed34c8..bf6fba6
*** a/src/test/regress/expected/plpgsql.out
--- b/src/test/regress/expected/plpgsql.out
*************** select refcursor_test2(20000, 20000) as
*** 2292,2297 ****
--- 2292,2412 ----
  (1 row)
  
  --
+ -- tests for cursors with named parameter arguments
+ --
+ create function namedparmcursor_test1(int, int) returns boolean as $$
+ declare
+     c1 cursor (param1 int, param2 int) for select * from rc_test where a > param1 and b > param2;
+     nonsense record;
+ begin
+     open c1(param2 := $2, -- command after ,
+             param1 := $1);
+     fetch c1 into nonsense;
+     close c1;
+     if found then
+         return true;
+     else
+         return false;
+     end if;
+ end
+ $$ language plpgsql;
+ select namedparmcursor_test1(20000, 20000) as "Should be false",
+        namedparmcursor_test1(20, 20) as "Should be true";
+  Should be false | Should be true 
+ -----------------+----------------
+  f               | t
+ (1 row)
+ 
+ create function namedparmcursor_test2(int, int) returns boolean as $$
+ declare
+     c1 cursor (param1 int, param2 int) for select * from rc_test where a > param1 and b > param2;
+     nonsense record;
+ begin
+     open c1(param1 := $1, $2);
+     fetch c1 into nonsense;
+     close c1;
+     if found then
+         return true;
+     else
+         return false;
+     end if;
+ end
+ $$ language plpgsql;
+ select namedparmcursor_test2(20000, 20000) as "Should be false",
+        namedparmcursor_test2(20, 20) as "Should be true";
+  Should be false | Should be true 
+ -----------------+----------------
+  f               | t
+ (1 row)
+ 
+ create function namedparmcursor_test3(int, int) returns boolean as $$
+ declare
+     c1 cursor (param1 int, param2 int) for select * from rc_test where a > param1 and b > param2;
+     nonsense record;
+ begin
+     open c1(param1 := $1 -- comment before ,
+             , $2);
+     fetch c1 into nonsense;
+     close c1;
+     if found then
+         return true;
+     else
+         return false;
+     end if;
+ end
+ $$ language plpgsql;
+ select namedparmcursor_test3(20000, 20000) as "Should be false",
+        namedparmcursor_test3(20, 20) as "Should be true";
+  Should be false | Should be true 
+ -----------------+----------------
+  f               | t
+ (1 row)
+ 
+ -- should fail
+ create function namedparmcursor_test4(int, int) returns void as $$
+ declare
+     c1 cursor (param1 int, param2 int) for select * from rc_test where a > param1 and b > param2;
+ begin
+     open c1($1, param1 := $1);
+ end
+ $$ language plpgsql;
+ ERROR:  cursor "c1" argument 1 "param1" provided multiple times
+ LINE 5:     open c1($1, param1 := $1);
+                                     ^
+ -- should fail
+ create function namedparmcursor_test5(int, int) returns void as $$
+ declare
+     c1 cursor (param1 int, param2 int) for select * from rc_test where a > param1 and b > param2;
+ begin
+     open c1(param2 := $2, param1 := $1, 3);
+ end
+ $$ language plpgsql;
+ ERROR:  too many arguments for cursor "c1"
+ LINE 5:     open c1(param2 := $2, param1 := $1, 3);
+                                               ^
+ -- should fail
+ create function namedparmcursor_test6(int, int) returns void as $$
+ declare
+     c1 cursor (param1 int, param2 int) for select * from rc_test where a > param1 and b > param2;
+ begin
+     open c1(param2 := $2);
+ end
+ $$ language plpgsql;
+ ERROR:  not enough arguments for cursor "c1"
+ LINE 5:     open c1(param2 := $2);
+                                 ^
+ -- should fail
+ create function namedparmcursor_test7(int, int) returns void as $$
+ declare
+     c1 cursor for select * from rc_test;
+ begin
+     open c1(param1 := $1);
+ end
+ $$ language plpgsql;
+ ERROR:  cursor "c1" has no arguments
+ LINE 5:     open c1(param1 := $1);
+                    ^
+ --
  -- tests for "raise" processing
  --
  create function raise_test1(int) returns int as $$
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
new file mode 100644
index 05f0315..59db90f
*** a/src/test/regress/sql/plpgsql.sql
--- b/src/test/regress/sql/plpgsql.sql
*************** select refcursor_test2(20000, 20000) as
*** 1946,1951 ****
--- 1946,2049 ----
         refcursor_test2(20, 20) as "Should be true";
  
  --
+ -- tests for cursors with named parameter arguments
+ --
+ create function namedparmcursor_test1(int, int) returns boolean as $$
+ declare
+     c1 cursor (param1 int, param2 int) for select * from rc_test where a > param1 and b > param2;
+     nonsense record;
+ begin
+     open c1(param2 := $2, -- command after ,
+             param1 := $1);
+     fetch c1 into nonsense;
+     close c1;
+     if found then
+         return true;
+     else
+         return false;
+     end if;
+ end
+ $$ language plpgsql;
+ 
+ select namedparmcursor_test1(20000, 20000) as "Should be false",
+        namedparmcursor_test1(20, 20) as "Should be true";
+ 
+ create function namedparmcursor_test2(int, int) returns boolean as $$
+ declare
+     c1 cursor (param1 int, param2 int) for select * from rc_test where a > param1 and b > param2;
+     nonsense record;
+ begin
+     open c1(param1 := $1, $2);
+     fetch c1 into nonsense;
+     close c1;
+     if found then
+         return true;
+     else
+         return false;
+     end if;
+ end
+ $$ language plpgsql;
+ 
+ select namedparmcursor_test2(20000, 20000) as "Should be false",
+        namedparmcursor_test2(20, 20) as "Should be true";
+ 
+ create function namedparmcursor_test3(int, int) returns boolean as $$
+ declare
+     c1 cursor (param1 int, param2 int) for select * from rc_test where a > param1 and b > param2;
+     nonsense record;
+ begin
+     open c1(param1 := $1 -- comment before ,
+             , $2);
+     fetch c1 into nonsense;
+     close c1;
+     if found then
+         return true;
+     else
+         return false;
+     end if;
+ end
+ $$ language plpgsql;
+ 
+ select namedparmcursor_test3(20000, 20000) as "Should be false",
+        namedparmcursor_test3(20, 20) as "Should be true";
+ 
+ -- should fail
+ create function namedparmcursor_test4(int, int) returns void as $$
+ declare
+     c1 cursor (param1 int, param2 int) for select * from rc_test where a > param1 and b > param2;
+ begin
+     open c1($1, param1 := $1);
+ end
+ $$ language plpgsql;
+ 
+ -- should fail
+ create function namedparmcursor_test5(int, int) returns void as $$
+ declare
+     c1 cursor (param1 int, param2 int) for select * from rc_test where a > param1 and b > param2;
+ begin
+     open c1(param2 := $2, param1 := $1, 3);
+ end
+ $$ language plpgsql;
+ 
+ -- should fail
+ create function namedparmcursor_test6(int, int) returns void as $$
+ declare
+     c1 cursor (param1 int, param2 int) for select * from rc_test where a > param1 and b > param2;
+ begin
+     open c1(param2 := $2);
+ end
+ $$ language plpgsql;
+ 
+ -- should fail
+ create function namedparmcursor_test7(int, int) returns void as $$
+ declare
+     c1 cursor for select * from rc_test;
+ begin
+     open c1(param1 := $1);
+ end
+ $$ language plpgsql;
+ 
+ --
  -- tests for "raise" processing
  --
  create function raise_test1(int) returns int as $$

--------------080009040102060304040000--