Better error message for window-function spec bizarreness - Mailing list pgsql-hackers

We've had a couple of complaints about the error message that's thrown
for the case where you try to copy-and-modify a window definition that
includes a frame clause:
http://www.postgresql.org/message-id/200911191711.nAJHBped009004@wwwmaster.postgresql.org
http://www.postgresql.org/message-id/CADyrUxP-5pNAqxjuFx9ToeTEhsxwo942PS3Bk_=JEKdMVg0W7A@mail.gmail.com

I propose the attached patch, which changes the text of the message to
"cannot copy window \"%s\" because it has a frame clause", and then adds
a HINT "Omit the parentheses in this OVER clause." if the clause is just
OVER (windowname) and doesn't include any attempt to modify the window's
properties.

I think we should back-patch this into all versions that have window
functions (ie, all supported branches).

Any objections or better ideas?

            regards, tom lane

diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index ea90e58..29183c1 100644
*** a/src/backend/parser/parse_clause.c
--- b/src/backend/parser/parse_clause.c
*************** transformWindowDefinitions(ParseState *p
*** 1735,1745 ****
          /*
           * Per spec, a windowdef that references a previous one copies the
           * previous partition clause (and mustn't specify its own).  It can
!          * specify its own ordering clause. but only if the previous one had
           * none.  It always specifies its own frame clause, and the previous
!          * one must not have a frame clause.  (Yeah, it's bizarre that each of
           * these cases works differently, but SQL:2008 says so; see 7.11
!          * <window clause> syntax rule 10 and general rule 1.)
           */
          if (refwc)
          {
--- 1735,1750 ----
          /*
           * Per spec, a windowdef that references a previous one copies the
           * previous partition clause (and mustn't specify its own).  It can
!          * specify its own ordering clause, but only if the previous one had
           * none.  It always specifies its own frame clause, and the previous
!          * one must not have a frame clause.  Yeah, it's bizarre that each of
           * these cases works differently, but SQL:2008 says so; see 7.11
!          * <window clause> syntax rule 10 and general rule 1.  The frame
!          * clause rule is especially bizarre because it makes "OVER foo"
!          * different from "OVER (foo)", solely in that the latter is required
!          * to throw an error if foo has a nondefault frame clause.    Well, ours
!          * not to reason why, but we do go out of our way to throw a useful
!          * error message for such cases.
           */
          if (refwc)
          {
*************** transformWindowDefinitions(ParseState *p
*** 1778,1788 ****
              wc->copiedOrder = false;
          }
          if (refwc && refwc->frameOptions != FRAMEOPTION_DEFAULTS)
              ereport(ERROR,
                      (errcode(ERRCODE_WINDOWING_ERROR),
!                      errmsg("cannot override frame clause of window \"%s\"",
!                             windef->refname),
                       parser_errposition(pstate, windef->location)));
          wc->frameOptions = windef->frameOptions;
          /* Process frame offset expressions */
          wc->startOffset = transformFrameOffset(pstate, wc->frameOptions,
--- 1783,1809 ----
              wc->copiedOrder = false;
          }
          if (refwc && refwc->frameOptions != FRAMEOPTION_DEFAULTS)
+         {
+             /*
+              * Use this message if this is a WINDOW clause, or if it's an OVER
+              * clause that includes ORDER BY or framing clauses.  (We already
+              * rejected PARTITION BY above, so no need to check that.)
+              */
+             if (windef->name ||
+                 orderClause || windef->frameOptions != FRAMEOPTION_DEFAULTS)
+                 ereport(ERROR,
+                         (errcode(ERRCODE_WINDOWING_ERROR),
+                          errmsg("cannot copy window \"%s\" because it has a frame clause",
+                                 windef->refname),
+                          parser_errposition(pstate, windef->location)));
+             /* Else this clause is just OVER (foo), so say this: */
              ereport(ERROR,
                      (errcode(ERRCODE_WINDOWING_ERROR),
!             errmsg("cannot copy window \"%s\" because it has a frame clause",
!                    windef->refname),
!                      errhint("Omit the parentheses in this OVER clause."),
                       parser_errposition(pstate, windef->location)));
+         }
          wc->frameOptions = windef->frameOptions;
          /* Process frame offset expressions */
          wc->startOffset = transformFrameOffset(pstate, wc->frameOptions,