Re: BUG #15237: I got "ERROR: source for a multiple-column UPDATE item must be a sub-SELECT or ROW() expression"

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> As far as (a) goes, it was an intentional compatibility breakage,

> So here I have a problem: the fact that it was a compatibility breakage
> seems not to have been discussed *or even so much as mentioned* on
> -hackers at any time.

I will freely admit that at the time I did the original patch, I did not
think that the change for the single-column case was of interest to
anyone.  Who'd write an extra four parentheses that they didn't need?
But it *was* discussed later, including on -hackers:

https://www.postgresql.org/message-id/flat/20170719174507.GA19616%40telsasoft.com#20170719174507.GA19616@telsasoft.com
https://www.postgresql.org/message-id/flat/CAMjNa7cDLzPcs0xnRpkvqmJ6Vb6G3EH8CYGp9ZBjXdpFfTz6dg%40mail.gmail.com

>  Tom> For example, suppose f(...) returns a single-column tuple result.
>  Tom> This should be legal, if x matches the type of the single column:
>  Tom>     update ... set (x) = f(...)
>  Tom> but if we try to do what you seem to have in mind, this would not
>  Tom> be:
>  Tom>     update ... set (x) = (f(...))

> The spec indeed says that this is not valid, since it would wrap an
> additional ROW() around the result, and would try and assign the row
> value to x.

I think that arguing that the spec requires that is fairly shaky, and
implementing it would be even shakier; for example, we'd have to start
worrying about whether ruleutils.c's habit of throwing in extra parens
when in doubt could ever result in unexpected change to the meaning.

I'd also point out that with the rule that the extra parens implicitly
mean "ROW()", it's fairly unclear what should happen with

    update ... set (x) = ((select ...))

If somebody were to hold a gun to my head and say "you must make this
work", I'd probably do something like the attached, which abuses the
operator_precedence_warning infrastructure to detect whether there were
extra parens.  One would have to argue about exactly what it should do
with parens around ROW() or (SELECT ...), but I chose to ignore them.
(I think that in the SELECT case, the grammar would actually absorb
the extra parens elsewhere, so that we couldn't do anything differently
anyway without further kluges.)

But I repeat that this is a bad idea and we'd regret it in the long run;
treating extra parens as meaning ROW() in some cases is just a
fundamentally unsound idea from both syntactic and semantic standpoints.
Given the extremely small number of complaints since v10 came out,
I think we should just stay with what we have.

            regards, tom lane

PS: I noticed while messing with this that there's a separate problem:
right now, turning on operator_precedence_warning causes unexpected
failures with extra parens around the RHS.  The code added below to
make transformMultiAssignRef look through AEXPR_PAREN nodes before
deciding anything is needed anyway to fix that.

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 90dfac2..64bd254 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
*************** static Node *makeRecursiveViewSelect(cha
*** 470,476 ****
  %type <node>    def_arg columnElem where_clause where_or_current_clause
                  a_expr b_expr c_expr AexprConst indirection_el opt_slice_bound
                  columnref in_expr having_clause func_table xmltable array_expr
!                 ExclusionWhereClause operator_def_arg
  %type <list>    rowsfrom_item rowsfrom_list opt_col_def_list
  %type <boolean> opt_ordinality
  %type <list>    ExclusionConstraintList ExclusionConstraintElem
--- 470,476 ----
  %type <node>    def_arg columnElem where_clause where_or_current_clause
                  a_expr b_expr c_expr AexprConst indirection_el opt_slice_bound
                  columnref in_expr having_clause func_table xmltable array_expr
!                 ct_row_expr ExclusionWhereClause operator_def_arg
  %type <list>    rowsfrom_item rowsfrom_list opt_col_def_list
  %type <boolean> opt_ordinality
  %type <list>    ExclusionConstraintList ExclusionConstraintElem
*************** set_clause:
*** 11070,11076 ****
                      $1->val = (Node *) $3;
                      $$ = list_make1($1);
                  }
!             | '(' set_target_list ')' '=' a_expr
                  {
                      int ncolumns = list_length($2);
                      int i = 1;
--- 11070,11076 ----
                      $1->val = (Node *) $3;
                      $$ = list_make1($1);
                  }
!             | '(' set_target_list ')' '=' ct_row_expr
                  {
                      int ncolumns = list_length($2);
                      int i = 1;
*************** c_expr:        columnref                                { $$ = $1; }
*** 13451,13457 ****
                          n->indirection = check_indirection($4, yyscanner);
                          $$ = (Node *)n;
                      }
!                     else if (operator_precedence_warning)
                      {
                          /*
                           * If precedence warnings are enabled, insert
--- 13451,13458 ----
                          n->indirection = check_indirection($4, yyscanner);
                          $$ = (Node *)n;
                      }
!                     else if (operator_precedence_warning ||
!                              pg_yyget_extra(yyscanner)->in_ctre)
                      {
                          /*
                           * If precedence warnings are enabled, insert
*************** c_expr:        columnref                                { $$ = $1; }
*** 13469,13474 ****
--- 13470,13479 ----
                           * suppress warnings, it's probably safe enough; and
                           * we'd just as soon not waste cycles on dummy parse
                           * nodes if we don't have to.
+                          *
+                          * We also insert AEXPR_PAREN nodes if we're inside a
+                          * ct_row_expr, so that parse analysis can distinguish
+                          * "(x)" from "x" later.
                           */
                          $$ = (Node *) makeA_Expr(AEXPR_PAREN, NIL, $2, NULL,
                                                   exprLocation($2));
*************** opt_asymmetric: ASYMMETRIC
*** 14599,14604 ****
--- 14604,14623 ----
              | /*EMPTY*/
          ;

+ /*
+  * Deal with the spec's poorly-thought-through "contextually typed row value
+  * expression" syntax.  For that, we need to detect whether the expression has
+  * outer parentheses.  We can't define it as "a_expr | (a_expr)" without
+  * causing reduce/reduce conflicts, so instead force AEXPR_PAREN nodes to be
+  * inserted for parens throughout the subexpression, and we'll sort it out
+  * during parse analysis.
+  */
+ ct_row_expr:
+             { pg_yyget_extra(yyscanner)->in_ctre++; }
+             a_expr
+             { pg_yyget_extra(yyscanner)->in_ctre--; $$ = $2; }
+         ;
+

  /*****************************************************************************
   *
*************** void
*** 16328,16331 ****
--- 16347,16351 ----
  parser_init(base_yy_extra_type *yyext)
  {
      yyext->parsetree = NIL;        /* in case grammar forgets to set it */
+     yyext->in_ctre = 0;            /* not handling ct_row_expr */
  }
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 385e54a..0d09d7b 100644
*** a/src/backend/parser/parse_expr.c
--- b/src/backend/parser/parse_expr.c
*************** transformMultiAssignRef(ParseState *psta
*** 1499,1515 ****
      /* We only need to transform the source if this is the first column */
      if (maref->colno == 1)
      {
          /*
           * For now, we only allow EXPR SubLinks and RowExprs as the source of
           * an UPDATE multiassignment.  This is sufficient to cover interesting
           * cases; at worst, someone would have to write (SELECT * FROM expr)
           * to expand a composite-returning expression of another form.
           */
!         if (IsA(maref->source, SubLink) &&
!             ((SubLink *) maref->source)->subLinkType == EXPR_SUBLINK)
          {
              /* Relabel it as a MULTIEXPR_SUBLINK */
!             sublink = (SubLink *) maref->source;
              sublink->subLinkType = MULTIEXPR_SUBLINK;
              /* And transform it */
              sublink = (SubLink *) transformExprRecurse(pstate,
--- 1499,1548 ----
      /* We only need to transform the source if this is the first column */
      if (maref->colno == 1)
      {
+         Node       *source = maref->source;
+         bool        have_parens = false;
+
+         /* Drill through any AEXPR_PAREN nodes */
+         while (source && IsA(source, A_Expr) &&
+                ((A_Expr *) source)->kind == AEXPR_PAREN)
+         {
+             have_parens = true;
+             source = ((A_Expr *) source)->lexpr;
+         }
+
+         /*
+          * If we have a single-column assignment, and the RHS had parens but
+          * isn't a SubLink or RowExpr, pretend it's a single-column RowExpr.
+          * This is quite broken, as it will cause odd inconsistencies whenever
+          * we get around to allowing other syntactic cases for the RHS, but
+          * some people insisted.
+          */
+         if (have_parens && maref->ncolumns == 1 &&
+             !(IsA(source, SubLink) &&
+               ((SubLink *) source)->subLinkType == EXPR_SUBLINK) &&
+             !IsA(source, RowExpr))
+         {
+             RowExpr    *r = makeNode(RowExpr);
+
+             r->args = list_make1(source);
+             r->row_typeid = InvalidOid;
+             r->colnames = NIL;
+             r->row_format = COERCE_IMPLICIT_CAST;    /* abuse */
+             r->location = -1;
+             source = (Node *) r;
+         }
+
          /*
           * For now, we only allow EXPR SubLinks and RowExprs as the source of
           * an UPDATE multiassignment.  This is sufficient to cover interesting
           * cases; at worst, someone would have to write (SELECT * FROM expr)
           * to expand a composite-returning expression of another form.
           */
!         if (IsA(source, SubLink) &&
!             ((SubLink *) source)->subLinkType == EXPR_SUBLINK)
          {
              /* Relabel it as a MULTIEXPR_SUBLINK */
!             sublink = (SubLink *) source;
              sublink->subLinkType = MULTIEXPR_SUBLINK;
              /* And transform it */
              sublink = (SubLink *) transformExprRecurse(pstate,
*************** transformMultiAssignRef(ParseState *psta
*** 1542,1552 ****
               */
              sublink->subLinkId = list_length(pstate->p_multiassign_exprs);
          }
!         else if (IsA(maref->source, RowExpr))
          {
              /* Transform the RowExpr, allowing SetToDefault items */
              rexpr = (RowExpr *) transformRowExpr(pstate,
!                                                  (RowExpr *) maref->source,
                                                   true);

              /* Check it returns required number of columns */
--- 1575,1585 ----
               */
              sublink->subLinkId = list_length(pstate->p_multiassign_exprs);
          }
!         else if (IsA(source, RowExpr))
          {
              /* Transform the RowExpr, allowing SetToDefault items */
              rexpr = (RowExpr *) transformRowExpr(pstate,
!                                                  (RowExpr *) source,
                                                   true);

              /* Check it returns required number of columns */
*************** transformMultiAssignRef(ParseState *psta
*** 1568,1574 ****
              ereport(ERROR,
                      (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                       errmsg("source for a multiple-column UPDATE item must be a sub-SELECT or ROW() expression"),
!                      parser_errposition(pstate, exprLocation(maref->source))));
      }
      else
      {
--- 1601,1607 ----
              ereport(ERROR,
                      (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                       errmsg("source for a multiple-column UPDATE item must be a sub-SELECT or ROW() expression"),
!                      parser_errposition(pstate, exprLocation(source))));
      }
      else
      {
diff --git a/src/include/parser/gramparse.h b/src/include/parser/gramparse.h
index 42e7ede..6e51081 100644
*** a/src/include/parser/gramparse.h
--- b/src/include/parser/gramparse.h
*************** typedef struct base_yy_extra_type
*** 53,58 ****
--- 53,59 ----
       * State variables that belong to the grammar.
       */
      List       *parsetree;        /* final parse result is delivered here */
+     int            in_ctre;        /* handling contextually-typed row expr? */
  } base_yy_extra_type;

  /*