Re: [HACKERS] INSERT and parentheses

[Tom Lane]

Marko Tiikkaja  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 -  1.403
--- analyze.c   18 Sep 2010 18:27:33 -
***
*** 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 intcount_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);
+ 

Re: [HACKERS] INSERT and parentheses

[Marko Tiikkaja]

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] INSERT and parentheses

[igor polishchuk]

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.
The original message id of the patch is
4bd58db3.4070...@cs.helsinki.fi


Submssion review
=

* Is he patch in context diff format?
Ys

* Dos it apply cleanly to the current CVS HEAD?
Applies cleanly to a source code snapshot

* Dos it include reasonable tests, necessary doc patches, etc?
I does not require a doc patch. A test is not included, but it looks pretty
trivial:

-- Prpare the test tables

drop table if exists foo;
drop table if exists boo;

crate table foo(
a nt,
b nt,
c nt);

crate table boo(
a nt,
b nt,
c nt);

insert into boo values (10,20,30);

-- Actual test

INSERT INTO foo(a,b,c) SELECT (a,b,c) FROM boo;
INSERT INTO foo(a,b,c) VALUES((0,1,2));

Usability Review
=

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."),


Feature test
=

The feature works as advertised for the test provided above.
No failures or crashes.
No visible affect on performance

Re: [HACKERS] INSERT and parentheses

[Bruce Momjian]

I have added this to the next commit-fest:

https://commitfest.postgresql.org/action/commitfest_view?id=6


---

Marko Tiikkaja wrote:
> Hi,
> 
> This came up on IRC today and I recall several instances of this during
> the last two months or so, so I decided to send a patch.  The problem in
> question occurs when you have extra parentheses in an INSERT list:
> 
> INSERT INTO foo(a,b,c) SELECT (a,b,c) FROM ..; or
> INSERT INTO foo(a,b,c) VALUES((0,1,2));
> 
> Both of these give you the same error:
> ERROR:  INSERT has more target columns than expressions
> 
> The first version is a lot more common and as it turns out, is sometimes
> very hard to spot.  This patch attaches a HINT message to these two
> cases.  The message itself could probably be a lot better, but I can't
> think of anything.
> 
> Thoughts?
> 
> 
> Regards,
> Marko Tiikkaja

[ Attachment, skipping... ]

> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + None of us is going to be here forever. +


-- 
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] INSERT and parentheses

[Tom Lane]

Stephen Frost  writes:
> Not to be a pain, but the hint really is kind of terrible..  It'd
> probably be better if you included somewhere that the insert appears to
> be a single column with a record-type rather than multiple columns of
> non-composite type..

I don't much care for the test, either.  AFAICS, a hint like this would
only be appropriate for a RowExpr item, *not* a Var.  It might also be
worth checking the number of items in the RowExpr before deciding that
the hint is appropriate.

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] INSERT and parentheses

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> > The first version is a lot more common and as it turns out, is sometimes
> > very hard to spot.  This patch attaches a HINT message to these two
> > cases.  The message itself could probably be a lot better, but I can't
> > think of anything.
> >
> > Thoughts?
> 
> I suggest adding it to the next CommitFest.  Since I've never been
> bitten by this, I can't get excited about the change, but I'm also not
> arrogant enough to believe that everyone else's experiences are the
> same as my own.

Not to be a pain, but the hint really is kind of terrible..  It'd
probably be better if you included somewhere that the insert appears to
be a single column with a record-type rather than multiple columns of
non-composite type..

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] INSERT and parentheses

[Robert Haas]

On Mon, Apr 26, 2010 at 8:57 AM, Marko Tiikkaja
 wrote:
> Hi,
>
> This came up on IRC today and I recall several instances of this during
> the last two months or so, so I decided to send a patch.  The problem in
> question occurs when you have extra parentheses in an INSERT list:
>
> INSERT INTO foo(a,b,c) SELECT (a,b,c) FROM ..; or
> INSERT INTO foo(a,b,c) VALUES((0,1,2));
>
> Both of these give you the same error:
> ERROR:  INSERT has more target columns than expressions
>
> The first version is a lot more common and as it turns out, is sometimes
> very hard to spot.  This patch attaches a HINT message to these two
> cases.  The message itself could probably be a lot better, but I can't
> think of anything.
>
> Thoughts?

I suggest adding it to the next CommitFest.  Since I've never been
bitten by this, I can't get excited about the change, but I'm also not
arrogant enough to believe that everyone else's experiences are the
same as my own.

...Robert

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] INSERT and parentheses

[Marko Tiikkaja]

Hi,

This came up on IRC today and I recall several instances of this during
the last two months or so, so I decided to send a patch.  The problem in
question occurs when you have extra parentheses in an INSERT list:

INSERT INTO foo(a,b,c) SELECT (a,b,c) FROM ..; or
INSERT INTO foo(a,b,c) VALUES((0,1,2));

Both of these give you the same error:
ERROR:  INSERT has more target columns than expressions

The first version is a lot more common and as it turns out, is sometimes
very hard to spot.  This patch attaches a HINT message to these two
cases.  The message itself could probably be a lot better, but I can't
think of anything.

Thoughts?


Regards,
Marko Tiikkaja
*** a/src/backend/parser/analyze.c
--- b/src/backend/parser/analyze.c
***
*** 730,742  transformInsertRow(ParseState *pstate, List *exprlist,

  list_length(icolumns));
if (stmtcols != NIL &&
list_length(exprlist) < list_length(icolumns))
!   ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
 errmsg("INSERT has more target columns than 
expressions"),
 parser_errposition(pstate,

exprLocation(list_nth(icolumns,

  list_length(exprlist));
  
/*
 * Prepare columns for assignment to target table.
 */
--- 730,761 

  list_length(icolumns));
if (stmtcols != NIL &&
list_length(exprlist) < list_length(icolumns))
!   {
!   /*
!* If the expression only has a single column of type record, 
it's
!* possible that that wasn't intended.
!*/
!   if (list_length(exprlist) == 1 &&
!   (IsA(linitial(exprlist), Var) &&
!   ((Var *) linitial(exprlist))->vartype == RECORDOID) ||
!   IsA(linitial(exprlist), RowExpr))
!   ereport(ERROR,
!   (errcode(ERRCODE_SYNTAX_ERROR),
!errmsg("INSERT has more target columns than 
expressions"),
!errhint("Did you accidentally use extra 
parentheses?"),
!parser_errposition(pstate,
!   
exprLocation(list_nth(icolumns,
!   
  list_length(exprlist));
!   else
!   ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
 errmsg("INSERT has more target columns than 
expressions"),
 parser_errposition(pstate,

exprLocation(list_nth(icolumns,

  list_length(exprlist));
  
+   }
+ 
/*
 * Prepare columns for assignment to target table.
 */

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers