On Thu, Apr 27, 2017 at 1:42 AM, Robert Haas <robertmhaas@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
