On Fri, 2009-12-04 at 11:54 +0900, Itagaki Takahiro wrote:
> Here is an update partitioning syntax patch.
>
> A bug reported by Marko is fixed.
I will review and eventually commit this, if appropriate, though it is
3rd in my queue and will probably not be done for at least 2 weeks,
possibly 4 weeks.
Some brief review comments
* SQL:2008 contains PARTITION BY syntax, though in completely different
context. A possible alternative would be to use PARTITIONED BY. Please
justify either way. Possibly add short section to docs to explain this.
* There are relatively few comments in-line. Please can you provide a
README section for the code that explains how partitioning works? A
reviewer's guide would also be helpful to explain some of the design
decisions in particular places.
* All of the tests use about 4 partitions, which is the kind of thing
that makes me think the test coverage isn't wide enough. More tests
please. This would include operations on 0?, 1 and many partitions. We
also need more test failures, covering all the dumbass things people
will attempt. Also need partitioning by strange datatypes, arrays,
timestamps with timezones and stupidly long list values. Read Rob
Treat's humorous dissection of earlier partitioning features at PGcon to
see what needs to be covered.
* Docs. This is looking fairly solid, so please begin working on docs. I
won't hold you to this in next few weeks, but we know it needs doing.
* It is essential that we have large real-world(ish) performance test
results that proves this patch will work in the areas for which it is
intended. We need a test with 500 partitions, using at least 10MB
partitions to see if there are any scale-related issues. This test case
will help set targets for later developments because it will highlight
performance issues in planning, DDL and other areas. This doesn't have
to be by the patch author, but we need to see evidence that this patch
operates in its primary use case.
-- Simon Riggs www.2ndQuadrant.com
