Re: Windowing Function Patch Review -> Standard Conformance

2008/12/28 David Rowley <dgrowley@gmail.com>:
> Tom Lane Wrote:
>> I've spent quite a bit of time reviewing the window functions patch,
>> and I think it is now ready to commit, other than the documentation
>> (which I've not looked at yet at all).  Attached is my current patch
>> against HEAD, sans documentation.  This incorporates the recently
>> discussed aggregate-function API changes and support for tuplestore
>> trimming.  There's a number of things that could be improved yet:
>>       * we really ought to have some support for non-built-in
>>         window functions
>>       * I think the planner could be a bit smarter about when to
>>         sort or not
>>       * tuplestore_advance and related code really needs to be made
>>         more efficient; it didn't matter much before but it does now
>> but I think these things can be worked on after the core patch is
>> committed.
>>
>>                       regards, tom lane
>
> I've started running my test queries that I used when reviewing the patch.
> The following crashes the backend:
>
> CREATE TABLE billofmaterials (
>  parentpart VARCHAR(20) NOT NULL,
>  childpart VARCHAR(20) NOT NULL,
>  quantity FLOAT NOT NULL,
>  CHECK(quantity > 0),
>  PRIMARY KEY(parentpart, childpart)
> );
>
> INSERT INTO billofmaterials VALUES('KITCHEN','TABLE',1);
> INSERT INTO billofmaterials VALUES('KITCHEN','COOKER',1);
> INSERT INTO billofmaterials VALUES('KITCHEN','FRIDGE',1);
> INSERT INTO billofmaterials VALUES('TABLE','CHAIR',4);
> INSERT INTO billofmaterials VALUES('CHAIR','LEG',4);
>
>
> WITH RECURSIVE bom AS (
>  SELECT parentpart,childpart,quantity,ROW_NUMBER() OVER (ORDER BY
> parentpart DESC) rn
>  FROM billofmaterials
>  WHERE parentpart = 'KITCHEN'
>  UNION ALL
>  SELECT b.parentpart,b.childpart,b.quantity,ROW_NUMBER() OVER (ORDER BY
> parentpart ASC) rn
>  FROM billofmaterials b
>  INNER JOIN bom ON b.parentpart = bom.childpart
> )
> SELECT * from bom;
>
> It seems not to like recursively calling row_number(). It does not crash if
> I replace the 2nd row_number() with the constant 1
>

It seems that parseCheckWindowFuncs() doesn't check CTE case whereas
parseCheckAggregates() checks it, including check of window functions.
So the urgent patch is as following, but is this operation allowed? I
am blind in CTE rules...


*** src/backend/parser/parse_agg.c.orig    Sun Dec 28 15:34:21 2008
--- src/backend/parser/parse_agg.c    Mon Dec 29 01:13:16 2008
***************
*** 336,347 ****                  errmsg("aggregate functions not allowed in a recursive query's
recursive term"),                  parser_errposition(pstate,
locate_agg_of_level((Node*) qry, 0))));
 
-     if (pstate->p_hasWindowFuncs && hasSelfRefRTEs)
-         ereport(ERROR,
-                 (errcode(ERRCODE_INVALID_RECURSION),
-                  errmsg("window functions not allowed in a recursive query's
recursive term"),
-                  parser_errposition(pstate,
-                                     locate_windowfunc((Node *) qry)))); }
 /*
--- 336,341 ----
***************
*** 357,366 ****
--- 351,374 ---- parseCheckWindowFuncs(ParseState *pstate, Query *qry) {     ListCell       *l;
+     bool            hasSelfRefRTEs;
     /* This should only be called if we found window functions */     Assert(pstate->p_hasWindowFuncs);

+     /*
+      * Scan the range table to see if there are JOIN or self-reference CTE
+      * entries.  We'll need this info below.
+      */
+     hasSelfRefRTEs = false;
+     foreach(l, pstate->p_rtable)
+     {
+         RangeTblEntry *rte = (RangeTblEntry *) lfirst(l);
+
+         if (rte->rtekind != RTE_JOIN && rte->rtekind == RTE_CTE &&
rte->self_reference)
+             hasSelfRefRTEs = true;
+     }
+     if (checkExprHasWindowFuncs(qry->jointree->quals))         ereport(ERROR,
(errcode(ERRCODE_WINDOWING_ERROR),
***************
*** 426,431 ****
--- 434,446 ----                                             locate_windowfunc(expr))));         }     }
+
+     if (pstate->p_hasWindowFuncs && hasSelfRefRTEs)
+         ereport(ERROR,
+                 (errcode(ERRCODE_INVALID_RECURSION),
+                  errmsg("window functions not allowed in a recursive query's
recursive term"),
+                  parser_errposition(pstate,
+                                     locate_windowfunc((Node *) qry)))); }
 /*

Regards,

-- 
Hitoshi Harada