On Thu, Apr 20, 2017 at 4:27 PM, Robert Haas <robertmh...@gmail.com> wrote: > So, I think we really need something like the syntax in Amul's patch > in order for this to work at all. Of course, the details can be > changed according to what seems best but I think the overall picture > is about right.
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: - As noted above, the syntax implemented by Amul's patch allows us to know the final partition constraint right away. Nagata-san's proposed syntax does not do that. Also, Amul's syntax allows for a way to split partitions (awkwardly, but we can improve it later); Nagata-san's doesn't provide any method at all. - Amul's patch derives the hash function to be used from the relevant hash opclass, whereas Nagata-san's patch requires the user to specify it explicitly. I think that there is no real use case for a user providing a custom hash function, and that using the opclass machinery to derive the function to be used is better. If a user DOES want to provide their own, they can always create a custom opclass with the appropriate support function and specify that it should be used when creating a hash-partitioned table, but most users will be happy for the system to supply the appropriate function automatically. - In Nagata-san's patch, convert_expr_for_hash() looks up a function called "abs" and an operator called "%" by name, which is not a good idea. We don't want to just find whatever is in the current search path; we want to make sure we're using the system-defined operators that we intend to be using. Amul's patch builds the constraint using a hard-coded internal function OID, F_SATISFIES_HASH_PARTITION. That's a lot more robust, and it's also likely to be faster because, in Amul's patch, we only call one function at the SQL level (satisfies_hash_partition), whereas in Nagata-san's patch, we'll end up calling three (abs, %, =). Nagata-san's version of get_qual_for_hash is implicated in this problem, too: it's looking up the operator to use based on the operator name (=) rather than the opclass properties. Note that the existing get_qual_for_list() and get_qual_for_range() use opclass properties, as does Amul's patch. - Nagata-san's patch only supports hash partitioning based on a single column, and that column must be NOT NULL. Amul's patch does not have these restrictions. - Neither patch contains any documentation updates, which is bad. 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). - When I try to do even something pretty trivial with Nagata-san's patches, it crashes: rhaas=# create table foo (a int, b text) partition by hash (a) partitions 7 using hashint4; CREATE TABLE rhaas=# create table foo1 partition of foo; <server crash> The ruleutils.c support in Nagata-san's patch is broken. If you execute the non-crashing statement from the above example and then run pg_dump, it doesn't dump "partitions 7 using hashint4", which means that the syntax in the dump is invalid. - Neither patch does anything about the fact that constraint exclusion won't work for hash partitioning. I mentioned this issue upthread in the last paragraph of http://postgr.es/m/CA+Tgmob7RsN5A=ehgYbLPx--c5CmptrK-dB=y-v--o+tkyf...@mail.gmail.com and I think it's imperative that we fix it in some way before we think about committing any of this. I think that needs to be done by extending relation_excluded_by_constraints() to have some specific smarts about hash partitioning, and maybe other kinds of partitioning as well (because it could probably be made much faster for list and range partitioning, too). - 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. - 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". 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). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
