2009/12/7 Greg Smith :
> Which means that views created in the window test could absolutely cause the
> rules test to fail given a bad race condition. Either rules or window needs
> to be moved to another section of the test schedule. (I guess you could cut
> down the scope of "rules" to avoid th
Andrew Gierth wrote:
But what you have in the regression tests _now_ is, as far as I can
tell, only working by chance; with "rules" and "window" being in the
same parallel group, and window.sql creating a view, you have an
obvious race condition, unless I'm missing something.
You're right. ru
> "Hitoshi" == Hitoshi Harada writes:
>> So for this and the regression test problem mentioned in the other
>> mail, I'm setting this back to "waiting on author".
Hitoshi> In my humble opinion, view regression test is not necessary
Hitoshi> in both my patch and yours. At least window tes
2009/12/7 Andrew Gierth :
>> "Hitoshi" == Hitoshi Harada writes:
>
> Hitoshi> Attached is updated version. I added AggGetMemoryContext()
> Hitoshi> in executor/nodeAgg.h (though I'm not sure where to go...)
> Hitoshi> and its second argument "iswindowagg" is output parameter to
> Hitoshi>
> "Hitoshi" == Hitoshi Harada writes:
Hitoshi> Attached is updated version. I added AggGetMemoryContext()
Hitoshi> in executor/nodeAgg.h (though I'm not sure where to go...)
Hitoshi> and its second argument "iswindowagg" is output parameter to
Hitoshi> know whether the call context is Agg
> "Hitoshi" == Hitoshi Harada writes:
Hitoshi> One thing for rule test, I checked existing regression test
Hitoshi> cases and concluded DROP VIEW is necessary, or even VIEW
Hitoshi> test for a specific feature is not needed. I remember your
Hitoshi> aggregate ORDER BY patch contains "rule
2009/12/5 Hitoshi Harada :
> I'm now reworking as reviewed. The last issue is whether we accept
> extension of frame types without RANGE support.
Attached is updated version. I added AggGetMemoryContext() in
executor/nodeAgg.h (though I'm not sure where to go...) and its second
argument "iswindow
2009/12/5 Andrew Gierth :
> 1) Memory context handling for aggregate calls
>
> aggcontext = AggGetMemoryContext(fcinfo->context);
> if (!aggcontext)
> ereport(...);
>
> For completeness, there should be two other functions: one to simply
> return whether we are in fact being called as
Andrew Gierth writes:
> If we're going to change the interface in this way, there should, IMO,
> be enough of a change that old code fails to compile; e.g. by renaming
> wincontext to partition_context or some equivalent change.
Agreed --- if we have to break things, break them obviously not
sile
Functionally this patch looks excellent; correct format, applies
cleanly, passes regression, and I've been unable to find any issues
with the code itself. But I still have a concern over the interface
change, so I'm setting this back to "waiting on author" for now even
though it's really a matter f
> "Hitoshi" == Hitoshi Harada writes:
Hitoshi> As earlier mail, I added aggcontext to WindowAggState.
This issue (as detailed in this post):
http://archives.postgresql.org/pgsql-hackers/2009-11/msg01871.php
is currently the only significant outstanding issue in my review of this
patch. I t
2009/11/19 Andrew Gierth :
> Here's the rest of the review, as far as I've taken it given the
> problems with the code.
>
> The patch applied cleanly and includes regression tests but not docs.
>
> Small nitpicks: there are some comments not updated (e.g. the
> big one at the start of eval_windowag
Here's the rest of the review, as far as I've taken it given the
problems with the code.
The patch applied cleanly and includes regression tests but not docs.
Small nitpicks: there are some comments not updated (e.g. the
big one at the start of eval_windowaggregates). A couple of lines are
commen
2009/11/15 Andrew Gierth :
> My thinking is that the executor definitely shouldn't be relying on it
> being a specific node type, but should just ExecEvalExpr it on the
> first call and store the result; then you don't have to know whether
> it's a Const or Param node or a more complex expression.
> "Hitoshi" == Hitoshi Harada writes:
>>> (A question here: the spec allows (by my reading) the use of
>>> parameters in the window frame clause, i.e. BETWEEN $1 PRECEDING
>>> AND $2 FOLLOWING. Wouldn't it therefore make more sense to treat
>>> the values as Exprs, albeit very limited on
Thanks for your review.
2009/11/15 Andrew Gierth :
> Hi, I've started reviewing your patch.
>
> I've already found some things that need work:
>
> - missing _readWindowFrameDef function (all nodes that are output
> from parse analysis must have both _read and _out functions,
> otherwise views
2009/11/15 Tom Lane :
> Andrew Gierth writes:
>> (A question here: the spec allows (by my reading) the use of
>> parameters in the window frame clause, i.e. BETWEEN $1 PRECEDING AND
>> $2 FOLLOWING. Wouldn't it therefore make more sense to treat the
>> values as Exprs, albeit very limited ones, a
Andrew Gierth writes:
> (A question here: the spec allows (by my reading) the use of
> parameters in the window frame clause, i.e. BETWEEN $1 PRECEDING AND
> $2 FOLLOWING. Wouldn't it therefore make more sense to treat the
> values as Exprs, albeit very limited ones, and eval them at startup
> ra
Hi, I've started reviewing your patch.
I've already found some things that need work:
- missing _readWindowFrameDef function (all nodes that are output
from parse analysis must have both _read and _out functions,
otherwise views can't work)
- the A_Const nodes should probably be transfor
19 matches
Mail list logo
