Обсуждение: Bug in 9.0Alpha4

Hi,<br />   I noticed a problem with the source code of 9.0Alpha 4. In parse_agg.c, there is a call made to
transformSortClause.<br /><br /><pre class="fragment"><font size="4">00098     torder = <a class="code"
href="http://doxygen.postgresql.org/parse__clause_8c.html#53199c36a198b5acf15a26fbd7311f79">transformSortClause</a>(pstate,<br
/>
<a name="l00099"></a>00099                                  agg-><a class="code"
href="http://doxygen.postgresql.org/structAggref.html#f477b6dc44bd60585cabf8608dcf2047">aggorder</a>,<br/><a
name="l00100"></a>00100                                 &tlist,<br /> 
<a name="l00101"></a>00101                                  <span class="keyword">true</span> <span class="comment">/*
fixunknowns */</span> ,<br /><a name="l00102"></a>00102                                  <span
class="keyword">true</span><span class="comment">/* force SQL99 rules */</span> );<br /> 
<a name="l00103"></a>00103 </font><br /><br /><br /></pre>   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
codeby downloading the 9.0alpha4 version. I am trying to understand this piece of code, while i thought i should report
thisbug.<br /><br />Thanks,<br />Gokul.<br /> 

Hi,<br />    I think, this should be the probable fix.<br /><br />There is agg_order in ParseFuncOrColumn, which should
getpassed on to transformAggregateCall and that should be placed in this call, instead of agg->aggorder.<br /><br
/>Thanks,<br/>Gokul.<br /><br /><div class="gmail_quote">On Tue, Mar 16, 2010 at 5:19 PM, Gokulakannan Somasundaram
<spandir="ltr"><<a href="mailto:gokul007@gmail.com">gokul007@gmail.com</a>></span> wrote:<br /><blockquote
class="gmail_quote"style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Hi,<br/>   I noticed a problem with the source code of 9.0Alpha 4. In parse_agg.c, there is a call made to
transformSortClause.<br /><br /><pre><font size="4">00098     torder = <a
href="http://doxygen.postgresql.org/parse__clause_8c.html#53199c36a198b5acf15a26fbd7311f79"
target="_blank">transformSortClause</a>(pstate,<br/> 

<a name="12766cf044e97507_l00099"></a>00099                                  agg-><a
href="http://doxygen.postgresql.org/structAggref.html#f477b6dc44bd60585cabf8608dcf2047"target="_blank">aggorder</a>,<br
/><aname="12766cf044e97507_l00100"></a>00100                                  &tlist,<br /> 

<a name="12766cf044e97507_l00101"></a>00101                                  <span>true</span> <span>/* fix unknowns
*/</span>,<br /><a name="12766cf044e97507_l00102"></a>00102                                  <span>true</span> <span>/*
forceSQL99 rules */</span> );<br /> 

<a name="12766cf044e97507_l00103"></a>00103 </font><br /><br /><br /></pre>   Here agg->aggorder should be a List of
SortGroupClausepointers, whereas transformSortClause expects the second argument as a list of SortBy pointers. I
verifiedthe doxygen code by downloading the 9.0alpha4 version. I am trying to understand this piece of code, while i
thoughti should report this bug.<br /><br />Thanks,<br />Gokul.<br /></blockquote></div><br /> 

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

Gokulakannan Somasundaram <gokul007@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
aggdistinctand agglevelsup.* However, the args list is just bare expressions, and the aggorder list* hasn't been
transformedat all.** Here we convert the args list into a targetlist by inserting TargetEntry* nodes, and then
transformthe aggorder and agg_distinct specifications to* produce lists of SortGroupClause nodes.  (That might also
resultin 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

Gokulakannan Somasundaram <gokul007@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

2010/3/17 Tom Lane <tgl@sss.pgh.pa.us>:
> Gokulakannan Somasundaram <gokul007@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?

Well, I think the point is args and aggorder are hidden in the Aggref
passed to transformAggregateCall, although they will be transformed in
the function. Isn't it enough to add more parameters for them
(agg_distinct is passed separately) and to leave the Aggref pointer
passing as present?

Regards,


--
Hitoshi Harada

Hitoshi Harada <umi.tanuki@gmail.com> writes:
> 2010/3/17 Tom Lane <tgl@sss.pgh.pa.us>:
>> 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?

> Well, I think the point is args and aggorder are hidden in the Aggref
> passed to transformAggregateCall, although they will be transformed in
> the function. Isn't it enough to add more parameters for them
> (agg_distinct is passed separately) and to leave the Aggref pointer
> passing as present?

Yeah, that's probably the least complicated solution.  Will fix.
        regards, tom lane