Bug in 9.0Alpha4
in [PgSql]
|
From: Gokulakannan Somasundaram on 16 Mar 2010 08:14 Hi, I think, this should be the probable fix. There is agg_order in ParseFuncOrColumn, which should get passed on to transformAggregateCall and that should be placed in this call, instead of agg->aggorder. Thanks, Gokul. On Tue, Mar 16, 2010 at 5:19 PM, Gokulakannan Somasundaram < gokul007(a)gmail.com> wrote: > Hi, > I noticed a problem with the source code of 9.0Alpha 4. In parse_agg.c, > there is a call made to transformSortClause. > > 00098 torder = transformSortClause <http://doxygen.postgresql.org/parse__clause_8c.html#53199c36a198b5acf15a26fbd7311f79>(pstate, > 00099 agg->aggorder <http://doxygen.postgresql.org/structAggref.html#f477b6dc44bd60585cabf8608dcf2047>, > 00100 &tlist, > 00101 true /* fix unknowns */ , > 00102 true /* force SQL99 rules */ ); > 00103 > > > Here agg->aggorder should be a List of SortGroupClause pointers, whereas > transformSortClause expects the second argument as a list of SortBy > pointers. I verified the doxygen code by downloading the 9.0alpha4 version. > I am trying to understand this piece of code, while i thought i should > report this bug. > > Thanks, > Gokul. >
From: Alvaro Herrera on 16 Mar 2010 11:09 Gokulakannan Somasundaram escribi�: > Hi, > I noticed a problem with the source code of 9.0Alpha 4. In parse_agg.c, > there is a call made to transformSortClause. > > 00098 torder = transformSortClause > <http://doxygen.postgresql.org/parse__clause_8c.html#53199c36a198b5acf15a26fbd7311f79>(pstate, > 00099 agg->aggorder > <http://doxygen.postgresql.org/structAggref.html#f477b6dc44bd60585cabf8608dcf2047>, > 00100 &tlist, > 00101 true /* fix unknowns */ , > 00102 true /* force SQL99 rules */ ); > 00103 > > > Here agg->aggorder should be a List of SortGroupClause pointers, whereas > transformSortClause expects the second argument as a list of SortBy > pointers. I verified the doxygen code by downloading the 9.0alpha4 version. > I am trying to understand this piece of code, while i thought i should > report this bug. Wow, it seems you're correct. This is quite obscure -- the result of the compiler not being able to check the type of pointers we store in Lists :-( -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 16 Mar 2010 12:04 Gokulakannan Somasundaram <gokul007(a)gmail.com> writes: > I noticed a problem with the source code of 9.0Alpha 4. In parse_agg.c, > there is a call made to transformSortClause. > ... > Here agg->aggorder should be a List of SortGroupClause pointers, whereas > transformSortClause expects the second argument as a list of SortBy > pointers. Uh, no, read the comment at the head of transformAggregateCall: * parse_func.c has recognized the function as an aggregate, and has set * up all the fields of the Aggref except aggdistinct and agglevelsup. * However, the args list is just bare expressions, and the aggorder list * hasn't been transformed at all. * * Here we convert the args list into a targetlist by inserting TargetEntry * nodes, and then transform the aggorder and agg_distinct specifications to * produce lists of SortGroupClause nodes. (That might also result in adding * resjunk expressions to the targetlist.) transformSortClause is passed the untransformed aggorder list, which is in fact a list of SortBy nodes, and it returns the transformed list (SortGroupClause nodes), which is stored back into the aggorder field a bit further down. There are a number of regression tests that would fail in obvious ways if this code didn't work. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Gokulakannan Somasundaram on 16 Mar 2010 12:13 > > transformSortClause is passed the untransformed aggorder list, which is > in fact a list of SortBy nodes, and it returns the transformed list > (SortGroupClause nodes), which is stored back into the aggorder field > a bit further down. > > There are a number of regression tests that would fail in obvious ways > if this code didn't work. > > Right Tom. I got confused, because the comment at Aggref struct definition told that it is a list of SortGroupClause. May be you can update your comments there. Thanks, Gokul.
From: Tom Lane on 16 Mar 2010 15:56
Gokulakannan Somasundaram <gokul007(a)gmail.com> writes: >> transformSortClause is passed the untransformed aggorder list, which is >> in fact a list of SortBy nodes, and it returns the transformed list >> (SortGroupClause nodes), which is stored back into the aggorder field >> a bit further down. > Right Tom. I got confused, because the comment at Aggref struct definition > told that it is a list of SortGroupClause. May be you can update your > comments there. I think that comment is fine. The reason this is confusing is that ParseFuncOrColumn uses the Aggref node to carry a couple of things that logically are input parameters to transformAggregateCall(). Although this affects nothing else and is commented at both ends, apparently it's confusing anyway. When we were doing the ordered-aggregates patch, I considered passing all those values as explicit parameters to transformAggregateCall, and having it build the Aggref node from scratch and return it. However having seven or eight parameters to transformAggregateCall (and more in future if we ever add more features here) didn't really seem to be better style than abusing Aggref a bit. But maybe it is the best way after all. Thoughts? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |