Re: Windowing Function Patch Review -> Standard Conformance

2008/12/28 David Rowley <dgrowley(at)gmail(dot)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