Prev: patch - per-tablespace random_page_cost/seq_page_cost
Next: named parameters in SQL functions
From: Tom Lane on 5 Dec 2009 01:06
Andrew Gierth <andrew(a)tao11.riddles.org.uk> 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
silently. I don't have time right now to think about this issue in
detail, but if those are the alternatives I think the choice is clear.
Quietly adding a memory leak to code that used to work well is not
acceptable.

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: Hitoshi Harada on 5 Dec 2009 03:50
2009/12/5 Andrew Gierth <andrew(a)tao11.riddles.org.uk>:

> 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 an aggregate, and another
> one to return whether it's safe to scribble on the first argument
> (while it's currently the case that these two are equivalent, it would
> be good not to assume that).
>
> Comments? This is the most significant issue as I see it.

Yep, I agree. The most essential point on this is that external
functions refer to the struct members directly; we should provide
kinds of API.

> (Also, a function in contrib/tsearch2 that accesses wincontext wasn't
> updated by the patch.)

Thanks for noticing. I didn't know it.

> 2) Keywords

The discussion is:

http://archives.postgresql.org/message-id/e08cc0400911241703u4bf53ek1c3910605a3d8778(a)mail.gmail.com

and ideas are extracted from Tom's mail in the last year just before
committing the first window function patch, except for changing to
COL_NAME_KEYWORD rather than RESERVED_KEYWORD as suggested. The reason
I picked it up was only that it works. I cannot tell the side effect
of COL_NAME_KEYWORD but I guess we tend to avoid RESERVED_KEYWORD as
far as possible.

And the reason BETWEEN cannot work as function name any more is based
on bison's output shift/reduce conflict when SCONST follows BETWEEN in
frame_extent. I don't have clear example that makes this happen,
though.


> 3) Regression tests
>
> Testing that views work is OK as far as it goes, but I think that view
> definition should be left in place rather than dropped (possibly with
> even more variants) so that the deparse code gets properly tested too.
> (see the "rules" test)

OK,

> 4) Deparse output
>
> The code is forcing explicit casting on the offset expressions, i.e.
> the deparsed code looks like
>
>  ... ROWS BETWEEN 1::bigint PRECEDING AND 1::bigint FOLLOWING ...
>
> This looks a bit ugly; is it avoidable? At least for ROWS it should
> be, I think, since the type is known; even for RANGE, the type would
> be determined by the sort column.

Hmm, I'll change it as LIMIT clause does. Pass false as showimplicit
to get_rule_expr() maybe?

> 5) Documentation issues
>
> The entry for BETWEEN in the keywords appendix isn't updated.
> (Wouldn't it make more sense for this to be generated from the keyword
> list somehow?)

I heard that you don't need to send keywords appendix in the patch
because it is auto-generated, if I remember correctly.

>
> Spelling:
> -     current row. In <literal>ROWS</> mode this value means phisical row
> +     current row. In <literal>ROWS</> mode this value means physical row
> (this error appears twice)

Oops, thanks.

I'm now reworking as reviewed. The last issue is whether we accept
extension of frame types without RANGE support.

Regards,


--
Hitoshi Harada

--
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: Hitoshi Harada on 5 Dec 2009 05:17
2009/12/5 Hitoshi Harada <umi.tanuki(a)gmail.com>:

> 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 "iswindowagg" is output parameter to know whether the call
context is Agg or WindowAgg. Your proposal of APIs to know whether the
function is called as Aggregate or not is also a candidate to be, but
it seems out of this patch scope, so it doesn't touch anything.

Fix tsearch function is also included, as well as typo phisical ->
physical. Pass false to get_rule_expr() of value in
PRECEDING/FOLLOWING.

One thing for rule test, I checked existing regression test cases and
concluded DROP VIEW is necessary, or even VIEW test for a specific
feature is not needed. I remember your aggregate ORDER BY patch
contains "rules" test changes. However, since processing order of
regression tests is not predictable and may change AFAIK, I guess it
shouldn't add those changes in rules.out.


Regards.

--
Hitoshi Harada
From: Andrew Gierth on 5 Dec 2009 05:53
>>>>> "Hitoshi" == Hitoshi Harada <umi.tanuki(a)gmail.com> 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 "rules" test
Hitoshi> changes. However, since processing order of regression tests
Hitoshi> is not predictable and may change AFAIK, I guess it
Hitoshi> shouldn't add those changes in rules.out.

Actually, looking more closely, the way you have it currently works only
by chance - "rules" and "window" are running in parallel, therefore the
view creation in "window" can break the output of "rules".

The order of regression tests is set in parallel_schedule and
serial_schedule; it's unpredictable only for tests within the same
parallel group.

I think a modification of the schedule is needed here; the only other
option would be to move the view creation into a different test.

--
Andrew.

--
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: Andrew Gierth on 6 Dec 2009 17:40
>>>>> "Hitoshi" == Hitoshi Harada <umi.tanuki(a)gmail.com> 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 or WindowAgg. Your
Hitoshi> proposal of APIs to know whether the function is called as
Hitoshi> Aggregate or not is also a candidate to be, but it seems out
Hitoshi> of this patch scope, so it doesn't touch anything.

I don't really like the extra argument; aggregate functions should
almost never have to care about whether they're being called as window
functions rather than aggregate functions. And if it does care, I
don't see why this is the appropriate function for it. At the very
least the function should accept NULL for the "iswindowagg" pointer to
avoid useless variables in the caller.

So for this and the regression test problem mentioned in the other mail,
I'm setting this back to "waiting on author".

--
Andrew.

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: patch - per-tablespace random_page_cost/seq_page_cost
Next: named parameters in SQL functions