Обсуждение: INSERT and parentheses

Marko et al,<br /> 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.<br />The original message id of the patch
is<a href="http://archives.postgresql.org/pgsql-hackers/2010-04/msg01200.php">4BD58DB3.4070605@cs.helsinki.fi</a><br
/><br/><br /> Submssion review<br /> =========================<br /><br /> * Is he patch in context diff format?<br />
Ys<br/><br /> * Dos it apply cleanly to the current CVS HEAD?<br /> Applies cleanly to a source code snapshot<br /><br
/>* Dos it include reasonable tests, necessary doc patches, etc?<br /> I does not require a doc patch. A test is not
included,but it looks pretty trivial:<br /><br /> -- Prpare the test tables<br /><br /> drop table if exists foo;<br />
droptable if exists boo;<br /><br /> crate table foo(<br /> a nt,<br /> b nt,<br /> c nt);<br /><br /> crate table
boo(<br/> a nt,<br /> b nt,<br /> c nt);<br /><br /> insert into boo values (10,20,30);<br /><br /> -- Actual test<br
/><br/> INSERT INTO foo(a,b,c) SELECT (a,b,c) FROM boo;<br /> INSERT INTO foo(a,b,c) VALUES((0,1,2));<br /><br />
UsabilityReview<br /> =========================<br /><br /> The patch provides a HINT for unclear error. This should
clarifyfor a user what exactly is wrong with the sql.<br /> However, the actual HINT text provided with the patch is
notvery clear, too.<br /> The Stephen Frost's suggestion would add clarity:<br /><br /> errhint("insert appears to be a
singlecolumn with a record-type rather than multiple columns of non-composite type."),<br /><br /><br /> Feature
test<br/> =========================<br /><br /> The feature works as advertised for the test provided above.<br /> No
failuresor crashes.<br /> No visible affect on performance  

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
hasmore 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
theINSERT. 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 -