Thread
Commits
-
Add hash partitioning.
- 1aba8e651ac3 11.0 landed
-
Add sanity check for pg_proc.provariadic
- 35f059e9bdfb 11.0 landed
-
Add hash_combine64.
- b7f3eb31405f 11.0 landed
-
Make RelationGetPartitionDispatchInfo expand depth-first.
- 77b6b5e9ceca 11.0 cited
-
Introduce 64-bit hash functions with a 64-bit seed.
- 81c5e46c490e 11.0 cited
-
pg_dump: Add a --load-via-partition-root option.
- 23d7680d04b9 11.0 cited
-
Re: [POC] hash partitioning
Yang Jie <yangjie@highgo.com> — 2017-08-28T07:33:46Z
Hello Looking at your hash partitioning syntax, I implemented a hash partition in a more concise way, with no need to determine the number of sub-tables, and dynamically add partitions. Description The hash partition's implement is on the basis of the original range / list partition,and using similar syntax. To create a partitioned table ,use: CREATE TABLE h (id int) PARTITION BY HASH(id); The partitioning key supports only one value, and I think the partition key can support multiple values, which may be difficult to implement when querying, but it is not impossible. A partition table can be create as bellow: CREATE TABLE h1 PARTITION OF h; CREATE TABLE h2 PARTITION OF h; CREATE TABLE h3 PARTITION OF h; FOR VALUES clause cannot be used, and the partition bound is calclulated automatically as partition index of single integer value. An inserted record is stored in a partition whose index equals DatumGetUInt32(OidFunctionCall1(lookup_type_cache(key->parttypid[0], TYPECACHE_HASH_PROC)->hash_proc, values[0])) % nparts/* Number of partitions */ ; In the above example, this is DatumGetUInt32(OidFunctionCall1(lookup_type_cache(key->parttypid[0], TYPECACHE_HASH_PROC)->hash_proc, id)) % 3; postgres=# insert into h select generate_series(1,20); INSERT 0 20 postgres=# select tableoid::regclass,* from h; tableoid | id ----------+---- h1 | 3 h1 | 5 h1 | 17 h1 | 19 h2 | 2 h2 | 6 h2 | 7 h2 | 11 h2 | 12 h2 | 14 h2 | 15 h2 | 18 h2 | 20 h3 | 1 h3 | 4 h3 | 8 h3 | 9 h3 | 10 h3 | 13 h3 | 16 (20 rows) The number of partitions here can be dynamically added, and if a new partition is created, the number of partitions changes, the calculated target partitions will change, and the same data is not reasonable in different partitions,So you need to re-calculate the existing data and insert the target partition when you create a new partition. postgres=# create table h4 partition of h; CREATE TABLE postgres=# select tableoid::regclass,* from h; tableoid | id ----------+---- h1 | 5 h1 | 17 h1 | 19 h1 | 6 h1 | 12 h1 | 8 h1 | 13 h2 | 11 h2 | 14 h3 | 1 h3 | 9 h3 | 2 h3 | 15 h4 | 3 h4 | 7 h4 | 18 h4 | 20 h4 | 4 h4 | 10 h4 | 16 (20 rows) When querying the data, the hash partition uses the same algorithm as the insertion, and filters out the table that does not need to be scanned. postgres=# explain analyze select * from h where id = 1; QUERY PLAN ---------------------------------------------------------------------------------------------------- Append (cost=0.00..41.88 rows=13 width=4) (actual time=0.020..0.023 rows=1 loops=1) -> Seq Scan on h3 (cost=0.00..41.88 rows=13 width=4) (actual time=0.013..0.016 rows=1 loops=1) Filter: (id = 1) Rows Removed by Filter: 3 Planning time: 0.346 ms Execution time: 0.061 ms (6 rows) postgres=# explain analyze select * from h where id in (1,5);; QUERY PLAN ---------------------------------------------------------------------------------------------------- Append (cost=0.00..83.75 rows=52 width=4) (actual time=0.016..0.028 rows=2 loops=1) -> Seq Scan on h1 (cost=0.00..41.88 rows=26 width=4) (actual time=0.015..0.018 rows=1 loops=1) Filter: (id = ANY ('{1,5}'::integer[])) Rows Removed by Filter: 6 -> Seq Scan on h3 (cost=0.00..41.88 rows=26 width=4) (actual time=0.005..0.007 rows=1 loops=1) Filter: (id = ANY ('{1,5}'::integer[])) Rows Removed by Filter: 3 Planning time: 0.720 ms Execution time: 0.074 ms (9 rows) postgres=# explain analyze select * from h where id = 1 or id = 5;; QUERY PLAN ---------------------------------------------------------------------------------------------------- Append (cost=0.00..96.50 rows=50 width=4) (actual time=0.017..0.078 rows=2 loops=1) -> Seq Scan on h1 (cost=0.00..48.25 rows=25 width=4) (actual time=0.015..0.019 rows=1 loops=1) Filter: ((id = 1) OR (id = 5)) Rows Removed by Filter: 6 -> Seq Scan on h3 (cost=0.00..48.25 rows=25 width=4) (actual time=0.005..0.010 rows=1 loops=1) Filter: ((id = 1) OR (id = 5)) Rows Removed by Filter: 3 Planning time: 0.396 ms Execution time: 0.139 ms (9 rows) Can not detach / attach / drop partition table. Best regards, young yonj1e.github.io yangjie@highgo.com -
Re: [POC] hash partitioning
Yugo Nagata <nagata@sraoss.co.jp> — 2017-08-28T08:30:15Z
Hi young, On Mon, 28 Aug 2017 15:33:46 +0800 "yangjie@highgo.com" <yangjie@highgo.com> wrote: > Hello > > Looking at your hash partitioning syntax, I implemented a hash partition in a more concise way, with no need to determine the number of sub-tables, and dynamically add partitions. I think it is great work, but the current consensus about hash-partitioning supports Amul's patch[1], in which the syntax is different from the my original proposal. So, you will have to read Amul's patch and make a discussion if you still want to propose your implementation. Regards, [1] https://www.postgresql.org/message-id/CAAJ_b965A2oog=6eFUhELexL3RmgFssB3G7LwkVA1bw0WUJJoA@mail.gmail.com > > Description > > The hash partition's implement is on the basis of the original range / list partition,and using similar syntax. > > To create a partitioned table ,use: > > CREATE TABLE h (id int) PARTITION BY HASH(id); > > The partitioning key supports only one value, and I think the partition key can support multiple values, > which may be difficult to implement when querying, but it is not impossible. > > A partition table can be create as bellow: > > CREATE TABLE h1 PARTITION OF h; > CREATE TABLE h2 PARTITION OF h; > CREATE TABLE h3 PARTITION OF h; > > FOR VALUES clause cannot be used, and the partition bound is calclulated automatically as partition index of single integer value. > > An inserted record is stored in a partition whose index equals > DatumGetUInt32(OidFunctionCall1(lookup_type_cache(key->parttypid[0], TYPECACHE_HASH_PROC)->hash_proc, values[0])) % nparts/* Number of partitions */ > ; > In the above example, this is DatumGetUInt32(OidFunctionCall1(lookup_type_cache(key->parttypid[0], TYPECACHE_HASH_PROC)->hash_proc, id)) % 3; > > postgres=# insert into h select generate_series(1,20); > INSERT 0 20 > postgres=# select tableoid::regclass,* from h; > tableoid | id > ----------+---- > h1 | 3 > h1 | 5 > h1 | 17 > h1 | 19 > h2 | 2 > h2 | 6 > h2 | 7 > h2 | 11 > h2 | 12 > h2 | 14 > h2 | 15 > h2 | 18 > h2 | 20 > h3 | 1 > h3 | 4 > h3 | 8 > h3 | 9 > h3 | 10 > h3 | 13 > h3 | 16 > (20 rows) > > The number of partitions here can be dynamically added, and if a new partition is created, the number of partitions changes, the calculated target partitions will change, and the same data is not reasonable in different partitions,So you need to re-calculate the existing data and insert the target partition when you create a new partition. > > postgres=# create table h4 partition of h; > CREATE TABLE > postgres=# select tableoid::regclass,* from h; > tableoid | id > ----------+---- > h1 | 5 > h1 | 17 > h1 | 19 > h1 | 6 > h1 | 12 > h1 | 8 > h1 | 13 > h2 | 11 > h2 | 14 > h3 | 1 > h3 | 9 > h3 | 2 > h3 | 15 > h4 | 3 > h4 | 7 > h4 | 18 > h4 | 20 > h4 | 4 > h4 | 10 > h4 | 16 > (20 rows) > > When querying the data, the hash partition uses the same algorithm as the insertion, and filters out the table that does not need to be scanned. > > postgres=# explain analyze select * from h where id = 1; > QUERY PLAN > ---------------------------------------------------------------------------------------------------- > Append (cost=0.00..41.88 rows=13 width=4) (actual time=0.020..0.023 rows=1 loops=1) > -> Seq Scan on h3 (cost=0.00..41.88 rows=13 width=4) (actual time=0.013..0.016 rows=1 loops=1) > Filter: (id = 1) > Rows Removed by Filter: 3 > Planning time: 0.346 ms > Execution time: 0.061 ms > (6 rows) > > postgres=# explain analyze select * from h where id in (1,5);; > QUERY PLAN > ---------------------------------------------------------------------------------------------------- > Append (cost=0.00..83.75 rows=52 width=4) (actual time=0.016..0.028 rows=2 loops=1) > -> Seq Scan on h1 (cost=0.00..41.88 rows=26 width=4) (actual time=0.015..0.018 rows=1 loops=1) > Filter: (id = ANY ('{1,5}'::integer[])) > Rows Removed by Filter: 6 > -> Seq Scan on h3 (cost=0.00..41.88 rows=26 width=4) (actual time=0.005..0.007 rows=1 loops=1) > Filter: (id = ANY ('{1,5}'::integer[])) > Rows Removed by Filter: 3 > Planning time: 0.720 ms > Execution time: 0.074 ms > (9 rows) > > postgres=# explain analyze select * from h where id = 1 or id = 5;; > QUERY PLAN > ---------------------------------------------------------------------------------------------------- > Append (cost=0.00..96.50 rows=50 width=4) (actual time=0.017..0.078 rows=2 loops=1) > -> Seq Scan on h1 (cost=0.00..48.25 rows=25 width=4) (actual time=0.015..0.019 rows=1 loops=1) > Filter: ((id = 1) OR (id = 5)) > Rows Removed by Filter: 6 > -> Seq Scan on h3 (cost=0.00..48.25 rows=25 width=4) (actual time=0.005..0.010 rows=1 loops=1) > Filter: ((id = 1) OR (id = 5)) > Rows Removed by Filter: 3 > Planning time: 0.396 ms > Execution time: 0.139 ms > (9 rows) > > Can not detach / attach / drop partition table. > > Best regards, > young > > > yonj1e.github.io > yangjie@highgo.com -- Yugo Nagata <nagata@sraoss.co.jp> -
Re: [POC] hash partitioning
Yang Jie <yangjie@highgo.com> — 2017-08-29T02:19:12Z
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-09-04T10:38:45Z
I've updated patch to use an extended hash function (Commit # 81c5e46c490e2426db243eada186995da5bb0ba7) for the partitioning. Regards, Amul On Thu, Jul 27, 2017 at 5:11 PM, amul sul <sulamul@gmail.com> wrote: > Attaching newer patches rebased against the latest master head. Thanks ! > > Regards, > Amul >
-
Re: [POC] hash partitioning
Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> — 2017-09-05T09:13:07Z
On Mon, Sep 4, 2017 at 4:08 PM, amul sul <sulamul@gmail.com> wrote: > I've updated patch to use an extended hash function (Commit # > 81c5e46c490e2426db243eada186995da5bb0ba7) for the partitioning. > > I have done some testing with these patches, everything looks fine, attaching sql and out file for reference. Thanks & Regards, Rajkumar Raghuwanshi QMG, EnterpriseDB Corporation
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-09-08T01:15:44Z
On Mon, Sep 4, 2017 at 6:38 AM, amul sul <sulamul@gmail.com> wrote: > I've updated patch to use an extended hash function (Commit # > 81c5e46c490e2426db243eada186995da5bb0ba7) for the partitioning. Committed 0001 after noticing that Jeevan Ladhe also found that change convenient for default partitioning. I made a few minor cleanups; hopefully I didn't break anything. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-09-08T12:40:25Z
On Fri, Sep 8, 2017 at 6:45 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Sep 4, 2017 at 6:38 AM, amul sul <sulamul@gmail.com> wrote: > > I've updated patch to use an extended hash function (Commit # > > 81c5e46c490e2426db243eada186995da5bb0ba7) for the partitioning. > > Committed 0001 after noticing that Jeevan Ladhe also found that change > convenient for default partitioning. I made a few minor cleanups; > hopefully I didn't break anything. > Thanks you. Rebased 0002 against this commit & renamed to 0001, PFA. Regards, Amul
-
Re: [POC] hash partitioning
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-09-11T08:17:22Z
On Fri, Sep 8, 2017 at 6:10 PM, amul sul <sulamul@gmail.com> wrote: > On Fri, Sep 8, 2017 at 6:45 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> >> On Mon, Sep 4, 2017 at 6:38 AM, amul sul <sulamul@gmail.com> wrote: >> > I've updated patch to use an extended hash function (Commit # >> > 81c5e46c490e2426db243eada186995da5bb0ba7) for the partitioning. >> >> Committed 0001 after noticing that Jeevan Ladhe also found that change >> convenient for default partitioning. I made a few minor cleanups; >> hopefully I didn't break anything. > > > Thanks you. > > Rebased 0002 against this commit & renamed to 0001, PFA. Given that we have default partition support now, I am wondering whether hash partitioned tables also should have default partitions. The way we have structured hash partitioning syntax, there can be "holes" in partitions. Default partition would help plug those holes. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-09-11T11:43:29Z
On Mon, Sep 11, 2017 at 4:17 AM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: >> Rebased 0002 against this commit & renamed to 0001, PFA. > > Given that we have default partition support now, I am wondering > whether hash partitioned tables also should have default partitions. > The way we have structured hash partitioning syntax, there can be > "holes" in partitions. Default partition would help plug those holes. Yeah, I was thinking about that, too. On the one hand, it seems like it's solving the problem the wrong way: if you've set up hash partitioning properly, you shouldn't have any holes. On the other hand, supporting it probably wouldn't cost anything noticeable and might make things seem more consistent. I'm not sure which way to jump on this one. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-09-11T12:00:53Z
Robert Haas wrote: > On Mon, Sep 11, 2017 at 4:17 AM, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: > >> Rebased 0002 against this commit & renamed to 0001, PFA. > > > > Given that we have default partition support now, I am wondering > > whether hash partitioned tables also should have default partitions. > > The way we have structured hash partitioning syntax, there can be > > "holes" in partitions. Default partition would help plug those holes. > > Yeah, I was thinking about that, too. On the one hand, it seems like > it's solving the problem the wrong way: if you've set up hash > partitioning properly, you shouldn't have any holes. On the other > hand, supporting it probably wouldn't cost anything noticeable and > might make things seem more consistent. I'm not sure which way to > jump on this one. How difficult/tedious/troublesome would be to install the missing partitions if you set hash partitioning with a default partition and only later on notice that some partitions are missing? I think if the answer is that you need to exclusive-lock something for a long time and this causes a disruption in production systems, then it's better not to allow a default partition at all and just force all the hash partitions to be there from the start. On the other hand, if you can get tuples out of the default partition into their intended regular partitions without causing any disruption, then it seems okay to allow default partitions in hash partitioning setups. (I, like many others, was unable to follow the default partition stuff as closely as I would have liked.) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-09-11T14:12:57Z
On Mon, Sep 11, 2017 at 8:00 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > How difficult/tedious/troublesome would be to install the missing > partitions if you set hash partitioning with a default partition and > only later on notice that some partitions are missing? I think if the > answer is that you need to exclusive-lock something for a long time and > this causes a disruption in production systems, then it's better not to > allow a default partition at all and just force all the hash partitions > to be there from the start. > > On the other hand, if you can get tuples out of the default partition > into their intended regular partitions without causing any disruption, > then it seems okay to allow default partitions in hash partitioning > setups. I think there's no real use case for default partitioning, and yeah, you do need exclusive locks to repartition things (whether hash partitioning or otherwise). It would be nice to fix that eventually, but it's hard, because the executor has to cope with the floor moving under it, and as of today, it really can't cope with that at all - not because of partitioning specifically, but because of existing design decisions that will require a lot of work (and probably arguing) to revisit. I think the way to get around the usability issues for hash partitioning is to eventually add some syntax that does things like (1) automatically create the table with N properly-configured partitions, (2) automatically split an existing partition into N pieces, and (3) automatically rewrite the whole table using a different partition count. People seem to find the hash partitioning stuff a little arcane. I don't want to discount that confusion with some sort of high-handed "I know better" attitude, I think the interface that users will actually see can end up being pretty straightforward. The complexity that is there in the syntax is to allow pg_upgrade and pg_dump/restore to work properly. But users don't necessarily have to use the same syntax that pg_dump does, just as you can say CREATE INDEX ON a (b) and let the system specify the index name, but at dump time the index name is specified explicitly. > (I, like many others, was unable to follow the default partition stuff > as closely as I would have liked.) Uh, sorry about that. Would it help if I wrote a blog post on it or something? The general idea is simple: any tuples that don't route to any other partition get routed to the default partition. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-09-11T16:15:04Z
On Mon, Sep 11, 2017 at 5:30 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Robert Haas wrote: > > On Mon, Sep 11, 2017 at 4:17 AM, Ashutosh Bapat > > <ashutosh.bapat@enterprisedb.com> wrote: > > >> Rebased 0002 against this commit & renamed to 0001, PFA. > > > > > > Given that we have default partition support now, I am wondering > > > whether hash partitioned tables also should have default partitions. > > > The way we have structured hash partitioning syntax, there can be > > > "holes" in partitions. Default partition would help plug those holes. > > > > Yeah, I was thinking about that, too. On the one hand, it seems like > > it's solving the problem the wrong way: if you've set up hash > > partitioning properly, you shouldn't have any holes. On the other > > hand, supporting it probably wouldn't cost anything noticeable and > > might make things seem more consistent. I'm not sure which way to > > jump on this one. > > How difficult/tedious/troublesome would be to install the missing > partitions if you set hash partitioning with a default partition and > only later on notice that some partitions are missing? I think if the > answer is that you need to exclusive-lock something for a long time and > this causes a disruption in production systems, then it's better not to > allow a default partition at all and just force all the hash partitions > to be there from the start. > > I am also leaning toward not to support a default partition for a hash partitioned table. The major drawback I can see is the constraint get created on the default partition table. IIUC, constraint on the default partition table are just negation of partition constraint on all its sibling partitions. Consider a hash partitioned table having partitions with (modulus 64, remainder 0) , ...., (modulus 64, remainder 62) hash bound and partition column are col1, col2,...,so on, then constraint for the default partition will be : NOT( (satisfies_hash_partition(64, 0, hash_fn1(col1), hash_fn2(col2), ...) && ... && satisfies_hash_partition(64, 62, hash_fn1(col1),hash_fn2(col2), ...)) Which will be much harmful to the performance than any other partitioning strategy because it calculate a hash for the same partitioning key multiple time. We could overcome this by having an another SQL function (e.g satisfies_default_hash_partition) which calculates hash value once and checks the remainder, and that would be a different path from the current default partition framework. Regards, Amul -
Re: [POC] hash partitioning
Jesper Pedersen <jesper.pedersen@redhat.com> — 2017-09-13T14:13:40Z
Hi Amul, On 09/08/2017 08:40 AM, amul sul wrote: > Rebased 0002 against this commit & renamed to 0001, PFA. > This patch needs a rebase. Best regards, Jesper
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-09-14T08:58:49Z
On Wed, Sep 13, 2017 at 7:43 PM, Jesper Pedersen <jesper.pedersen@redhat.com > wrote: > Hi Amul, > > On 09/08/2017 08:40 AM, amul sul wrote: > >> Rebased 0002 against this commit & renamed to 0001, PFA. >> >> > This patch needs a rebase. > > Thanks for your note. Attached is the patch rebased on the latest master head. Also added error on creating d efault partition for the hash partitioned table , and updated document & test script for the same. Regards, Amul
-
Re: [POC] hash partitioning
Jesper Pedersen <jesper.pedersen@redhat.com> — 2017-09-14T15:39:26Z
Hi Amul, On 09/14/2017 04:58 AM, amul sul wrote: > On Wed, Sep 13, 2017 at 7:43 PM, Jesper Pedersen <jesper.pedersen@redhat.com >> This patch needs a rebase. >> >> > Thanks for your note. > > Attached is the patch rebased on the latest master head. > Also added error on creating default partition for the hash partitioned table, > and updated document & test script for the same. > Thanks ! When I do CREATE TABLE mytab ( a integer NOT NULL, b integer NOT NULL, c integer, d integer ) PARTITION BY HASH (b); and create 64 partitions; CREATE TABLE mytab_p00 PARTITION OF mytab FOR VALUES WITH (MODULUS 64, REMAINDER 0); ... CREATE TABLE mytab_p63 PARTITION OF mytab FOR VALUES WITH (MODULUS 64, REMAINDER 63); and associated indexes CREATE INDEX idx_p00 ON mytab_p00 USING btree (b, a); ... CREATE INDEX idx_p63 ON mytab_p63 USING btree (b, a); Populate the database, and do ANALYZE. Given EXPLAIN (ANALYZE, VERBOSE, BUFFERS ON) SELECT a, b, c, d FROM mytab WHERE b = 42 gives Append -> Index Scan using idx_p00 (cost rows=7) (actual rows=0) ... -> Index Scan using idx_p63 (cost rows=7) (actual rows=0) E.g. all partitions are being scanned. Of course one partition will contain the rows I'm looking for. Best regards, Jesper
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-09-14T16:05:16Z
On Thu, Sep 14, 2017 at 11:39 AM, Jesper Pedersen <jesper.pedersen@redhat.com> wrote: > When I do > > CREATE TABLE mytab ( > a integer NOT NULL, > b integer NOT NULL, > c integer, > d integer > ) PARTITION BY HASH (b); > > and create 64 partitions; > > CREATE TABLE mytab_p00 PARTITION OF mytab FOR VALUES WITH (MODULUS 64, > REMAINDER 0); > ... > CREATE TABLE mytab_p63 PARTITION OF mytab FOR VALUES WITH (MODULUS 64, > REMAINDER 63); > > and associated indexes > > CREATE INDEX idx_p00 ON mytab_p00 USING btree (b, a); > ... > CREATE INDEX idx_p63 ON mytab_p63 USING btree (b, a); > > Populate the database, and do ANALYZE. > > Given > > EXPLAIN (ANALYZE, VERBOSE, BUFFERS ON) SELECT a, b, c, d FROM mytab WHERE b > = 42 > > gives > > Append > -> Index Scan using idx_p00 (cost rows=7) (actual rows=0) > ... > -> Index Scan using idx_p63 (cost rows=7) (actual rows=0) > > E.g. all partitions are being scanned. Of course one partition will contain > the rows I'm looking for. Yeah, we need Amit Langote's work in http://postgr.es/m/098b9c71-1915-1a2a-8d52-1a7a50ce79e8@lab.ntt.co.jp to land and this patch to be adapted to make use of it. I think that's the major thing still standing in the way of this. Concerns were also raised about not having a way to see the hash function, but we fixed that in 81c5e46c490e2426db243eada186995da5bb0ba7 and hopefully this patch has been updated to use a seed (I haven't looked yet). And there was a concern about hash functions not being portable, but the conclusion of that was basically that most people think --load-via-partition-root will be a satisfactory workaround for cases where that becomes a problem (cf. commit 23d7680d04b958de327be96ffdde8f024140d50e). So this is the major remaining issue that I know about. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Jesper Pedersen <jesper.pedersen@redhat.com> — 2017-09-14T16:53:12Z
Hi, On 09/14/2017 12:05 PM, Robert Haas wrote: > On Thu, Sep 14, 2017 at 11:39 AM, Jesper Pedersen > <jesper.pedersen@redhat.com> wrote: >> When I do >> >> CREATE TABLE mytab ( >> a integer NOT NULL, >> b integer NOT NULL, >> c integer, >> d integer >> ) PARTITION BY HASH (b); >> >> and create 64 partitions; >> >> CREATE TABLE mytab_p00 PARTITION OF mytab FOR VALUES WITH (MODULUS 64, >> REMAINDER 0); >> ... >> CREATE TABLE mytab_p63 PARTITION OF mytab FOR VALUES WITH (MODULUS 64, >> REMAINDER 63); >> >> and associated indexes >> >> CREATE INDEX idx_p00 ON mytab_p00 USING btree (b, a); >> ... >> CREATE INDEX idx_p63 ON mytab_p63 USING btree (b, a); >> >> Populate the database, and do ANALYZE. >> >> Given >> >> EXPLAIN (ANALYZE, VERBOSE, BUFFERS ON) SELECT a, b, c, d FROM mytab WHERE b >> = 42 >> >> gives >> >> Append >> -> Index Scan using idx_p00 (cost rows=7) (actual rows=0) >> ... >> -> Index Scan using idx_p63 (cost rows=7) (actual rows=0) >> >> E.g. all partitions are being scanned. Of course one partition will contain >> the rows I'm looking for. > > Yeah, we need Amit Langote's work in > http://postgr.es/m/098b9c71-1915-1a2a-8d52-1a7a50ce79e8@lab.ntt.co.jp > to land and this patch to be adapted to make use of it. I think > that's the major thing still standing in the way of this. Concerns > were also raised about not having a way to see the hash function, but > we fixed that in 81c5e46c490e2426db243eada186995da5bb0ba7 and > hopefully this patch has been updated to use a seed (I haven't looked > yet). And there was a concern about hash functions not being > portable, but the conclusion of that was basically that most people > think --load-via-partition-root will be a satisfactory workaround for > cases where that becomes a problem (cf. commit > 23d7680d04b958de327be96ffdde8f024140d50e). So this is the major > remaining issue that I know about. > Thanks for the information, Robert ! Best regards, Jesper
-
Re: [POC] hash partitioning
David Fetter <david@fetter.org> — 2017-09-14T16:54:44Z
On Mon, Sep 11, 2017 at 07:43:29AM -0400, Robert Haas wrote: > On Mon, Sep 11, 2017 at 4:17 AM, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: > >> Rebased 0002 against this commit & renamed to 0001, PFA. > > > > Given that we have default partition support now, I am wondering > > whether hash partitioned tables also should have default > > partitions. The way we have structured hash partitioning syntax, > > there can be "holes" in partitions. Default partition would help > > plug those holes. > > Yeah, I was thinking about that, too. On the one hand, it seems > like it's solving the problem the wrong way: if you've set up hash > partitioning properly, you shouldn't have any holes. Should we be pointing the gun away from people's feet by making hash partitions that cover the space automagically when the partitioning scheme[1] is specified? In other words, do we have a good reason to have only some of the hash partitions so defined by default? Best, David. [1] For now, that's just the modulus, but the PoC included specifying hashing functions, so I assume other ways to specify the partitioning scheme could eventually be proposed. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-09-14T16:56:57Z
On Thu, Sep 14, 2017 at 12:54 PM, David Fetter <david@fetter.org> wrote: > Should we be pointing the gun away from people's feet by making hash > partitions that cover the space automagically when the partitioning > scheme[1] is specified? In other words, do we have a good reason to have > only some of the hash partitions so defined by default? Sure, we can add some convenience syntax for that, but I'd like to get the basic stuff working before doing that kind of polishing. If nothing else, I assume Keith Fiske's pg_partman will provide a way to magically DTRT about an hour after this goes in. But probably we can do better in core easily enough. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Jesper Pedersen <jesper.pedersen@redhat.com> — 2017-09-14T17:07:11Z
On 09/14/2017 12:56 PM, Robert Haas wrote: > On Thu, Sep 14, 2017 at 12:54 PM, David Fetter <david@fetter.org> wrote: >> Should we be pointing the gun away from people's feet by making hash >> partitions that cover the space automagically when the partitioning >> scheme[1] is specified? In other words, do we have a good reason to have >> only some of the hash partitions so defined by default? > > Sure, we can add some convenience syntax for that, but I'd like to get > the basic stuff working before doing that kind of polishing. > > If nothing else, I assume Keith Fiske's pg_partman will provide a way > to magically DTRT about an hour after this goes in. But probably we > can do better in core easily enough. > Yeah, it would be nice to have a syntax like ) PARTITION BY HASH (col) WITH (AUTO_CREATE = 64); But then there also needs to be a way to create the 64 associated indexes too for everything to be easy. Best regards, Jesper
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-09-14T17:52:27Z
On Thu, Sep 14, 2017 at 1:07 PM, Jesper Pedersen <jesper.pedersen@redhat.com> wrote: > Yeah, it would be nice to have a syntax like > > ) PARTITION BY HASH (col) WITH (AUTO_CREATE = 64); > > But then there also needs to be a way to create the 64 associated indexes > too for everything to be easy. Well, for that, there's this proposal: http://postgr.es/m/c8fe4f6b-ff46-aae0-89e3-e936a35f0cfd@postgrespro.ru As several people have right pointed out, there's a lot of work to be done on partitioning it to get it to where we want it to be. Even in v10, it's got significant benefits, such as much faster bulk-loading, but I don't hear anybody disputing the notion that a lot more work is needed. The good news is that a lot of that work is already in progress; the bad news is that a lot of that work is not done yet. But I think that's OK. We can't solve every problem at once, and I think we're moving things along here at a reasonably brisk pace. That didn't stop me from complaining bitterly to someone just yesterday that we aren't moving faster still, but unfortunately EnterpriseDB has only been able to get 12 developers to do any work at all on partitioning this release cycle, and 3 of those have so far helped only with review and benchmarking. It's a pity we can't do more, but considering how many community projects are 1-person efforts I think it's pretty good. To be clear, I know you're not (or at least I assume you're not) trying to beat me up about this, just raising a concern, and I'm not trying to beat you up either, just let you know that it is definitely on the radar screen but not there yet. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Jesper Pedersen <jesper.pedersen@redhat.com> — 2017-09-14T18:05:02Z
On 09/14/2017 01:52 PM, Robert Haas wrote: > On Thu, Sep 14, 2017 at 1:07 PM, Jesper Pedersen > <jesper.pedersen@redhat.com> wrote: >> Yeah, it would be nice to have a syntax like >> >> ) PARTITION BY HASH (col) WITH (AUTO_CREATE = 64); >> >> But then there also needs to be a way to create the 64 associated indexes >> too for everything to be easy. > > Well, for that, there's this proposal: > > http://postgr.es/m/c8fe4f6b-ff46-aae0-89e3-e936a35f0cfd@postgrespro.ru > > As several people have right pointed out, there's a lot of work to be > done on partitioning it to get it to where we want it to be. Even in > v10, it's got significant benefits, such as much faster bulk-loading, > but I don't hear anybody disputing the notion that a lot more work is > needed. The good news is that a lot of that work is already in > progress; the bad news is that a lot of that work is not done yet. > > But I think that's OK. We can't solve every problem at once, and I > think we're moving things along here at a reasonably brisk pace. That > didn't stop me from complaining bitterly to someone just yesterday > that we aren't moving faster still, but unfortunately EnterpriseDB has > only been able to get 12 developers to do any work at all on > partitioning this release cycle, and 3 of those have so far helped > only with review and benchmarking. It's a pity we can't do more, but > considering how many community projects are 1-person efforts I think > it's pretty good. > > To be clear, I know you're not (or at least I assume you're not) > trying to beat me up about this, just raising a concern, and I'm not > trying to beat you up either, just let you know that it is definitely > on the radar screen but not there yet. > Definitely not a complain about the work being done. I think the scope of Amul's and others work on hash partition support is where it needs to be. Improvements can always follow in future release. My point was that is easy to script the definition of the partitions and their associated indexes, so it is more important to focus on the core functionality with the developer / review resources available. However, it is a little bit difficult to follow the dependencies between different partition patches, so I may not always provide sane feedback, as seen in [1]. [1] https://www.postgresql.org/message-id/579077fd-8f07-aff7-39bc-b92c855cdb70%40redhat.com Best regards, Jesper
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-09-14T19:47:50Z
On Thu, Sep 14, 2017 at 2:05 PM, Jesper Pedersen <jesper.pedersen@redhat.com> wrote: > However, it is a little bit difficult to follow the dependencies between > different partition patches, so I may not always provide sane feedback, as > seen in [1]. > > [1] > https://www.postgresql.org/message-id/579077fd-8f07-aff7-39bc-b92c855cdb70%40redhat.com Yeah, no issues. I knew about the dependency between those patches, but I'm pretty sure there wasn't any terribly explicit discussion about it, even if the issue probably came up parenthetically someplace or other. Oops. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Thom Brown <thom@linux.com> — 2017-09-14T23:00:04Z
On 14 September 2017 at 09:58, amul sul <sulamul@gmail.com> wrote: > On Wed, Sep 13, 2017 at 7:43 PM, Jesper Pedersen > <jesper.pedersen@redhat.com> wrote: >> >> Hi Amul, >> >> On 09/08/2017 08:40 AM, amul sul wrote: >>> >>> Rebased 0002 against this commit & renamed to 0001, PFA. >>> >> >> This patch needs a rebase. >> > > Thanks for your note. > Attached is the patch rebased on the latest master head. > Also added error on > creating > d > efault partition > for the hash partitioned table > , > and updated document & > test script for the same. Sorry, but this needs another rebase as it's broken by commit 77b6b5e9ceca04dbd6f0f6cd3fc881519acc8714. Thom
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-09-15T06:30:09Z
On Fri, Sep 15, 2017 at 4:30 AM, Thom Brown <thom@linux.com> wrote: > On 14 September 2017 at 09:58, amul sul <sulamul@gmail.com> wrote: > > On Wed, Sep 13, 2017 at 7:43 PM, Jesper Pedersen > > <jesper.pedersen@redhat.com> wrote: > >> > >> Hi Amul, > >> > >> On 09/08/2017 08:40 AM, amul sul wrote: > >>> > >>> Rebased 0002 against this commit & renamed to 0001, PFA. > >>> > >> > >> This patch needs a rebase. > >> > > > > Thanks for your note. > > Attached is the patch rebased on the latest master head. > > Also added error on > > creating > > d > > efault partition > > for the hash partitioned table > > , > > and updated document & > > test script for the same. > > Sorry, but this needs another rebase as it's broken by commit > 77b6b5e9ceca04dbd6f0f6cd3fc881519acc8714. > > Attached rebased patch, thanks. Regards, Amul
-
Re: [POC] hash partitioning
Jesper Pedersen <jesper.pedersen@redhat.com> — 2017-09-18T15:25:02Z
On 09/15/2017 02:30 AM, amul sul wrote: > Attached rebased patch, thanks. > While reading through the patch I thought it would be better to keep MODULUS and REMAINDER in caps, if CREATE TABLE was in caps too in order to highlight that these are "keywords" for hash partition. Also updated some of the documentation. V20 patch passes make check-world, and my testing (typical 64 partitions, and various ATTACH/DETACH scenarios). Thanks for working on this ! Best regards, Jesper
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-09-27T07:05:37Z
On Mon, Sep 18, 2017 at 8:55 PM, Jesper Pedersen <jesper.pedersen@redhat.com > wrote: > On 09/15/2017 02:30 AM, amul sul wrote: > >> Attached rebased patch, thanks. >> >> > While reading through the patch I thought it would be better to keep > MODULUS and REMAINDER in caps, if CREATE TABLE was in caps too in order to > highlight that these are "keywords" for hash partition. > > Also updated some of the documentation. > > Thanks a lot for the patch, included in the attached version. > V20 patch passes make check-world, and my testing (typical 64 partitions, > and various ATTACH/DETACH scenarios). > Nice, thanks again. Regards, Amul
-
Re: [POC] hash partitioning
Jesper Pedersen <jesper.pedersen@redhat.com> — 2017-09-27T13:41:26Z
On 09/27/2017 03:05 AM, amul sul wrote: >>> Attached rebased patch, thanks. >>> >>> >> While reading through the patch I thought it would be better to keep >> MODULUS and REMAINDER in caps, if CREATE TABLE was in caps too in order to >> highlight that these are "keywords" for hash partition. >> >> Also updated some of the documentation. >> >> > Thanks a lot for the patch, included in the attached version. > Thank you. Based on [1] I have moved the patch to "Ready for Committer". [1] https://www.postgresql.org/message-id/CA%2BTgmoYsw3pusDen4_A44c7od%2BbEAST0eYo%2BjODtyofR0W2soQ%40mail.gmail.com Best regards, Jesper
-
Re: [POC] hash partitioning
Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-09-28T05:54:59Z
On 2017/09/27 22:41, Jesper Pedersen wrote: > On 09/27/2017 03:05 AM, amul sul wrote: >>>> Attached rebased patch, thanks. >>>> >>>> >>> While reading through the patch I thought it would be better to keep >>> MODULUS and REMAINDER in caps, if CREATE TABLE was in caps too in order to >>> highlight that these are "keywords" for hash partition. >>> >>> Also updated some of the documentation. >>> >>> >> Thanks a lot for the patch, included in the attached version. >> > > Thank you. > > Based on [1] I have moved the patch to "Ready for Committer". Thanks a lot Amul for working on this. Like Jesper said, the patch looks pretty good overall. I was looking at the latest version with intent to study certain things about hash partitioning the way patch implements it, during which I noticed some things. + The modulus must be a positive integer, and the remainder must a must be a + suppose you have a hash-partitioned table with 8 children, each of which + has modulus 8, but find it necessary to increase the number of partitions + to 16. Might it be a good idea to say 8 "partitions" instead of "children" in the first sentence? + each modulus-8 partition until none remain. While this may still involve + a large amount of data movement at each step, it is still better than + having to create a whole new table and move all the data at once. + </para> + I read the paragraph that ends with the above text and started wondering if the example to redistribute data in hash partitions by detaching and attaching with new modulus/remainder could be illustrated with an example? Maybe in the examples section of the ALTER TABLE page? + Since hash operator class provide only equality, not ordering, collation Either "Since hash operator classes provide" or "Since hash operator class provides" Other than the above points, patch looks good. By the way, I noticed a couple of things about hash partition constraints: 1. In get_qual_for_hash(), using get_fn_expr_rettype(&key->partsupfunc[i]), which returns InvalidOid for the lack of fn_expr being set to non-NULL value, causes funcrettype of the FuncExpr being generated for hashing partition key columns to be set to InvalidOid, which I think is wrong. That is, the following if condition in get_fn_expr_rettype() is always satisfied: if (!flinfo || !flinfo->fn_expr) return InvalidOid; I think we could use get_func_rettype(&key->partsupfunc[i].fn_oid) instead. Attached patch hash-v21-set-funcexpr-funcrettype-correctly.patch, which applies on top v21 of your patch. 2. It seems that the reason constraint exclusion doesn't work with hash partitions as implemented by the patch is that predtest.c: operator_predicate_proof() returns false even without looking into the hash partition constraint, which is of the following form: satisfies_hash_partition(<mod>, <rem>, <key1-exthash>,..) beccause the above constraint expression doesn't translate into a a binary opclause (an OpExpr), which operator_predicate_proof() knows how to work with. So, false is returned at the beginning of that function by the following code: if (!is_opclause(predicate)) return false; For example, create table p (a int) partition by hash (a); create table p0 partition of p for values with (modulus 4, remainder 0); create table p1 partition of p for values with (modulus 4, remainder 1); \d+ p0 <...> Partition constraint: satisfies_hash_partition(4, 0, hashint4extended(a, '8816678312871386367'::bigint)) -- both p0 and p1 scanned explain select * from p where satisfies_hash_partition(4, 0, hashint4extended(a, '8816678312871386367'::bigint)); QUERY PLAN ---------------------------------------------------------------------------------------------------- Append (cost=0.00..96.50 rows=1700 width=4) -> Seq Scan on p0 (cost=0.00..48.25 rows=850 width=4) Filter: satisfies_hash_partition(4, 0, hashint4extended(a, '8816678312871386367'::bigint)) -> Seq Scan on p1 (cost=0.00..48.25 rows=850 width=4) Filter: satisfies_hash_partition(4, 0, hashint4extended(a, '8816678312871386367'::bigint)) (5 rows) -- both p0 and p1 scanned explain select * from p where satisfies_hash_partition(4, 1, hashint4extended(a, '8816678312871386367'::bigint)); QUERY PLAN ---------------------------------------------------------------------------------------------------- Append (cost=0.00..96.50 rows=1700 width=4) -> Seq Scan on p0 (cost=0.00..48.25 rows=850 width=4) Filter: satisfies_hash_partition(4, 1, hashint4extended(a, '8816678312871386367'::bigint)) -> Seq Scan on p1 (cost=0.00..48.25 rows=850 width=4) Filter: satisfies_hash_partition(4, 1, hashint4extended(a, '8816678312871386367'::bigint)) (5 rows) I looked into how satisfies_hash_partition() works and came up with an idea that I think will make constraint exclusion work. What if we emitted the hash partition constraint in the following form instead: hash_partition_mod(hash_partition_hash(key1-exthash, key2-exthash), <mod>) = <rem> With that form, constraint exclusion seems to work as illustrated below: \d+ p0 <...> Partition constraint: (hash_partition_modulus(hash_partition_hash(hashint4extended(a, '8816678312871386367'::bigint)), 4) = 0) -- note only p0 is scanned explain select * from p where hash_partition_modulus(hash_partition_hash(hashint4extended(a, '8816678312871386367'::bigint)), 4) = 0; QUERY PLAN -------------------------------------------------------------------------------------------------------------------------- Append (cost=0.00..61.00 rows=13 width=4) -> Seq Scan on p0 (cost=0.00..61.00 rows=13 width=4) Filter: (hash_partition_modulus(hash_partition_hash(hashint4extended(a, '8816678312871386367'::bigint)), 4) = 0) (3 rows) -- note only p1 is scanned explain select * from p where hash_partition_modulus(hash_partition_hash(hashint4extended(a, '8816678312871386367'::bigint)), 4) = 1; QUERY PLAN -------------------------------------------------------------------------------------------------------------------------- Append (cost=0.00..61.00 rows=13 width=4) -> Seq Scan on p1 (cost=0.00..61.00 rows=13 width=4) Filter: (hash_partition_modulus(hash_partition_hash(hashint4extended(a, '8816678312871386367'::bigint)), 4) = 1) (3 rows) I tried to implement that in the attached hash-v21-hash-part-constraint.patch, which applies on top v21 of your patch (actually on top of hash-v21-set-funcexpr-funcrettype-correctly.patch, which I think should be applied anyway as it fixes a bug of the original patch). What do you think? Eventually, the new partition-pruning method [1] will make using constraint exclusion obsolete, but it might be a good idea to have it working if we can. Thanks, Amit [1] https://commitfest.postgresql.org/14/1272/ -
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-09-28T09:56:48Z
On Thu, Sep 28, 2017 at 11:24 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > On 2017/09/27 22:41, Jesper Pedersen wrote: >> On 09/27/2017 03:05 AM, amul sul wrote: >>>>> Attached rebased patch, thanks. >>>>> >>>>> >>>> While reading through the patch I thought it would be better to keep >>>> MODULUS and REMAINDER in caps, if CREATE TABLE was in caps too in order to >>>> highlight that these are "keywords" for hash partition. >>>> >>>> Also updated some of the documentation. >>>> >>>> >>> Thanks a lot for the patch, included in the attached version. >>> >> >> Thank you. >> >> Based on [1] I have moved the patch to "Ready for Committer". > > Thanks a lot Amul for working on this. Like Jesper said, the patch looks > pretty good overall. I was looking at the latest version with intent to > study certain things about hash partitioning the way patch implements it, > during which I noticed some things. > Thanks Amit for looking at the patch. > + The modulus must be a positive integer, and the remainder must a > > must be a > Fixed in the attached version. > + suppose you have a hash-partitioned table with 8 children, each of > which > + has modulus 8, but find it necessary to increase the number of > partitions > + to 16. > Fixed in the attached version. > Might it be a good idea to say 8 "partitions" instead of "children" in the > first sentence? > > + each modulus-8 partition until none remain. While this may still > involve > + a large amount of data movement at each step, it is still better than > + having to create a whole new table and move all the data at once. > + </para> > + > Fixed in the attached version. > I read the paragraph that ends with the above text and started wondering > if the example to redistribute data in hash partitions by detaching and > attaching with new modulus/remainder could be illustrated with an example? > Maybe in the examples section of the ALTER TABLE page? > I think hint in the documentation is more than enough. There is N number of ways of data redistribution, the document is not meant to explain all of those. > + Since hash operator class provide only equality, not ordering, > collation > > Either "Since hash operator classes provide" or "Since hash operator class > provides" > Fixed in the attached version. > Other than the above points, patch looks good. > > > By the way, I noticed a couple of things about hash partition constraints: > > 1. In get_qual_for_hash(), using > get_fn_expr_rettype(&key->partsupfunc[i]), which returns InvalidOid for > the lack of fn_expr being set to non-NULL value, causes funcrettype of the > FuncExpr being generated for hashing partition key columns to be set to > InvalidOid, which I think is wrong. That is, the following if condition > in get_fn_expr_rettype() is always satisfied: > > if (!flinfo || !flinfo->fn_expr) > return InvalidOid; > > I think we could use get_func_rettype(&key->partsupfunc[i].fn_oid) > instead. Attached patch > hash-v21-set-funcexpr-funcrettype-correctly.patch, which applies on top > v21 of your patch. > Thanks for the patch, included in the attached version. > 2. It seems that the reason constraint exclusion doesn't work with hash > partitions as implemented by the patch is that predtest.c: > operator_predicate_proof() returns false even without looking into the > hash partition constraint, which is of the following form: > > satisfies_hash_partition(<mod>, <rem>, <key1-exthash>,..) > > beccause the above constraint expression doesn't translate into a a binary > opclause (an OpExpr), which operator_predicate_proof() knows how to work > with. So, false is returned at the beginning of that function by the > following code: > > if (!is_opclause(predicate)) > return false; > > For example, > > create table p (a int) partition by hash (a); > create table p0 partition of p for values with (modulus 4, remainder 0); > create table p1 partition of p for values with (modulus 4, remainder 1); > \d+ p0 > <...> > Partition constraint: satisfies_hash_partition(4, 0, hashint4extended(a, > '8816678312871386367'::bigint)) > > -- both p0 and p1 scanned > explain select * from p where satisfies_hash_partition(4, 0, > hashint4extended(a, '8816678312871386367'::bigint)); > QUERY PLAN > > ---------------------------------------------------------------------------------------------------- > Append (cost=0.00..96.50 rows=1700 width=4) > -> Seq Scan on p0 (cost=0.00..48.25 rows=850 width=4) > Filter: satisfies_hash_partition(4, 0, hashint4extended(a, > '8816678312871386367'::bigint)) > -> Seq Scan on p1 (cost=0.00..48.25 rows=850 width=4) > Filter: satisfies_hash_partition(4, 0, hashint4extended(a, > '8816678312871386367'::bigint)) > (5 rows) > > -- both p0 and p1 scanned > explain select * from p where satisfies_hash_partition(4, 1, > hashint4extended(a, '8816678312871386367'::bigint)); > QUERY PLAN > > ---------------------------------------------------------------------------------------------------- > Append (cost=0.00..96.50 rows=1700 width=4) > -> Seq Scan on p0 (cost=0.00..48.25 rows=850 width=4) > Filter: satisfies_hash_partition(4, 1, hashint4extended(a, > '8816678312871386367'::bigint)) > -> Seq Scan on p1 (cost=0.00..48.25 rows=850 width=4) > Filter: satisfies_hash_partition(4, 1, hashint4extended(a, > '8816678312871386367'::bigint)) > (5 rows) > > > I looked into how satisfies_hash_partition() works and came up with an > idea that I think will make constraint exclusion work. What if we emitted > the hash partition constraint in the following form instead: > > hash_partition_mod(hash_partition_hash(key1-exthash, key2-exthash), > <mod>) = <rem> > > With that form, constraint exclusion seems to work as illustrated below: > > \d+ p0 > <...> > Partition constraint: > (hash_partition_modulus(hash_partition_hash(hashint4extended(a, > '8816678312871386367'::bigint)), 4) = 0) > > -- note only p0 is scanned > explain select * from p where > hash_partition_modulus(hash_partition_hash(hashint4extended(a, > '8816678312871386367'::bigint)), 4) = 0; > QUERY PLAN > > -------------------------------------------------------------------------------------------------------------------------- > Append (cost=0.00..61.00 rows=13 width=4) > -> Seq Scan on p0 (cost=0.00..61.00 rows=13 width=4) > Filter: > (hash_partition_modulus(hash_partition_hash(hashint4extended(a, > '8816678312871386367'::bigint)), 4) = 0) > (3 rows) > > -- note only p1 is scanned > explain select * from p where > hash_partition_modulus(hash_partition_hash(hashint4extended(a, > '8816678312871386367'::bigint)), 4) = 1; > QUERY PLAN > > -------------------------------------------------------------------------------------------------------------------------- > Append (cost=0.00..61.00 rows=13 width=4) > -> Seq Scan on p1 (cost=0.00..61.00 rows=13 width=4) > Filter: > (hash_partition_modulus(hash_partition_hash(hashint4extended(a, > '8816678312871386367'::bigint)), 4) = 1) > (3 rows) > > I tried to implement that in the attached > hash-v21-hash-part-constraint.patch, which applies on top v21 of your > patch (actually on top of > hash-v21-set-funcexpr-funcrettype-correctly.patch, which I think should be > applied anyway as it fixes a bug of the original patch). > > What do you think? Eventually, the new partition-pruning method [1] will > make using constraint exclusion obsolete, but it might be a good idea to > have it working if we can. > It does not really do the partition pruning via constraint exclusion and I don't think anyone is going to use the remainder in the where condition to fetch data and hash partitioning is not meant for that. But I am sure that we could solve this problem using your and Beena's work toward faster partition pruning[1] and Runtime Partition Pruning[2]. Will think on this changes if it is required for the pruning feature. Regards, Amul 1] https://postgr.es/m/098b9c71-1915-1a2a-8d52-1a7a50ce79e8@lab.ntt.co.jp 2] https://postgr.es/m/CAOG9ApE16ac-_VVZVvv0gePSgkg_BwYEV1NBqZFqDR2bBE0X0A@mail.gmail.com
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-09-29T16:53:39Z
On Thu, Sep 28, 2017 at 1:54 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > I looked into how satisfies_hash_partition() works and came up with an > idea that I think will make constraint exclusion work. What if we emitted > the hash partition constraint in the following form instead: > > hash_partition_mod(hash_partition_hash(key1-exthash, key2-exthash), > <mod>) = <rem> > > With that form, constraint exclusion seems to work as illustrated below: > > \d+ p0 > <...> > Partition constraint: > (hash_partition_modulus(hash_partition_hash(hashint4extended(a, > '8816678312871386367'::bigint)), 4) = 0) > > -- note only p0 is scanned > explain select * from p where > hash_partition_modulus(hash_partition_hash(hashint4extended(a, > '8816678312871386367'::bigint)), 4) = 0; What we actually want constraint exclusion to cover is SELECT * FROM p WHERE a = 525600; As Amul says, nobody's going to enter a query in the form you have it here. Life is too short to take time to put queries into bizarre forms. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Jesper Pedersen <jesper.pedersen@redhat.com> — 2017-10-06T12:05:51Z
Hi Amul, On 09/28/2017 05:56 AM, amul sul wrote: > It does not really do the partition pruning via constraint exclusion and I don't > think anyone is going to use the remainder in the where condition to fetch > data and hash partitioning is not meant for that. > > But I am sure that we could solve this problem using your and Beena's work > toward faster partition pruning[1] and Runtime Partition Pruning[2]. > > Will think on this changes if it is required for the pruning feature. > Could you rebase on latest master ? Best regards, Jesper
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-10-07T11:52:42Z
On Fri, Oct 6, 2017 at 5:35 PM, Jesper Pedersen <jesper.pedersen@redhat.com> wrote: > Hi Amul, > > Could you rebase on latest master ? > Sure will post that soon, but before that, I need to test hash partitioning with recent partition-wise join commit (f49842d1ee), thanks. Regards, Amul
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-10-09T11:14:29Z
On Sat, Oct 7, 2017 at 5:22 PM, amul sul <sulamul@gmail.com> wrote: > On Fri, Oct 6, 2017 at 5:35 PM, Jesper Pedersen > <jesper.pedersen@redhat.com> wrote: >> Hi Amul, >> >> Could you rebase on latest master ? >> > > Sure will post that soon, but before that, I need to test hash partitioning > with recent partition-wise join commit (f49842d1ee), thanks. > Updated patch attached. 0001 is the rebased of the previous patch, no new change. 0002 few changes in partition-wise join code to support hash-partitioned table as well & regression tests. Thanks & Regards, Amul
-
Re: [POC] hash partitioning
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-10-09T12:21:21Z
On Mon, Oct 9, 2017 at 4:44 PM, amul sul <sulamul@gmail.com> wrote: > 0002 few changes in partition-wise join code to support > hash-partitioned table as well & regression tests. + switch (key->strategy) + { + case PARTITION_STRATEGY_HASH: + /* + * Indexes array is same as the greatest modulus. + * See partition_bounds_equal() for more explanation. + */ + num_indexes = DatumGetInt32(src->datums[ndatums - 1][0]); + break; This logic is duplicated at multiple places. I think it's time we consolidate these changes in a function/macro and call it from the places where we have to calculate number of indexes based on the information in partition descriptor. Refactoring existing code might be a separate patch and then add hash partitioning case in hash partitioning patch. + int dim = hash_part? 2 : partnatts; Call the variable as natts_per_datum or just natts? + hash_part? true : key->parttypbyval[j], + key->parttyplen[j]); parttyplen is the length of partition key attribute, whereas what you want here is the length of type of modulus and remainder. Is that correct? Probably we need some special handling wherever parttyplen and parttypbyval is used e.g. in call to partition_bounds_equal() from build_joinrel_partition_info(). -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-10-10T10:02:55Z
On Mon, Oct 9, 2017 at 5:51 PM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > On Mon, Oct 9, 2017 at 4:44 PM, amul sul <sulamul@gmail.com> wrote: > Thanks Ashutosh for your review, please find my comment inline. > >> 0002 few changes in partition-wise join code to support >> hash-partitioned table as well & regression tests. > > + switch (key->strategy) > + { > + case PARTITION_STRATEGY_HASH: > + /* > + * Indexes array is same as the greatest modulus. > + * See partition_bounds_equal() for more explanation. > + */ > + num_indexes = DatumGetInt32(src->datums[ndatums - 1][0]); > + break; > This logic is duplicated at multiple places. I think it's time we consolidate > these changes in a function/macro and call it from the places where we have to > calculate number of indexes based on the information in partition descriptor. > Refactoring existing code might be a separate patch and then add hash > partitioning case in hash partitioning patch. > Make sense, added get_partition_bound_num_indexes() to get number of index elements in 0001 & get_greatest_modulus() as name suggested to get the greatest modulus of the hash partition bound in 0002. > + int dim = hash_part? 2 : partnatts; > Call the variable as natts_per_datum or just natts? > natts represents the number of attributes, but for the hash partition bound we are not dealing with the attribute so that I have used short-form of dimension, thoughts? > + hash_part? true : key->parttypbyval[j], > + key->parttyplen[j]); > parttyplen is the length of partition key attribute, whereas what you want here > is the length of type of modulus and remainder. Is that correct? Probably we > need some special handling wherever parttyplen and parttypbyval is used e.g. in > call to partition_bounds_equal() from build_joinrel_partition_info(). > Unless I am missing something, I don't think we should worry about parttyplen because in the datumCopy() when the datatype is pass-by-value then typelen is ignored. Regards, Amul -
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-10-10T10:10:22Z
On Tue, Oct 10, 2017 at 3:32 PM, amul sul <sulamul@gmail.com> wrote: > On Mon, Oct 9, 2017 at 5:51 PM, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: >> On Mon, Oct 9, 2017 at 4:44 PM, amul sul <sulamul@gmail.com> wrote: >> > > Thanks Ashutosh for your review, please find my comment inline. > >> >>> 0002 few changes in partition-wise join code to support >>> hash-partitioned table as well & regression tests. >> >> + switch (key->strategy) >> + { >> + case PARTITION_STRATEGY_HASH: >> + /* >> + * Indexes array is same as the greatest modulus. >> + * See partition_bounds_equal() for more explanation. >> + */ >> + num_indexes = DatumGetInt32(src->datums[ndatums - 1][0]); >> + break; >> This logic is duplicated at multiple places. I think it's time we consolidate >> these changes in a function/macro and call it from the places where we have to >> calculate number of indexes based on the information in partition descriptor. >> Refactoring existing code might be a separate patch and then add hash >> partitioning case in hash partitioning patch. >> > > Make sense, added get_partition_bound_num_indexes() to get number of index > elements in 0001 & get_greatest_modulus() as name suggested to get the greatest > modulus of the hash partition bound in 0002. > >> + int dim = hash_part? 2 : partnatts; >> Call the variable as natts_per_datum or just natts? >> > > natts represents the number of attributes, but for the hash partition bound we > are not dealing with the attribute so that I have used short-form of dimension, > thoughts? Okay, I think the dimension(dim) is also unfit here. Any suggestions? > >> + hash_part? true : key->parttypbyval[j], >> + key->parttyplen[j]); >> parttyplen is the length of partition key attribute, whereas what you want here >> is the length of type of modulus and remainder. Is that correct? Probably we >> need some special handling wherever parttyplen and parttypbyval is used e.g. in >> call to partition_bounds_equal() from build_joinrel_partition_info(). >> > > Unless I am missing something, I don't think we should worry about parttyplen > because in the datumCopy() when the datatype is pass-by-value then typelen > is ignored. > > Regards, > Amul -
Re: [POC] hash partitioning
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-10-10T10:12:04Z
On Tue, Oct 10, 2017 at 3:32 PM, amul sul <sulamul@gmail.com> wrote: >> + hash_part? true : key->parttypbyval[j], >> + key->parttyplen[j]); >> parttyplen is the length of partition key attribute, whereas what you want here >> is the length of type of modulus and remainder. Is that correct? Probably we >> need some special handling wherever parttyplen and parttypbyval is used e.g. in >> call to partition_bounds_equal() from build_joinrel_partition_info(). >> > > Unless I am missing something, I don't think we should worry about parttyplen > because in the datumCopy() when the datatype is pass-by-value then typelen > is ignored. That's true, but it's ugly, passing typbyvalue of one type and len of other. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
-
Re: [POC] hash partitioning
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-10-10T10:13:11Z
On Tue, Oct 10, 2017 at 3:40 PM, amul sul <sulamul@gmail.com> wrote: >> >> natts represents the number of attributes, but for the hash partition bound we >> are not dealing with the attribute so that I have used short-form of dimension, >> thoughts? > > Okay, I think the dimension(dim) is also unfit here. Any suggestions? > I think natts is ok, since we are dealing with the number of attributes in the pack of datums; esp. when ndatums is already taken. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-10-10T11:07:40Z
On Tue, Oct 10, 2017 at 3:42 PM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > On Tue, Oct 10, 2017 at 3:32 PM, amul sul <sulamul@gmail.com> wrote: > >>> + hash_part? true : key->parttypbyval[j], >>> + key->parttyplen[j]); >>> parttyplen is the length of partition key attribute, whereas what you want here >>> is the length of type of modulus and remainder. Is that correct? Probably we >>> need some special handling wherever parttyplen and parttypbyval is used e.g. in >>> call to partition_bounds_equal() from build_joinrel_partition_info(). >>> >> >> Unless I am missing something, I don't think we should worry about parttyplen >> because in the datumCopy() when the datatype is pass-by-value then typelen >> is ignored. > > That's true, but it's ugly, passing typbyvalue of one type and len of other. > How about the attached patch(0003)? Also, the dim variable is renamed to natts. Regards, Amul
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-10-12T01:01:43Z
On Tue, Oct 10, 2017 at 7:07 AM, amul sul <sulamul@gmail.com> wrote: > How about the attached patch(0003)? > Also, the dim variable is renamed to natts. I'm not sure I believe this comment: + /* + * We arrange the partitions in the ascending order of their modulus + * and remainders. Also every modulus is factor of next larger + * modulus. This means that the index of a given partition is same as + * the remainder of that partition. Also entries at (remainder + N * + * modulus) positions in indexes array are all same for (modulus, + * remainder) specification for any partition. Thus datums array from + * both the given bounds are same, if and only if their indexes array + * will be same. So, it suffices to compare indexes array. + */ I am particularly not sure that I believe that the index of a partition must be the same as the remainder. It doesn't seem like that would be true when there is more than one modulus or when some partitions are missing. + if (offset < 0) + { + next_modulus = DatumGetInt32(datums[0][0]); + valid_modulus = (next_modulus % spec->modulus) == 0; + } + else + { + prev_modulus = DatumGetInt32(datums[offset][0]); + valid_modulus = (spec->modulus % prev_modulus) == 0; + + if (valid_modulus && (offset + 1) < ndatums) + { + next_modulus = DatumGetInt32(datums[offset + 1][0]); + valid_modulus = (next_modulus % spec->modulus) == 0; + } + } I don't think this is quite right. It checks the new modulus against prev_modulus whenever prev_modulus is defined, which is correct, but it doesn't check the new modulus against the next_modulus except when offset < 0. But actually that check needs to be performed, I think, whenever the new modulus is less than the greatest modulus seen so far. + * For a partitioned table defined as: + * CREATE TABLE simple_hash (a int, b char(10)) PARTITION BY HASH (a, b); + * + * CREATE TABLE p_p1 PARTITION OF simple_hash + * FOR VALUES WITH (MODULUS 2, REMAINDER 1); + * CREATE TABLE p_p2 PARTITION OF simple_hash + * FOR VALUES WITH (MODULUS 4, REMAINDER 2); + * CREATE TABLE p_p3 PARTITION OF simple_hash + * FOR VALUES WITH (MODULUS 8, REMAINDER 0); + * CREATE TABLE p_p4 PARTITION OF simple_hash + * FOR VALUES WITH (MODULUS 8, REMAINDER 4); + * + * This function will return one of the following in the form of an + * expression: + * + * for p_p1: satisfies_hash_partition(2, 1, hash_fn_1_extended(a, HASH_SEED), + * hash_fn_2_extended(b, HASH_SEED)) + * for p_p2: satisfies_hash_partition(4, 2, hash_fn_1_extended(a, HASH_SEED), + * hash_fn_2_extended(b, HASH_SEED)) + * for p_p3: satisfies_hash_partition(8, 0, hash_fn_1_extended(a, HASH_SEED), + * hash_fn_2_extended(b, HASH_SEED)) + * for p_p4: satisfies_hash_partition(8, 4, hash_fn_1_extended(a, HASH_SEED), + * hash_fn_2_extended(b, HASH_SEED)) I think instead of this lengthy example you should try to explain the general rule. Maybe something like: the partition constraint for a hash partition is always a call to the built-in function satisfies_hash_partition(). The first two arguments are the modulus and remainder for the partition; the remaining arguments are the hash values computed for each column of the partition key using the extended hash function from the appropriate opclass. +static uint64 +mix_hash_value(int nkeys, Datum *hash_array, bool *isnull) It would be nice to use the hash_combine() facility Andres recently added for this rather than having a way to do it that is specific to hash partitioning, but that function only works for 32-bit hash values. Maybe we can persuade Andres to add a hash_combine64... + * a hash operator class Missing period at end. + if (strategy == PARTITION_STRATEGY_HASH) + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("default hash partition is not supported"))); Maybe errmsg("a hash-partitioned table may not have a default partition")? +/* Seed for the extended hash function */ +#define HASH_SEED UINT64CONST(0x7A5B22367996DCFF) I suggest HASH_PARTITION_SEED -- this is too generic. Have you checked how well the tests you've added cover the code you've added? What code is not covered by the tests, and is there any way to cover it? Thanks, -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -
Re: [POC] hash partitioning
Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-10-12T01:31:39Z
On 2017/09/30 1:53, Robert Haas wrote: > On Thu, Sep 28, 2017 at 1:54 AM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> I looked into how satisfies_hash_partition() works and came up with an >> idea that I think will make constraint exclusion work. What if we emitted >> the hash partition constraint in the following form instead: >> >> hash_partition_mod(hash_partition_hash(key1-exthash, key2-exthash), >> <mod>) = <rem> >> >> With that form, constraint exclusion seems to work as illustrated below: >> >> \d+ p0 >> <...> >> Partition constraint: >> (hash_partition_modulus(hash_partition_hash(hashint4extended(a, >> '8816678312871386367'::bigint)), 4) = 0) >> >> -- note only p0 is scanned >> explain select * from p where >> hash_partition_modulus(hash_partition_hash(hashint4extended(a, >> '8816678312871386367'::bigint)), 4) = 0; > > What we actually want constraint exclusion to cover is SELECT * FROM p > WHERE a = 525600; I agree. > As Amul says, nobody's going to enter a query in the form you have it > here. Life is too short to take time to put queries into bizarre > forms. Here too. I was falsely thinking that satisfies_hash_partition() is intended to be used for more than just enforcing the partition constraint when data is directly inserted into a hash partition, or more precisely to be used in the CHECK constraint of the table that is to be attached as a hash partition. Now, we ask users to add such a constraint to avoid the constraint validation scan, because the system knows how to infer from the constraint that the partition constraint is satisfied. I observed however that, unlike range and list partitioning, the hash partition's constraint could only ever be implied because of structural equality (equal()'ness) of the existing constraint expression and the partition constraint expression. For example, a more restrictive range or list qual implies the partition constraint, but it requires performing btree operator based proof. The proof is impossible with the chosen structure of hash partitioning constraint, but it seems that that's OK. That is, it's OK to ask users to add the exact constraint (matching modulus and reminder values in the call to satisfies_hash_partition() specified in the CHECK constraint) to avoid the validation scan. Thanks, Amit
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-10-12T13:08:37Z
On Thu, Oct 12, 2017 at 6:31 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Oct 10, 2017 at 7:07 AM, amul sul <sulamul@gmail.com> wrote: >> How about the attached patch(0003)? >> Also, the dim variable is renamed to natts. > > I'm not sure I believe this comment: > > + /* > + * We arrange the partitions in the ascending order of their modulus > + * and remainders. Also every modulus is factor of next larger > + * modulus. This means that the index of a given partition is same as > + * the remainder of that partition. Also entries at (remainder + N * > + * modulus) positions in indexes array are all same for (modulus, > + * remainder) specification for any partition. Thus datums array from > + * both the given bounds are same, if and only if their indexes array > + * will be same. So, it suffices to compare indexes array. > + */ > > I am particularly not sure that I believe that the index of a > partition must be the same as the remainder. It doesn't seem like > that would be true when there is more than one modulus or when some > partitions are missing. > Looks like an explanation by the comment is not good enough, will think on this. Here are the links for the previous discussion: 1] https://postgr.es/m/CAFjFpRfHqSGBjNgJV2p%2BC4Yr5Qxvwygdsg4G_VQ6q9NTB-i3MA%40mail.gmail.com 2] https://postgr.es/m/CAFjFpRdeESKFkVGgmOdYvmD3d56-58c5VCBK0zDRjHrkq_VcNg%40mail.gmail.com > + if (offset < 0) > + { > + next_modulus = DatumGetInt32(datums[0][0]); > + valid_modulus = (next_modulus % spec->modulus) == 0; > + } > + else > + { > + prev_modulus = DatumGetInt32(datums[offset][0]); > + valid_modulus = (spec->modulus % prev_modulus) == 0; > + > + if (valid_modulus && (offset + 1) < ndatums) > + { > + next_modulus = > DatumGetInt32(datums[offset + 1][0]); > + valid_modulus = (next_modulus % > spec->modulus) == 0; > + } > + } > > I don't think this is quite right. It checks the new modulus against > prev_modulus whenever prev_modulus is defined, which is correct, but > it doesn't check the new modulus against the next_modulus except when > offset < 0. But actually that check needs to be performed, I think, > whenever the new modulus is less than the greatest modulus seen so > far. > It does. See the "if (valid_modulus && (offset + 1) < ndatums)" block in the else part of the snippet that you are referring. For e.g new modulus 25 & 150 is not accepted for the hash partitioned bound with modulus 10,50,200. Will cover this test as well. > + * For a partitioned table defined as: > + * CREATE TABLE simple_hash (a int, b char(10)) PARTITION BY HASH (a, b); > + * > + * CREATE TABLE p_p1 PARTITION OF simple_hash > + * FOR VALUES WITH (MODULUS 2, REMAINDER 1); > + * CREATE TABLE p_p2 PARTITION OF simple_hash > + * FOR VALUES WITH (MODULUS 4, REMAINDER 2); > + * CREATE TABLE p_p3 PARTITION OF simple_hash > + * FOR VALUES WITH (MODULUS 8, REMAINDER 0); > + * CREATE TABLE p_p4 PARTITION OF simple_hash > + * FOR VALUES WITH (MODULUS 8, REMAINDER 4); > + * > + * This function will return one of the following in the form of an > + * expression: > + * > + * for p_p1: satisfies_hash_partition(2, 1, hash_fn_1_extended(a, HASH_SEED), > + * hash_fn_2_extended(b, > HASH_SEED)) > + * for p_p2: satisfies_hash_partition(4, 2, hash_fn_1_extended(a, HASH_SEED), > + * hash_fn_2_extended(b, > HASH_SEED)) > + * for p_p3: satisfies_hash_partition(8, 0, hash_fn_1_extended(a, HASH_SEED), > + * hash_fn_2_extended(b, > HASH_SEED)) > + * for p_p4: satisfies_hash_partition(8, 4, hash_fn_1_extended(a, HASH_SEED), > + * hash_fn_2_extended(b, > HASH_SEED)) > > I think instead of this lengthy example you should try to explain the > general rule. Maybe something like: the partition constraint for a > hash partition is always a call to the built-in function > satisfies_hash_partition(). The first two arguments are the modulus > and remainder for the partition; the remaining arguments are the hash > values computed for each column of the partition key using the > extended hash function from the appropriate opclass. > Okay will add this. > +static uint64 > +mix_hash_value(int nkeys, Datum *hash_array, bool *isnull) > How about combining high 32 bits and the low 32 bits separately as shown below? static inline uint64 hash_combine64(uint64 a, uint64 b) { return (((uint64) hash_combine((uint32) a >> 32, (uint32) b >> 32) << 32) | hash_combine((unit32) a, (unit32) b)); } > It would be nice to use the hash_combine() facility Andres recently > added for this rather than having a way to do it that is specific to > hash partitioning, but that function only works for 32-bit hash > values. Maybe we can persuade Andres to add a hash_combine64... > > + * a hash operator class > > Missing period at end. > Okay will fix this. > + if (strategy == PARTITION_STRATEGY_HASH) > + ereport(ERROR, > + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), > + errmsg("default hash partition is not supported"))); > > Maybe errmsg("a hash-partitioned table may not have a default partition")? > Okay will add this. > +/* Seed for the extended hash function */ > +#define HASH_SEED UINT64CONST(0x7A5B22367996DCFF) > > I suggest HASH_PARTITION_SEED -- this is too generic. > Okay will add this. > Have you checked how well the tests you've added cover the code you've > added? What code is not covered by the tests, and is there any way to > cover it? > Will try to get gcov report for this patch. Thanks for your review. Regards, Amul -
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-10-12T14:05:26Z
On Thu, Oct 12, 2017 at 9:08 AM, amul sul <sulamul@gmail.com> wrote: > How about combining high 32 bits and the low 32 bits separately as shown below? > > static inline uint64 > hash_combine64(uint64 a, uint64 b) > { > return (((uint64) hash_combine((uint32) a >> 32, (uint32) b >> 32) << 32) > | hash_combine((unit32) a, (unit32) b)); > } I doubt that's the best approach, but I don't have something specific to recommend. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -
Re: [POC] hash partitioning
Andres Freund <andres@anarazel.de> — 2017-10-12T19:43:53Z
On 2017-10-12 10:05:26 -0400, Robert Haas wrote: > On Thu, Oct 12, 2017 at 9:08 AM, amul sul <sulamul@gmail.com> wrote: > > How about combining high 32 bits and the low 32 bits separately as shown below? > > > > static inline uint64 > > hash_combine64(uint64 a, uint64 b) > > { > > return (((uint64) hash_combine((uint32) a >> 32, (uint32) b >> 32) << 32) > > | hash_combine((unit32) a, (unit32) b)); > > } > > I doubt that's the best approach, but I don't have something specific > to recommend. Yea, that doesn't look great. There's basically no intermixing between low and high 32 bits. going on. We probably should just expand the concept of the 32 bit function: static inline uint32 hash_combine32(uint32 a, uint32 b) { /* 0x9e3779b9 is the golden ratio reciprocal */ a ^= b + 0x9e3779b9 + (a << 6) + (a >> 2); return a; } to something roughly like: static inline uint64 hash_combine64(uint64 a, uint64 b) { /* 0x49A0F4DD15E5A8E3 is 64bit random data */ a ^= b + 0x49A0F4DD15E5A8E3 + (a << 54) + (a >> 7); return a; } In contrast to the 32 bit version's fancy use of the golden ratio reciprocal as a constant I went brute force, and just used 64bit of /dev/random. From my understanding the important property is that bits are independent from each other, nothing else. The shift widths are fairly random, but they should bring in enough bit perturbation when mixing in only 32bit of hash value (i.e 0x00000000xxxxxxxx). Are we going to rely on the the combine function to stay the same forever after? Greetings, Andres Freund -
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-10-12T20:06:11Z
On Thu, Oct 12, 2017 at 3:43 PM, Andres Freund <andres@anarazel.de> wrote: > Are we going to rely on the the combine function to stay the same > forever after? If we change them, it will be a pg_upgrade compatibility break for anyone using hash-partitioned tables with more than one partitioning column. Dump and reload will also break unless --load-via-partition-root is used. In other words, it's not utterly fixed in stone --- we invented --load-via-partition-root primarily to cope with circumstances that could change hash values --- but we sure don't want to be changing it with any regularity, or for a less-than-excellent reason. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Andres Freund <andres@anarazel.de> — 2017-10-12T20:20:28Z
On 2017-10-12 16:06:11 -0400, Robert Haas wrote: > On Thu, Oct 12, 2017 at 3:43 PM, Andres Freund <andres@anarazel.de> wrote: > > Are we going to rely on the the combine function to stay the same > > forever after? > > If we change them, it will be a pg_upgrade compatibility break for > anyone using hash-partitioned tables with more than one partitioning > column. Dump and reload will also break unless > --load-via-partition-root is used. > > In other words, it's not utterly fixed in stone --- we invented > --load-via-partition-root primarily to cope with circumstances that > could change hash values --- but we sure don't want to be changing it > with any regularity, or for a less-than-excellent reason. Yea, that's what I expected. It'd probably good for somebody to run smhasher or such on the output of the combine function (or even better, on both the 32 and 64 bit variants) in that case. Greetings, Andres Freund
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-10-12T21:27:52Z
On Thu, Oct 12, 2017 at 4:20 PM, Andres Freund <andres@anarazel.de> wrote: >> In other words, it's not utterly fixed in stone --- we invented >> --load-via-partition-root primarily to cope with circumstances that >> could change hash values --- but we sure don't want to be changing it >> with any regularity, or for a less-than-excellent reason. > > Yea, that's what I expected. It'd probably good for somebody to run > smhasher or such on the output of the combine function (or even better, > on both the 32 and 64 bit variants) in that case. Not sure how that test suite works exactly, but presumably the characteristics in practice will depend the behavior of the hash functions used as input the combine function - so the behavior could be good for an (int, int) key but bad for a (text, date) key, or whatever. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Andres Freund <andres@anarazel.de> — 2017-10-12T21:30:29Z
On 2017-10-12 17:27:52 -0400, Robert Haas wrote: > On Thu, Oct 12, 2017 at 4:20 PM, Andres Freund <andres@anarazel.de> wrote: > >> In other words, it's not utterly fixed in stone --- we invented > >> --load-via-partition-root primarily to cope with circumstances that > >> could change hash values --- but we sure don't want to be changing it > >> with any regularity, or for a less-than-excellent reason. > > > > Yea, that's what I expected. It'd probably good for somebody to run > > smhasher or such on the output of the combine function (or even better, > > on both the 32 and 64 bit variants) in that case. > > Not sure how that test suite works exactly, but presumably the > characteristics in practice will depend the behavior of the hash > functions used as input the combine function - so the behavior could > be good for an (int, int) key but bad for a (text, date) key, or > whatever. I don't think that's true, unless you have really bad hash functions on the the component hashes. A hash combine function can't really do anything about badly hashed input, what you want is that it doesn't *reduce* the quality of the hash by combining. Greetings, Andres Freund
-
Re: [POC] hash partitioning
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-10-16T09:06:01Z
On Tue, Oct 10, 2017 at 4:37 PM, amul sul <sulamul@gmail.com> wrote: > On Tue, Oct 10, 2017 at 3:42 PM, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: >> On Tue, Oct 10, 2017 at 3:32 PM, amul sul <sulamul@gmail.com> wrote: >> >>>> + hash_part? true : key->parttypbyval[j], >>>> + key->parttyplen[j]); >>>> parttyplen is the length of partition key attribute, whereas what you want here >>>> is the length of type of modulus and remainder. Is that correct? Probably we >>>> need some special handling wherever parttyplen and parttypbyval is used e.g. in >>>> call to partition_bounds_equal() from build_joinrel_partition_info(). >>>> >>> >>> Unless I am missing something, I don't think we should worry about parttyplen >>> because in the datumCopy() when the datatype is pass-by-value then typelen >>> is ignored. >> >> That's true, but it's ugly, passing typbyvalue of one type and len of other. >> > > How about the attached patch(0003)? > Also, the dim variable is renamed to natts. Probably we should move changes to partition_bounds_copy() in 0003 to 0001, whose name suggests so. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
-
Re: [POC] hash partitioning
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-10-16T09:44:05Z
On Mon, Oct 16, 2017 at 2:36 PM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > > Probably we should move changes to partition_bounds_copy() in 0003 to > 0001, whose name suggests so. > We can't do this, hash partition strategy is introduced by 0002. Sorry for the noise. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-10-24T07:13:12Z
On Fri, Oct 13, 2017 at 3:00 AM, Andres Freund <andres@anarazel.de> wrote: > On 2017-10-12 17:27:52 -0400, Robert Haas wrote: >> On Thu, Oct 12, 2017 at 4:20 PM, Andres Freund <andres@anarazel.de> wrote: >> >> In other words, it's not utterly fixed in stone --- we invented >> >> --load-via-partition-root primarily to cope with circumstances that >> >> could change hash values --- but we sure don't want to be changing it >> >> with any regularity, or for a less-than-excellent reason. >> > >> > Yea, that's what I expected. It'd probably good for somebody to run >> > smhasher or such on the output of the combine function (or even better, >> > on both the 32 and 64 bit variants) in that case. >> >> Not sure how that test suite works exactly, but presumably the >> characteristics in practice will depend the behavior of the hash >> functions used as input the combine function - so the behavior could >> be good for an (int, int) key but bad for a (text, date) key, or >> whatever. > > I don't think that's true, unless you have really bad hash functions on > the the component hashes. A hash combine function can't really do > anything about badly hashed input, what you want is that it doesn't > *reduce* the quality of the hash by combining. > I tried to get suggested SMHasher[1] test result for the hash_combine for 32-bit and 64-bit version. SMHasher works on hash keys of the form {0}, {0,1}, {0,1,2}... up to N=255, using 256-N as the seed, for the hash_combine testing we needed two hash value to be combined, for that, I've generated 64 and 128-bit hash using cityhash functions[2] for the given smhasher key then split in two part to test 32-bit and 64-bit hash_combine function respectively. Attached patch for SMHasher code changes & output of 32-bit and 64-bit hash_combine testing. Note that I have skipped speed test this test which is irrelevant here. By referring other hash function results [3], we can see that hash_combine test results are not bad either. Do let me know if current testing is not good enough or if you want me to do more testing, thanks. 1] https://github.com/aappleby/smhasher 2] https://github.com/aappleby/smhasher/blob/master/src/CityTest.cpp 3] https://github.com/rurban/smhasher/tree/master/doc Regards, Amul -
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-10-24T11:21:27Z
On Thu, Oct 12, 2017 at 6:38 PM, amul sul <sulamul@gmail.com> wrote: > On Thu, Oct 12, 2017 at 6:31 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Tue, Oct 10, 2017 at 7:07 AM, amul sul <sulamul@gmail.com> wrote: >>> How about the attached patch(0003)? >>> Also, the dim variable is renamed to natts. >> >> I'm not sure I believe this comment: >> >> + /* >> + * We arrange the partitions in the ascending order of their modulus >> + * and remainders. Also every modulus is factor of next larger >> + * modulus. This means that the index of a given partition is same as >> + * the remainder of that partition. Also entries at (remainder + N * >> + * modulus) positions in indexes array are all same for (modulus, >> + * remainder) specification for any partition. Thus datums array from >> + * both the given bounds are same, if and only if their indexes array >> + * will be same. So, it suffices to compare indexes array. >> + */ >> >> I am particularly not sure that I believe that the index of a >> partition must be the same as the remainder. It doesn't seem like >> that would be true when there is more than one modulus or when some >> partitions are missing. >> > > Looks like an explanation by the comment is not good enough, will think on this. > > Here are the links for the previous discussion: > 1] https://postgr.es/m/CAFjFpRfHqSGBjNgJV2p%2BC4Yr5Qxvwygdsg4G_VQ6q9NTB-i3MA%40mail.gmail.com > 2] https://postgr.es/m/CAFjFpRdeESKFkVGgmOdYvmD3d56-58c5VCBK0zDRjHrkq_VcNg%40mail.gmail.com > I have modified the comment little bit, now let me explain the theory behind it. rd_partdesc->boundinfo->indexes array stores an index in rd_partdesc->oids array corresponding to a given partition falls at the positions. And position in indexes array is decided using remainder + N * modulus_of_that_partition (where N = 0,1,2,..,). For the case where the same modulus, the remainder will be 0,1,2,.., and the index of that partition will be at 0,1,2,..,. (N=0). For the case where more than one modulus then an index of a partition oid in the oids array could be stored at the multiple places in indexes array if its modulus is < greatest_modulus amongst bound (where N = 0,1,2,..,). For example, partition bound (Modulus, remainder) = p1(2,0), p2(4,1), p3(8,3), p4(8,7) Oids array [p1,p2,p3,p4] sorted by Modulus and then by remainder and indexes array [0, 1, 0, 3, 0, 1, 0, 4] size of indexes array is greatest_modulus. In other word, if a partition index in oids array in the indexes array is stored multiple times, then the lowest of the differences between them is the modulus of that partition. In above case for the partition p1, index in oids array stored at 0,2,4,6. You can see lowest is the remainder and minimum difference is the modulus of p1. Since indexes arrays in both the bounds are same, for a given index in oids array, the positions where it falls is same for both bounds. One can argue that two different moduli could have the same remainder position, which is not allowed because that will cause partition overlap error at creation and also we have a restriction on modulus that each modulus in the hash partition bound should be the factor of next modulus. > [....] > >> +static uint64 >> +mix_hash_value(int nkeys, Datum *hash_array, bool *isnull) >> > How about combining high 32 bits and the low 32 bits separately as shown below? > > static inline uint64 > hash_combine64(uint64 a, uint64 b) > { > return (((uint64) hash_combine((uint32) a >> 32, (uint32) b >> 32) << 32) > | hash_combine((unit32) a, (unit32) b)); > } > I have used hash_combine64 function suggested by Andres [1]. >[....] >> Have you checked how well the tests you've added cover the code you've >> added? What code is not covered by the tests, and is there any way to >> cover it? >> > Will try to get gcov report for this patch. > Tests in the attached patch covers almost all the code expect few[2]. Updated patch attached. 1] https://postgr.es/m/20171012194353.3nealiykmjura4bi%40alap3.anarazel.de 2] Refer gcov_output.txt attachment. Regards, Amul Sul -
Re: [POC] hash partitioning
Andres Freund <andres@anarazel.de> — 2017-10-24T11:30:04Z
On 2017-10-24 12:43:12 +0530, amul sul wrote: > I tried to get suggested SMHasher[1] test result for the hash_combine > for 32-bit and 64-bit version. > > SMHasher works on hash keys of the form {0}, {0,1}, {0,1,2}... up to > N=255, using 256-N as the seed, for the hash_combine testing we > needed two hash value to be combined, for that, I've generated 64 > and 128-bit hash using cityhash functions[2] for the given smhasher > key then split in two part to test 32-bit and 64-bit hash_combine > function respectively. Attached patch for SMHasher code changes & > output of 32-bit and 64-bit hash_combine testing. Note that I have > skipped speed test this test which is irrelevant here. > > By referring other hash function results [3], we can see that hash_combine > test results are not bad either. > > Do let me know if current testing is not good enough or if you want me to do > more testing, thanks. This looks very good! Both the tests you did, and the results for hash_combineXX. I therefore think we can go ahead with that formulation of hash_combine64? Greetings, Andres Freund -
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-10-24T11:35:26Z
On Tue, Oct 24, 2017 at 5:00 PM, Andres Freund <andres@anarazel.de> wrote: > On 2017-10-24 12:43:12 +0530, amul sul wrote: >> I tried to get suggested SMHasher[1] test result for the hash_combine >> for 32-bit and 64-bit version. >> >> SMHasher works on hash keys of the form {0}, {0,1}, {0,1,2}... up to >> N=255, using 256-N as the seed, for the hash_combine testing we >> needed two hash value to be combined, for that, I've generated 64 >> and 128-bit hash using cityhash functions[2] for the given smhasher >> key then split in two part to test 32-bit and 64-bit hash_combine >> function respectively. Attached patch for SMHasher code changes & >> output of 32-bit and 64-bit hash_combine testing. Note that I have >> skipped speed test this test which is irrelevant here. >> >> By referring other hash function results [3], we can see that hash_combine >> test results are not bad either. >> >> Do let me know if current testing is not good enough or if you want me to do >> more testing, thanks. > > This looks very good! Both the tests you did, and the results for > hash_combineXX. I therefore think we can go ahead with that formulation > of hash_combine64? > Thanks, Andres. Yes we can, I've added your suggested hash_combine64 in the latest patch[1]. Regards, Amul 1] https://postgr.es/m/CAAJ_b97R2rJinGPAVmZZzpNV%3D-5BgYFxDfY9HYdM1bCYJFGmQw%40mail.gmail.com -
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-10-29T07:08:28Z
On Tue, Oct 24, 2017 at 1:21 PM, amul sul <sulamul@gmail.com> wrote: > Updated patch attached. This patch needs a rebase. It appears that satisfies_hash_func is declared incorrectly in pg_proc.h. ProcedureCreate seems to think that provariadic should be ANYOID if the type of the last element is ANYOID, ANYELEMENTOID if the type of the last element is ANYARRAYOID, and otherwise the element type corresponding to the array type. But here you have the last element as int4[] but provariadic is any. I wrote the following query to detect problems of this type, and I think we might want to just go ahead and add this to the regression test suite, verifying that it returns no rows: select oid::regprocedure, provariadic::regtype, proargtypes::regtype[] from pg_proc where provariadic != 0 and case proargtypes[array_length(proargtypes, 1)-1] when 2276 then 2276 -- any -> any when 2277 then 2283 -- anyarray -> anyelement else (select t.oid from pg_type t where t.typarray = proargtypes[array_length(proargtypes, 1)-1]) end != provariadic; The simple fix is change provariadic to int4 and call it good. It's tempting to go the other way and actually make it satisfies_hash_partition(int4, int4, variadic "any"), passing the column values directly and letting satisfies_hash_partition doing the hashing itself. Any arguments that had a partition key type different from the column type would have a RelabelType node placed on top of the column, so that get_fn_expr_argtype would return the partition key type. Then, the function could look up the hash function for that type and call it directly on the value. That way, we'd be doing only one function call instead of many, and the partition constraint would look nicer in \d+ output, too. :-) On the other hand, that would also mean that we'd have to look up the extended hash function every time through this function, though maybe that could be prevented by using fn_extra to cache FmgrInfos for all the hash functions on the first time through. I'm not sure how that would compare in terms of speed with what you have now, but maybe it's worth trying. The second paragraph of the CREATE TABLE documentation for PARTITION OF needs to be updated like this: "The form with <literal>IN</literal> is used for list partitioning, the form with <literal>FROM</literal> and <literal>TO</literal> is used for range partitioning, and the form with <literal>WITH</literal> is used for hash partitioning." The CREATE TABLE documentation says "When using range partitioning, the partition key can include multiple columns or expressions (up to 32,"; this should be changed to say "When using range or hash partitioning". - expression. If no B-tree operator class is specified when creating a - partitioned table, the default B-tree operator class for the datatype will - be used. If there is none, an error will be reported. + expression. If no operator class is specified when creating a partitioned + table, the default operator class of the appropriate type (btree for list + and range partitioning, hash for hash partitioning) will be used. If + there is none, an error will be reported. + </para> + + <para> + Since hash operator class provides only equality, not ordering, collation + is not relevant for hash partitioning. The behaviour will be unaffected + if a collation is specified. + </para> + + <para> + Hash partitioning will use support function 2 routines from the operator + class. If there is none, an error will be reported. See <xref + linkend="xindex-support"> for details of operator class support + functions. I think we should rework this a little more heavily. I suggest the following, starting after "a single column or expression": <para> Range and list partitioning require a btree operator class, while hash partitioning requires a hash operator class. If no operator class is specified explicitly, the default operator class of the appropriate type will be used; if no default operator class exists, an error will be raised. When hash partitioning is used, the operator class used must implement support function 2 (see <xref linkend="xindex-support"> for details). </para> I think we can leave out the part about collations. It's possibly worth a longer explanation here at some point: for range partitioning, collation can affect which rows go into which partitions; for list partitioning, it can't, but it can affect the order in which partitions are expanded (which is a can of worms I'm not quite ready to try to explain in user-facing documentation); for hash partitioning, it makes no difference at all. Although at some point we may want to document this, I think it's a job for a separate patch, since (1) the existing documentation doesn't document the precise import of collations on existing partitioning types and (2) I'm not sure that CREATE TABLE is really the best place to explain this. The example commands for creating a hash-partitioned table are missing spaces between WITH and the parenthesis which follows. In 0003, the changes to partition_bounds_copy claim that I shouldn't worry about the fact that typlen is set to 4 because datumCopy won't use it for a pass-by-value datatype, but I think that calling functions with incorrect arguments and hoping that they ignore them and therefore nothing bad happens doesn't sound like a very good idea. Fortunately, I think the actual code is fine; I think we just need to change the comments. For hash partitioning, the datums array always contains two integers, which are of type int4, which is indeed a pass-by-value type of length 4 (note that if we were using int8 for the modulus and remainder, we'd need to set byval to FLOAT8PASSBYVAL). I would just write this as: if (hash_part) { typlen = sizeof(int32); /* always int4 */ byval = true; /* int4 is pass-by-value */ } + for (i = 0; i < nkeys; i++) + { + if (!isnull[i]) + rowHash = hash_combine64(rowHash, DatumGetUInt64(hash_array[i])); + } Excess braces. I think it might be better to inline the logic in mix_hash_value() into each of the two callers. Then, the callers wouldn't need Datum hash_array[PARTITION_MAX_KEYS]; they could just fold each new hash value into a uint64 value. That seems likely to be slightly faster and I don't see any real downside. rhaas=# create table natch (a citext, b text) partition by hash (a); ERROR: XX000: missing support function 2(16398,16398) in opfamily 16437 LOCATION: RelationBuildPartitionKey, relcache.c:954 It shouldn't be possible to reach an elog() from SQL, and this is not a friendly error message. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-10-30T12:22:35Z
On Sun, Oct 29, 2017 at 12:38 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Oct 24, 2017 at 1:21 PM, amul sul <sulamul@gmail.com> wrote: >> Updated patch attached. > > This patch needs a rebase. Sure, thanks a lot for your review. > > It appears that satisfies_hash_func is declared incorrectly in > pg_proc.h. ProcedureCreate seems to think that provariadic should be > ANYOID if the type of the last element is ANYOID, ANYELEMENTOID if the > type of the last element is ANYARRAYOID, and otherwise the element > type corresponding to the array type. But here you have the last > element as int4[] but provariadic is any. Actually, int4[] is also inappropriate type as we have started using a 64bit hash function. We need something int8[] which is not available, so that I have used ANYARRAYOID in the attached patch(0004). > I wrote the following query > to detect problems of this type, and I think we might want to just go > ahead and add this to the regression test suite, verifying that it > returns no rows: > > select oid::regprocedure, provariadic::regtype, proargtypes::regtype[] > from pg_proc where provariadic != 0 > and case proargtypes[array_length(proargtypes, 1)-1] > when 2276 then 2276 -- any -> any > when 2277 then 2283 -- anyarray -> anyelement > else (select t.oid from pg_type t where t.typarray = > proargtypes[array_length(proargtypes, 1)-1]) end > != provariadic; > Added in 0001 patch. > The simple fix is change provariadic to int4 and call it good. It's > tempting to go the other way and actually make it > satisfies_hash_partition(int4, int4, variadic "any"), passing the > column values directly and letting satisfies_hash_partition doing the > hashing itself. Any arguments that had a partition key type different > from the column type would have a RelabelType node placed on top of > the column, so that get_fn_expr_argtype would return the partition key > type. Then, the function could look up the hash function for that > type and call it directly on the value. That way, we'd be doing only > one function call instead of many, and the partition constraint would > look nicer in \d+ output, too. :-) On the other hand, that would > also mean that we'd have to look up the extended hash function every > time through this function, though maybe that could be prevented by > using fn_extra to cache FmgrInfos for all the hash functions on the > first time through. I'm not sure how that would compare in terms of > speed with what you have now, but maybe it's worth trying. > One advantage of current implementation is that we can see which hash function are used for the each partitioning column and also we don't need to worry about user specified opclass and different input types. Something similar I've tried in my initial patch version[1], but I have missed user specified opclass handling for each partitioning column. Do you want me to handle opclass using RelabelType node? I am afraid that, that would make the \d+ output more horrible than the current one if non-default opclass used. > The second paragraph of the CREATE TABLE documentation for PARTITION > OF needs to be updated like this: "The form with <literal>IN</literal> > is used for list partitioning, the form with <literal>FROM</literal> > and <literal>TO</literal> is used for range partitioning, and the form > with <literal>WITH</literal> is used for hash partitioning." > Fixed in the attached version(0004). > The CREATE TABLE documentation says "When using range partitioning, > the partition key can include multiple columns or expressions (up to > 32,"; this should be changed to say "When using range or hash > partitioning". > Fixed in the attached version(0004). > - expression. If no B-tree operator class is specified when creating a > - partitioned table, the default B-tree operator class for the > datatype will > - be used. If there is none, an error will be reported. > + expression. If no operator class is specified when creating a > partitioned > + table, the default operator class of the appropriate type (btree for list > + and range partitioning, hash for hash partitioning) will be used. If > + there is none, an error will be reported. > + </para> > + > + <para> > + Since hash operator class provides only equality, not ordering, collation > + is not relevant for hash partitioning. The behaviour will be unaffected > + if a collation is specified. > + </para> > + > + <para> > + Hash partitioning will use support function 2 routines from the operator > + class. If there is none, an error will be reported. See <xref > + linkend="xindex-support"> for details of operator class support > + functions. > > I think we should rework this a little more heavily. I suggest the > following, starting after "a single column or expression": > > <para> > Range and list partitioning require a btree operator class, while hash > partitioning requires a hash operator class. If no operator class is > specified explicitly, the default operator class of the appropriate > type will be used; if no default operator class exists, an error will > be raised. When hash partitioning is used, the operator class used > must implement support function 2 (see <xref linkend="xindex-support"> > for details). > </para> > Thanks again, added in the attached version(0004). > I think we can leave out the part about collations. It's possibly > worth a longer explanation here at some point: for range partitioning, > collation can affect which rows go into which partitions; for list > partitioning, it can't, but it can affect the order in which > partitions are expanded (which is a can of worms I'm not quite ready > to try to explain in user-facing documentation); for hash > partitioning, it makes no difference at all. Although at some point > we may want to document this, I think it's a job for a separate patch, > since (1) the existing documentation doesn't document the precise > import of collations on existing partitioning types and (2) I'm not > sure that CREATE TABLE is really the best place to explain this. > Okay. > The example commands for creating a hash-partitioned table are missing > spaces between WITH and the parenthesis which follows. > Fixed in the attached version(0004). > In 0003, the changes to partition_bounds_copy claim that I shouldn't > worry about the fact that typlen is set to 4 because datumCopy won't > use it for a pass-by-value datatype, but I think that calling > functions with incorrect arguments and hoping that they ignore them > and therefore nothing bad happens doesn't sound like a very good idea. > Fortunately, I think the actual code is fine; I think we just need to > change the comments. For hash partitioning, the datums array always > contains two integers, which are of type int4, which is indeed a > pass-by-value type of length 4 (note that if we were using int8 for > the modulus and remainder, we'd need to set byval to FLOAT8PASSBYVAL). > I would just write this as: > > if (hash_part) > { > typlen = sizeof(int32); /* always int4 */ > byval = true; /* int4 is pass-by-value */ > } > Fixed in the attached version (now patch number is 0005). > + for (i = 0; i < nkeys; i++) > + { > + if (!isnull[i]) > + rowHash = hash_combine64(rowHash, > DatumGetUInt64(hash_array[i])); > + } > > Excess braces. > Fixed in the attached version(0004). > I think it might be better to inline the logic in mix_hash_value() > into each of the two callers. Then, the callers wouldn't need Datum > hash_array[PARTITION_MAX_KEYS]; they could just fold each new hash > value into a uint64 value. That seems likely to be slightly faster > and I don't see any real downside. > Fixed in the attached version(0004). > rhaas=# create table natch (a citext, b text) partition by hash (a); > ERROR: XX000: missing support function 2(16398,16398) in opfamily 16437 > LOCATION: RelationBuildPartitionKey, relcache.c:954 > > It shouldn't be possible to reach an elog() from SQL, and this is not > a friendly error message. > How about an error message in the attached patch(0004)? 1] https://postgr.es/m/CAAJ_b96AQBAxSQ2mxnTmx9zXh79GdP_dQWv0aupjcmz+jpiGjw@mail.gmail.com Regards, Amul -
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-10-31T04:24:10Z
On Mon, Oct 30, 2017 at 5:52 PM, amul sul <sulamul@gmail.com> wrote: > Actually, int4[] is also inappropriate type as we have started using a 64bit > hash function. We need something int8[] which is not available, so that I > have used ANYARRAYOID in the attached patch(0004). I don't know why you think int8[] is not available. rhaas=# select 'int8[]'::regtype; regtype ---------- bigint[] (1 row) >> I wrote the following query >> to detect problems of this type, and I think we might want to just go >> ahead and add this to the regression test suite, verifying that it >> returns no rows: >> >> select oid::regprocedure, provariadic::regtype, proargtypes::regtype[] >> from pg_proc where provariadic != 0 >> and case proargtypes[array_length(proargtypes, 1)-1] >> when 2276 then 2276 -- any -> any >> when 2277 then 2283 -- anyarray -> anyelement >> else (select t.oid from pg_type t where t.typarray = >> proargtypes[array_length(proargtypes, 1)-1]) end >> != provariadic; >> > > Added in 0001 patch. Committed. > One advantage of current implementation is that we can see which hash > function are used for the each partitioning column and also we don't need to > worry about user specified opclass and different input types. > > Something similar I've tried in my initial patch version[1], but I have missed > user specified opclass handling for each partitioning column. Do you want me > to handle opclass using RelabelType node? I am afraid that, that would make > the \d+ output more horrible than the current one if non-default opclass used. Maybe we should just pass the OID of the partition (or both the partition and the parent, so we can get the lock ordering right?) instead. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-10-31T04:47:33Z
On Tue, Oct 31, 2017 at 9:54 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Oct 30, 2017 at 5:52 PM, amul sul <sulamul@gmail.com> wrote: >> Actually, int4[] is also inappropriate type as we have started using a 64bit >> hash function. We need something int8[] which is not available, so that I >> have used ANYARRAYOID in the attached patch(0004). > > I don't know why you think int8[] is not available. > > rhaas=# select 'int8[]'::regtype; > regtype > ---------- > bigint[] > (1 row) > I missed _int8, was searching for INT8ARRAYOID in pg_type.h, my bad. >>> I wrote the following query >>> to detect problems of this type, and I think we might want to just go >>> ahead and add this to the regression test suite, verifying that it >>> returns no rows: >>> >>> select oid::regprocedure, provariadic::regtype, proargtypes::regtype[] >>> from pg_proc where provariadic != 0 >>> and case proargtypes[array_length(proargtypes, 1)-1] >>> when 2276 then 2276 -- any -> any >>> when 2277 then 2283 -- anyarray -> anyelement >>> else (select t.oid from pg_type t where t.typarray = >>> proargtypes[array_length(proargtypes, 1)-1]) end >>> != provariadic; >>> >> >> Added in 0001 patch. > > Committed. > Thanks ! >> One advantage of current implementation is that we can see which hash >> function are used for the each partitioning column and also we don't need to >> worry about user specified opclass and different input types. >> >> Something similar I've tried in my initial patch version[1], but I have missed >> user specified opclass handling for each partitioning column. Do you want me >> to handle opclass using RelabelType node? I am afraid that, that would make >> the \d+ output more horrible than the current one if non-default opclass used. > > Maybe we should just pass the OID of the partition (or both the > partition and the parent, so we can get the lock ordering right?) > instead. > Okay, will try this. Regards, Amul
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-11-01T10:16:27Z
On Tue, Oct 31, 2017 at 10:17 AM, amul sul <sulamul@gmail.com> wrote: > On Tue, Oct 31, 2017 at 9:54 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Mon, Oct 30, 2017 at 5:52 PM, amul sul <sulamul@gmail.com> wrote: >>> Actually, int4[] is also inappropriate type as we have started using a 64bit >>> hash function. We need something int8[] which is not available, so that I >>> have used ANYARRAYOID in the attached patch(0004). >> >> I don't know why you think int8[] is not available. >> >> rhaas=# select 'int8[]'::regtype; >> regtype >> ---------- >> bigint[] >> (1 row) >> > > I missed _int8, was searching for INT8ARRAYOID in pg_type.h, my bad. > Fixed in the 0003 patch. >>>>[....] >>> Something similar I've tried in my initial patch version[1], but I have missed >>> user specified opclass handling for each partitioning column. Do you want me >>> to handle opclass using RelabelType node? I am afraid that, that would make >>> the \d+ output more horrible than the current one if non-default opclass used. >> >> Maybe we should just pass the OID of the partition (or both the >> partition and the parent, so we can get the lock ordering right?) >> instead. >> > Okay, will try this. > In 0005, I rewrote satisfies_hash_partition, to accept parent id, modulus and remainder as before, and the column values directly. This function opens parent relation to get its PartitionKey which has extended hash function information in a partsupfunc array, using this it will calculates a hash for the partition key. Also, it will copy this partsupfunc array into function memory context so that we don't need to open parent relation again and again in the subsequent function call to get extended hash functions information (e.g. bulk insert). In \d+ partition constraint will be : satisfies_hash_partition('16384'::oid, 2, 0, a, b) where 16384 is parent relid, 2 is modulus, 0 is remainder and 'a' & 'b' are partition column. In the earlier version partition constraint was (i.e. without 0005 patch): satisfies_hash_partition(2, 0, hashint4extended(a,'8816678312871386365'::bigint), hashtextextended(b, '8816678312871386365'::bigint)) I did small performance test using a copy command to load 100,000,000 records and a separate insert command for each record to load 2,00,000 records and result are as follow: +---------+-----------------+--------------------+ | Command | With 0005 patch | Without 0005 patch | +---------+-----------------+--------------------+ | COPY | 63.719 seconds | 64.925 seconds | +---------+-----------------+--------------------+ | INSERT | 179.21 seconds | 174.89 seconds | +---------+-----------------+--------------------+ Although partition constraints become more simple, there isn't any performance gain with 0005 patch. Also I am little skeptic about logic in 0005 where we copied extended hash function info from the partition key, what if parent is changed while we are using it? Do we need to keep lock on parent until commit in satisfies_hash_partition? Regards, Amul -
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-11-02T08:05:48Z
On Wed, Nov 1, 2017 at 3:46 PM, amul sul <sulamul@gmail.com> wrote: > Although partition constraints become more simple, there isn't any performance > gain with 0005 patch. Also I am little skeptic about logic in 0005 where we > copied extended hash function info from the partition key, what if parent is > changed while we are using it? Do we need to keep lock on parent until commit in > satisfies_hash_partition? I don't think it should be possible for the parent to be changed. I mean, the partition key is altogether immutable -- it can't be changed after creation time. The partition bounds can be changed for individual partitions but that would require a lock on the partition. Can you give an example of the kind of scenario about which you are concerned? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-11-02T08:15:22Z
On Thu, Nov 2, 2017 at 1:35 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Nov 1, 2017 at 3:46 PM, amul sul <sulamul@gmail.com> wrote: >> Although partition constraints become more simple, there isn't any performance >> gain with 0005 patch. Also I am little skeptic about logic in 0005 where we >> copied extended hash function info from the partition key, what if parent is >> changed while we are using it? Do we need to keep lock on parent until commit in >> satisfies_hash_partition? > > I don't think it should be possible for the parent to be changed. I > mean, the partition key is altogether immutable -- it can't be changed > after creation time. The partition bounds can be changed for > individual partitions but that would require a lock on the partition. > > Can you give an example of the kind of scenario about which you are concerned? > Yes, you are correct, column involved in the partitioning are immutable. I was just worried about any change in the partition key column that might change selected hash function. Regards, Amul
-
Re: [POC] hash partitioning
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-11-02T08:22:51Z
On Thu, Nov 2, 2017 at 1:35 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Nov 1, 2017 at 3:46 PM, amul sul <sulamul@gmail.com> wrote: >> Although partition constraints become more simple, there isn't any performance >> gain with 0005 patch. Also I am little skeptic about logic in 0005 where we >> copied extended hash function info from the partition key, what if parent is >> changed while we are using it? Do we need to keep lock on parent until commit in >> satisfies_hash_partition? > > I don't think it should be possible for the parent to be changed. I > mean, the partition key is altogether immutable -- it can't be changed > after creation time. The partition bounds can be changed for > individual partitions but that would require a lock on the partition. > > Can you give an example of the kind of scenario about which you are concerned? Right now partition keys are immutable but we don't have much code written with that assumption. All the code usually keeps a lock on the parent till the time they use the information in the partition key. In a distant future, which may not exist, we may support ALTER TABLE ... PARTITION BY to change partition keys (albeit at huge cost of data movement). If we do that, we will have to remember this one-off instance of code which assumes that the partition keys are immutable. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-11-02T09:01:54Z
On Thu, Nov 2, 2017 at 1:45 PM, amul sul <sulamul@gmail.com> wrote: > Yes, you are correct, column involved in the partitioning are immutable. > > I was just worried about any change in the partition key column that > might change selected hash function. Any such change, even if it were allowed, would have to take AccessExclusiveLock on the child. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-11-02T09:02:25Z
On Thu, Nov 2, 2017 at 1:52 PM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > Right now partition keys are immutable but we don't have much code > written with that assumption. All the code usually keeps a lock on the > parent till the time they use the information in the partition key. In > a distant future, which may not exist, we may support ALTER TABLE ... > PARTITION BY to change partition keys (albeit at huge cost of data > movement). If we do that, we will have to remember this one-off > instance of code which assumes that the partition keys are immutable. I am pretty sure this is by no means the only piece of code which assumes that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Robert Haas <robertmhaas@gmail.com> — 2017-11-09T23:11:32Z
On Wed, Nov 1, 2017 at 6:16 AM, amul sul <sulamul@gmail.com> wrote: > Fixed in the 0003 patch. I have committed this patch set with the attached adjustments. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [POC] hash partitioning
Amul Sul <sulamul@gmail.com> — 2017-11-10T05:38:05Z
On Fri, Nov 10, 2017 at 4:41 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Nov 1, 2017 at 6:16 AM, amul sul <sulamul@gmail.com> wrote: >> Fixed in the 0003 patch. > > I have committed this patch set with the attached adjustments. > Thanks a lot for your support & a ton of thanks to all reviewer. Regards, Amul