From: | Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> |
---|---|
To: | Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: add more frame types in window functions (ROWS) |
Date: | 2009-12-05 08:50:33 |
Message-ID: | e08cc0400912050050m63d3be04vc13f584c1552b8de@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
2009/12/5 Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)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@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
From | Date | Subject | |
---|---|---|---|
Next Message | Simon Riggs | 2009-12-05 09:51:23 | Re: Summary and Plan for Hot Standby |
Previous Message | Daniel Farina | 2009-12-05 08:32:04 | Re: [PATCH 4/4] Add tests to dblink covering use of COPY TO FUNCTION |