On Thu, Apr 27, 2017 at 1:42 AM, Robert Haas <robertmh...@gmail.com> wrote:
>I spent some time today looking at these patches. It seems like there >is some more work still needed here to produce something committable >regardless of which way we go, but I am inclined to think that Amul's >patch is a better basis for work going forward than Nagata-san's >patch. Here are some general comments on the two patches: Thanks for your time. [...] > - Neither patch contains any documentation updates, which is bad. Fixed in the attached version. > > Nagata-san's patch also contains no regression tests. Amul's patch > does, but they need to be rebased, since they no longer apply, and I > think some other improvements are possible as well. It's probably not > necessary to re-test things like whether temp and non-temp tables can > be mixed within a partitioning hierarchy, but there should be tests > that tuple routing actually works. The case where it fails because no > matching partition exists should be tested as well. Also, the tests > should validate not only that FOR VALUES isn't accept when creating a > hash partition (which they do) but also that WITH (...) isn't accepted > for a range or list partition (which they do not). > Fixed in the attached version. [...] > - Amul's patch should perhaps update tab completion support: create > table foo1 partition of foo <tab> completes with "for values", but now > "with" will be another option. > Fixed in the attached version. > > - Amul's patch probably needs to validate the WITH () clause more > thoroughly. I bet you get a not-very-great error message if you leave > out "modulus" and no error at all if you leave out "remainder". > Thats not true, there will be syntax error if you leave modulus or remainder, see this: postgres=# CREATE TABLE hpart_2 PARTITION OF hash_parted WITH(modulus 4); ERROR: syntax error at or near ")" LINE 1: ...hpart_2 PARTITION OF hash_parted WITH(modulus 4); > > This is not yet a detailed review - I may be missing things, and > review and commentary from others is welcome. If there is no major > disagreement with the idea of moving forward using Amul's patch as a > base, then I will do a more detailed review of that patch (or, > hopefully, an updated version that addresses the above comments). > I have made a smaller change in earlier proposed syntax to create partition to be aligned with current range and list partition syntax, new syntax will be as follow: CREATE TABLE p1 PARTITION OF hash_parted FOR VALUES WITH (modulus 10, remainder 1); Regards, Amul
hash-partitioning_another_design-v2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
