Code review for UPDATE SET (columnlist) patch. Make it handle as much

of the syntax as this fundamentally dead-end approach can, in particular
combinations of single and multi column assignments.  Improve rather
inadequate documentation and provide some regression tests.

4 files changed, 195 insertions, 141 deletions

diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 7b5d17a1fc8..ec2200f3bc9 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -1,5 +1,5 @@
 <!--
-$PostgreSQL: pgsql/doc/src/sgml/ref/update.sgml,v 1.39 2006/09/02 20:34:47 momjian Exp $
+$PostgreSQL: pgsql/doc/src/sgml/ref/update.sgml,v 1.40 2006/09/03 22:37:05 tgl Exp $
 PostgreSQL documentation
 -->
 
@@ -21,8 +21,8 @@ PostgreSQL documentation
  <refsynopsisdiv>
 <synopsis>
 UPDATE [ ONLY ] <replaceable class="PARAMETER">table</replaceable> [ [ AS ] <replaceable class="parameter">alias</replaceable> ]
-    [ SET <replaceable class="PARAMETER">column</replaceable> = { <replaceable class="PARAMETER">expression</replaceable> | DEFAULT } [, ...] |
-      SET ( <replaceable class="PARAMETER">column</replaceable> [, ...] ) = ( { <replaceable class="PARAMETER">expression</replaceable> | DEFAULT } [, ...] ) [, ...] ]
+    SET { <replaceable class="PARAMETER">column</replaceable> = { <replaceable class="PARAMETER">expression</replaceable> | DEFAULT } |
+          ( <replaceable class="PARAMETER">column</replaceable> [, ...] ) = ( { <replaceable class="PARAMETER">expression</replaceable> | DEFAULT } [, ...] ) } [, ...]
     [ FROM <replaceable class="PARAMETER">fromlist</replaceable> ]
     [ WHERE <replaceable class="PARAMETER">condition</replaceable> ]
     [ RETURNING * | <replaceable class="parameter">output_expression</replaceable> [ AS <replaceable class="parameter">output_name</replaceable> ] [, ...] ]
@@ -252,10 +252,6 @@ UPDATE films SET kind = 'Dramatic' WHERE kind = 'Drama';
 UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
   WHERE city = 'San Francisco' AND date = '2003-07-03';
 </programlisting>
-<programlisting>
-UPDATE weather SET (temp_lo, temp_hi, prcp) = (temp_lo+1, temp_lo+15, DEFAULT)
-  WHERE city = 'San Francisco' AND date = '2003-07-03';
-</programlisting>
   </para>
 
   <para>
@@ -269,6 +265,14 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
   </para>
 
   <para>
+   Use the alternative column-list syntax to do the same update:
+<programlisting>
+UPDATE weather SET (temp_lo, temp_hi, prcp) = (temp_lo+1, temp_lo+15, DEFAULT)
+  WHERE city = 'San Francisco' AND date = '2003-07-03';
+</programlisting>
+  </para>
+
+  <para>
    Increment the sales count of the salesperson who manages the
    account for Acme Corporation, using the <literal>FROM</literal>
    clause syntax:
@@ -317,6 +321,19 @@ COMMIT;
   </para>
 
   <para>
+   According to the standard, the column-list syntax should allow a list
+   of columns to be assigned from a single row-valued expression, such
+   as a sub-select:
+<programlisting>
+UPDATE accounts SET (contact_last_name, contact_first_name) =
+    (SELECT last_name, first_name FROM salesmen
+     WHERE salesmen.id = accounts.sales_id);
+</programlisting>
+   This is not currently implemented &mdash; the source must be a list
+   of independent expressions.
+  </para>
+
+  <para>
    Some other database systems offer a <literal>FROM</> option in which
    the target table is supposed to be listed again within <literal>FROM</>.
    That is not how <productname>PostgreSQL</productname> interprets
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 2ff24296e90..e0d52887958 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -11,7 +11,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/parser/gram.y,v 2.564 2006/09/03 03:19:44 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/parser/gram.y,v 2.565 2006/09/03 22:37:05 tgl Exp $
  *
  * HISTORY
  *	  AUTHOR			DATE			MAJOR EVENT
@@ -236,9 +236,9 @@ static void doNegateFloat(Value *v);
 				name_list from_clause from_list opt_array_bounds
 				qualified_name_list any_name any_name_list
 				any_operator expr_list attrs
-				target_list update_col_list update_target_list
-				update_value_list set_opt insert_column_list
-				values_list def_list indirection opt_indirection
+				target_list insert_column_list set_target_list
+				set_clause_list set_clause multiple_set_clause
+				ctext_expr_list ctext_row def_list indirection opt_indirection
 				group_clause TriggerFuncArgs select_limit
 				opt_select_limit opclass_item_list
 				transaction_mode_list_or_empty
@@ -299,7 +299,7 @@ static void doNegateFloat(Value *v);
 %type <list>	when_clause_list
 %type <ival>	sub_type
 %type <list>	OptCreateAs CreateAsList
-%type <node>	CreateAsElement values_item
+%type <node>	CreateAsElement ctext_expr
 %type <value>	NumericOnly FloatOnly IntegerOnly
 %type <alias>	alias_clause
 %type <sortby>	sortby
@@ -308,8 +308,7 @@ static void doNegateFloat(Value *v);
 %type <jexpr>	joined_table
 %type <range>	relation_expr
 %type <range>	relation_expr_opt_alias
-%type <target>	target_el update_target_el update_col_list_el insert_column_item
-%type <list>	update_target_lists_list update_target_lists_el
+%type <target>	target_el single_set_clause set_target insert_column_item
 
 %type <typnam>	Typename SimpleTypename ConstTypename
 				GenericType Numeric opt_float
@@ -5488,7 +5487,7 @@ opt_nowait:	NOWAIT							{ $$ = TRUE; }
  *****************************************************************************/
 
 UpdateStmt: UPDATE relation_expr_opt_alias
-			SET set_opt
+			SET set_clause_list
 			from_clause
 			where_clause
 			returning_clause
@@ -5503,9 +5502,65 @@ UpdateStmt: UPDATE relation_expr_opt_alias
 				}
 		;
 
-set_opt:
-			update_target_list						{ $$ = $1; }
-			| update_target_lists_list				{ $$ = $1; }
+set_clause_list:
+			set_clause							{ $$ = $1; }
+			| set_clause_list ',' set_clause	{ $$ = list_concat($1,$3); }
+		;
+
+set_clause:
+			single_set_clause						{ $$ = list_make1($1); }
+			| multiple_set_clause					{ $$ = $1; }
+		;
+
+single_set_clause:
+			set_target '=' ctext_expr
+				{
+					$$ = $1;
+					$$->val = (Node *) $3;
+				}
+		;
+
+multiple_set_clause:
+			'(' set_target_list ')' '=' ctext_row
+				{
+					ListCell *col_cell;
+					ListCell *val_cell;
+
+					/*
+					 * Break the ctext_row apart, merge individual expressions
+					 * into the destination ResTargets.  XXX this approach
+					 * cannot work for general row expressions as sources.
+					 */
+					if (list_length($2) != list_length($5))
+						ereport(ERROR,
+								(errcode(ERRCODE_SYNTAX_ERROR),
+								 errmsg("number of columns does not match number of values")));
+					forboth(col_cell, $2, val_cell, $5)
+					{
+						ResTarget *res_col = (ResTarget *) lfirst(col_cell);
+						Node *res_val = (Node *) lfirst(val_cell);
+
+						res_col->val = res_val;
+					}
+				    
+					$$ = $2;
+				}
+		;
+
+set_target:
+			ColId opt_indirection
+				{
+					$$ = makeNode(ResTarget);
+					$$->name = $1;
+					$$->indirection = $2;
+					$$->val = NULL;	/* upper production sets this */
+					$$->location = @1;
+				}
+		;
+
+set_target_list:
+			set_target								{ $$ = list_make1($1); }
+			| set_target_list ',' set_target		{ $$ = lappend($1,$3); }
 		;
 
 
@@ -5887,83 +5942,20 @@ locked_rels_list:
 
 
 values_clause:
-			VALUES '(' values_list ')'
+			VALUES ctext_row
 				{
 					SelectStmt *n = makeNode(SelectStmt);
-					n->valuesLists = list_make1($3);
+					n->valuesLists = list_make1($2);
 					$$ = (Node *) n;
 				}
-			| values_clause ',' '(' values_list ')'
+			| values_clause ',' ctext_row
 				{
 					SelectStmt *n = (SelectStmt *) $1;
-					n->valuesLists = lappend(n->valuesLists, $4);
+					n->valuesLists = lappend(n->valuesLists, $3);
 					$$ = (Node *) n;
 				}
 		;
 
-values_list: values_item							{ $$ = list_make1($1); }
-			| values_list ',' values_item			{ $$ = lappend($1, $3); }
-		;
-
-values_item:
-			a_expr					{ $$ = (Node *) $1; }
-			| DEFAULT				{ $$ = (Node *) makeNode(SetToDefault); }
-		;
-
-update_target_lists_list:
-			update_target_lists_el { $$ = $1; }
-			| update_target_lists_list ',' update_target_lists_el { $$ = list_concat($1, $3); }
-		;
-		
-update_target_lists_el:
-			'(' update_col_list ')' '=' '(' update_value_list ')'
-				{
-					ListCell *col_cell;
-					ListCell *val_cell;
-
-					if (list_length($2) != list_length($6))
-					{
-						ereport(ERROR, 
-							(errcode(ERRCODE_SYNTAX_ERROR),
-							 errmsg("number of columns does not match to number of values")));
-					}
-
-					for (col_cell = list_head($2), val_cell = list_head($6);
-						 col_cell != NULL && val_cell != NULL;
-						 col_cell = lnext(col_cell), val_cell = lnext(val_cell))
-					{
-						/* merge update_value_list with update_col_list */
-						ResTarget *res_col = (ResTarget *) lfirst(col_cell);
-						Node *res_val = (Node *) lfirst(val_cell);
-
-						res_col->val = res_val;
-					}
-				    
-					$$ = $2;
-				}
-		;
-
-update_col_list:
-			update_col_list_el { $$ = list_make1($1); }
-			| update_col_list ',' update_col_list_el { $$ = lappend($1, $3); }
-		;
-
-update_col_list_el:
-			ColId opt_indirection
-				{
-					$$ = makeNode(ResTarget);
-					$$->name = $1;
-					$$->indirection = $2;
-					$$->val = NULL;
-					$$->location = @1;
-				}
-		;
-
-update_value_list:
-			values_item { $$ = list_make1($1); }
-			| update_value_list ',' values_item { $$ = lappend($1, $3); }
-		;
-
 
 /*****************************************************************************
  *
@@ -8232,10 +8224,35 @@ opt_asymmetric: ASYMMETRIC
 			| /*EMPTY*/
 		;
 
+/*
+ * The SQL spec defines "contextually typed value expressions" and
+ * "contextually typed row value constructors", which for our purposes
+ * are the same as "a_expr" and "row" except that DEFAULT can appear at
+ * the top level.
+ */
+
+ctext_expr:
+			a_expr					{ $$ = (Node *) $1; }
+			| DEFAULT				{ $$ = (Node *) makeNode(SetToDefault); }
+		;
+
+ctext_expr_list:
+			ctext_expr								{ $$ = list_make1($1); }
+			| ctext_expr_list ',' ctext_expr		{ $$ = lappend($1, $3); }
+		;
+
+/*
+ * We should allow ROW '(' ctext_expr_list ')' too, but that seems to require
+ * making VALUES a fully reserved word, which will probably break more apps
+ * than allowing the noise-word is worth.
+ */
+ctext_row: '(' ctext_expr_list ')'					{ $$ = $2; }
+		;
+
 
 /*****************************************************************************
  *
- *	target lists for SELECT, UPDATE, INSERT
+ *	target list for SELECT
  *
  *****************************************************************************/
 
@@ -8275,31 +8292,6 @@ target_el:	a_expr AS ColLabel
 				}
 		;
 
-update_target_list:
-			update_target_el			  { $$ = list_make1($1); }
-			| update_target_list ',' update_target_el { $$ = lappend($1,$3); }
-		;
-
-update_target_el:
-			ColId opt_indirection '=' a_expr
-				{
-					$$ = makeNode(ResTarget);
-					$$->name = $1;
-					$$->indirection = $2;
-					$$->val = (Node *) $4;
-					$$->location = @1;
-				}
-			| ColId opt_indirection '=' DEFAULT
-				{
-					$$ = makeNode(ResTarget);
-					$$->name = $1;
-					$$->indirection = $2;
-					$$->val = (Node *) makeNode(SetToDefault);
-					$$->location = @1;
-				}
-
-		;
-
 
 /*****************************************************************************
  *
diff --git a/src/test/regress/expected/update.out b/src/test/regress/expected/update.out
index 55d82628e28..33d52a55a09 100644
--- a/src/test/regress/expected/update.out
+++ b/src/test/regress/expected/update.out
@@ -1,56 +1,85 @@
 --
--- UPDATE ... SET <col> = DEFAULT;
+-- UPDATE syntax tests
 --
 CREATE TABLE update_test (
     a   INT DEFAULT 10,
-    b   INT
+    b   INT,
+    c   TEXT
 );
-INSERT INTO update_test VALUES (5, 10);
-INSERT INTO update_test VALUES (10, 15);
+INSERT INTO update_test VALUES (5, 10, 'foo');
+INSERT INTO update_test(b, a) VALUES (15, 10);
 SELECT * FROM update_test;
- a  | b  
-----+----
-  5 | 10
- 10 | 15
+ a  | b  |  c  
+----+----+-----
+  5 | 10 | foo
+ 10 | 15 | 
 (2 rows)
 
 UPDATE update_test SET a = DEFAULT, b = DEFAULT;
 SELECT * FROM update_test;
- a  | b 
-----+---
- 10 |  
- 10 |  
+ a  | b |  c  
+----+---+-----
+ 10 |   | foo
+ 10 |   | 
 (2 rows)
 
 -- aliases for the UPDATE target table
 UPDATE update_test AS t SET b = 10 WHERE t.a = 10;
 SELECT * FROM update_test;
- a  | b  
-----+----
- 10 | 10
- 10 | 10
+ a  | b  |  c  
+----+----+-----
+ 10 | 10 | foo
+ 10 | 10 | 
 (2 rows)
 
 UPDATE update_test t SET b = t.b + 10 WHERE t.a = 10;
 SELECT * FROM update_test;
- a  | b  
-----+----
- 10 | 20
- 10 | 20
+ a  | b  |  c  
+----+----+-----
+ 10 | 20 | foo
+ 10 | 20 | 
 (2 rows)
 
 --
 -- Test VALUES in FROM
 --
 UPDATE update_test SET a=v.i FROM (VALUES(100, 20)) AS v(i, j)
-	WHERE update_test.b = v.j;
+  WHERE update_test.b = v.j;
 SELECT * FROM update_test;
-  a  | b  
------+----
- 100 | 20
- 100 | 20
+  a  | b  |  c  
+-----+----+-----
+ 100 | 20 | foo
+ 100 | 20 | 
 (2 rows)
 
+--
+-- Test multiple-set-clause syntax
+--
+UPDATE update_test SET (c,b,a) = ('bugle', b+11, DEFAULT) WHERE c = 'foo';
+SELECT * FROM update_test;
+  a  | b  |   c   
+-----+----+-------
+ 100 | 20 | 
+  10 | 31 | bugle
+(2 rows)
+
+UPDATE update_test SET (c,b) = ('car', a+b), a = a + 1 WHERE a = 10;
+SELECT * FROM update_test;
+  a  | b  |  c  
+-----+----+-----
+ 100 | 20 | 
+  11 | 41 | car
+(2 rows)
+
+-- fail, multi assignment to same column:
+UPDATE update_test SET (c,b) = ('car', a+b), b = a + 1 WHERE a = 10;
+ERROR:  multiple assignments to same column "b"
+-- XXX this should work, but doesn't yet:
+UPDATE update_test SET (a,b) = (select a,b FROM update_test where c = 'foo')
+  WHERE a = 10;
+ERROR:  syntax error at or near "select"
+LINE 1: UPDATE update_test SET (a,b) = (select a,b FROM update_test ...
+                                        ^
 -- if an alias for the target table is specified, don't allow references
 -- to the original table name
 BEGIN;
diff --git a/src/test/regress/sql/update.sql b/src/test/regress/sql/update.sql
index 51007b2ff18..2df29958108 100644
--- a/src/test/regress/sql/update.sql
+++ b/src/test/regress/sql/update.sql
@@ -1,14 +1,15 @@
 --
--- UPDATE ... SET <col> = DEFAULT;
+-- UPDATE syntax tests
 --
 
 CREATE TABLE update_test (
     a   INT DEFAULT 10,
-    b   INT
+    b   INT,
+    c   TEXT
 );
 
-INSERT INTO update_test VALUES (5, 10);
-INSERT INTO update_test VALUES (10, 15);
+INSERT INTO update_test VALUES (5, 10, 'foo');
+INSERT INTO update_test(b, a) VALUES (15, 10);
 
 SELECT * FROM update_test;
 
@@ -30,10 +31,25 @@ SELECT * FROM update_test;
 --
 
 UPDATE update_test SET a=v.i FROM (VALUES(100, 20)) AS v(i, j)
-	WHERE update_test.b = v.j;
+  WHERE update_test.b = v.j;
 
 SELECT * FROM update_test;
 
+--
+-- Test multiple-set-clause syntax
+--
+
+UPDATE update_test SET (c,b,a) = ('bugle', b+11, DEFAULT) WHERE c = 'foo';
+SELECT * FROM update_test;
+UPDATE update_test SET (c,b) = ('car', a+b), a = a + 1 WHERE a = 10;
+SELECT * FROM update_test;
+-- fail, multi assignment to same column:
+UPDATE update_test SET (c,b) = ('car', a+b), b = a + 1 WHERE a = 10;
+
+-- XXX this should work, but doesn't yet:
+UPDATE update_test SET (a,b) = (select a,b FROM update_test where c = 'foo')
+  WHERE a = 10;
+
 -- if an alias for the target table is specified, don't allow references
 -- to the original table name
 BEGIN;