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:

remainder 1);


Attachment: 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:

Reply via email to