INSERT and parentheses

On 2010-08-24 8:25 AM +0300, igor polishchuk wrote:

Marko et al,
This is my first ever attempt of a patch review just for learning the
procedure. I'm not a postgres developer, so the review is partial and mostly
from the usability prospective.

That's all right. I'm sure any help is appreciated.

The patch provides a HINT for unclear error. This should clarify for a user
what exactly is wrong with the sql.
However, the actual HINT text provided with the patch is not very clear,
too.
The Stephen Frost's suggestion would add clarity:

errhint("insert appears to be a single column with a record-type rather than
multiple columns of non-composite type."),

This isn't entirely accurate, either; the columns are not necessarily of
non-composite types.

Regards,
Marko Tiikkaja

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:

[ patch for a misleading error message ]

I've committed the attached revised version of this patch. The main
change is to only emit the hint if the insertion source can be shown
to be a RowExpr with exactly the number of columns expected by the
INSERT. This should avoid emitting the hint in cases where it's not
relevant, and allows a specific wording for the hint, as was recommended
by Stephen Frost.

regards, tom lane

Index: analyze.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/analyze.c,v
retrieving revision 1.403
diff -c -r1.403 analyze.c
*** analyze.c	27 Aug 2010 20:30:08 -0000	1.403
--- analyze.c	18 Sep 2010 18:27:33 -0000
***************
*** 47,52 ****
--- 47,53 ----
  static Query *transformInsertStmt(ParseState *pstate, InsertStmt *stmt);
  static List *transformInsertRow(ParseState *pstate, List *exprlist,
  				   List *stmtcols, List *icolumns, List *attrnos);
+ static int	count_rowexpr_columns(ParseState *pstate, Node *expr);
  static Query *transformSelectStmt(ParseState *pstate, SelectStmt *stmt);
  static Query *transformValuesClause(ParseState *pstate, SelectStmt *stmt);
  static Query *transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt);
***************
*** 726,737 ****
--- 727,753 ----
  												  list_length(icolumns))))));
  	if (stmtcols != NIL &&
  		list_length(exprlist) < list_length(icolumns))
+ 	{
+ 		/*
+ 		 * We can get here for cases like INSERT ... SELECT (a,b,c) FROM ...
+ 		 * where the user accidentally created a RowExpr instead of separate
+ 		 * columns.  Add a suitable hint if that seems to be the problem,
+ 		 * because the main error message is quite misleading for this case.
+ 		 * (If there's no stmtcols, you'll get something about data type
+ 		 * mismatch, which is less misleading so we don't worry about giving
+ 		 * a hint in that case.)
+ 		 */
  		ereport(ERROR,
  				(errcode(ERRCODE_SYNTAX_ERROR),
  				 errmsg("INSERT has more target columns than expressions"),
+ 				 ((list_length(exprlist) == 1 &&
+ 				   count_rowexpr_columns(pstate, linitial(exprlist)) ==
+ 				   list_length(icolumns)) ?
+ 				  errhint("The insertion source is a row expression containing the same number of columns expected by the INSERT. Did you accidentally use extra parentheses?") : 0),
  				 parser_errposition(pstate,
  									exprLocation(list_nth(icolumns,
  												  list_length(exprlist))))));
+ 	}

  	/*
  	 * Prepare columns for assignment to target table.
***************
*** 762,767 ****
--- 778,826 ----
  	return result;
  }

+ /*
+  * count_rowexpr_columns -
+  *	  get number of columns contained in a ROW() expression;
+  *	  return -1 if expression isn't a RowExpr or a Var referencing one.
+  *
+  * This is currently used only for hint purposes, so we aren't terribly
+  * tense about recognizing all possible cases.  The Var case is interesting
+  * because that's what we'll get in the INSERT ... SELECT (...) case.
+  */
+ static int
+ count_rowexpr_columns(ParseState *pstate, Node *expr)
+ {
+ 	if (expr == NULL)
+ 		return -1;
+ 	if (IsA(expr, RowExpr))
+ 		return list_length(((RowExpr *) expr)->args);
+ 	if (IsA(expr, Var))
+ 	{
+ 		Var		   *var = (Var *) expr;
+ 		AttrNumber	attnum = var->varattno;
+ 
+ 		if (attnum > 0 && var->vartype == RECORDOID)
+ 		{
+ 			RangeTblEntry *rte;
+ 
+ 			rte = GetRTEByRangeTablePosn(pstate, var->varno, var->varlevelsup);
+ 			if (rte->rtekind == RTE_SUBQUERY)
+ 			{
+ 				/* Subselect-in-FROM: examine sub-select's output expr */
+ 				TargetEntry *ste = get_tle_by_resno(rte->subquery->targetList,
+ 													attnum);
+ 
+ 				if (ste == NULL || ste->resjunk)
+ 					return -1;
+ 				expr = (Node *) ste->expr;
+ 				if (IsA(expr, RowExpr))
+ 					return list_length(((RowExpr *) expr)->args);
+ 			}
+ 		}
+ 	}
+ 	return -1;
+ }
+ 

/*
* transformSelectStmt -