Thread
Commits
-
Fix multiple problems with satisfies_hash_partition.
- f3b0897a1213 11.0 landed
-
Add hash partitioning.
- 1aba8e651ac3 11.0 cited
-
pgsql: Add hash partitioning.
Robert Haas <rhaas@postgresql.org> — 2017-11-09T23:10:11Z
Add hash partitioning. Hash partitioning is useful when you want to partition a growing data set evenly. This can be useful to keep table sizes reasonable, which makes maintenance operations such as VACUUM faster, or to enable partition-wise join. At present, we still depend on constraint exclusion for partitioning pruning, and the shape of the partition constraints for hash partitioning is such that that doesn't work. Work is underway to fix that, which should both improve performance and make partitioning pruning work with hash partitioning. Amul Sul, reviewed and tested by Dilip Kumar, Ashutosh Bapat, Yugo Nagata, Rajkumar Raghuwanshi, Jesper Pedersen, and by me. A few final tweaks also by me. Discussion: http://postgr.es/m/CAAJ_b96fhpJAP=ALbETmeLk1Uni_GFZD938zgenhF49qgDTjaQ@mail.gmail.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/1aba8e651ac3e37e1d2d875842de1e0ed22a651e Modified Files -------------- doc/src/sgml/ddl.sgml | 28 +- doc/src/sgml/ref/alter_table.sgml | 7 + doc/src/sgml/ref/create_table.sgml | 85 +++- src/backend/catalog/partition.c | 682 ++++++++++++++++++++++++--- src/backend/commands/tablecmds.c | 48 +- src/backend/nodes/copyfuncs.c | 2 + src/backend/nodes/equalfuncs.c | 2 + src/backend/nodes/outfuncs.c | 2 + src/backend/nodes/readfuncs.c | 2 + src/backend/optimizer/path/joinrels.c | 12 +- src/backend/parser/gram.y | 76 ++- src/backend/parser/parse_utilcmd.c | 29 +- src/backend/utils/adt/ruleutils.c | 15 +- src/backend/utils/cache/relcache.c | 26 +- src/bin/psql/tab-complete.c | 2 +- src/include/catalog/catversion.h | 2 +- src/include/catalog/partition.h | 3 + src/include/catalog/pg_proc.h | 4 + src/include/nodes/parsenodes.h | 8 +- src/test/regress/expected/alter_table.out | 62 +++ src/test/regress/expected/create_table.out | 78 ++- src/test/regress/expected/insert.out | 46 ++ src/test/regress/expected/partition_join.out | 81 ++++ src/test/regress/expected/update.out | 29 ++ src/test/regress/sql/alter_table.sql | 64 +++ src/test/regress/sql/create_table.sql | 51 +- src/test/regress/sql/insert.sql | 33 ++ src/test/regress/sql/partition_join.sql | 32 ++ src/test/regress/sql/update.sql | 28 ++ src/tools/pgindent/typedefs.list | 1 + 30 files changed, 1420 insertions(+), 120 deletions(-)
-
Re: pgsql: Add hash partitioning.
Andreas Seltenreich <seltenreich@gmx.de> — 2017-11-11T21:31:48Z
Robert Haas writes: > Add hash partitioning. sqlsmith triggers coredumps calling satisfies_hash_partition(). ISTM this function is lacking argument validation. Example: ,---- | PostgreSQL stand-alone backend 11devel | backend> select satisfies_hash_partition('pg_class'::regclass,null,null,null); | Program received signal SIGSEGV, Segmentation fault. | 0x00005555556b3914 in satisfies_hash_partition (fcinfo=0x555555f5f668) at partition.c:3435 | 3435 fmgr_info_copy(&my_extra->partsupfunc[j], `---- regards, Andreas -
Re: pgsql: Add hash partitioning.
Amul Sul <sulamul@gmail.com> — 2017-11-13T07:11:23Z
On Sun, Nov 12, 2017 at 3:01 AM, Andreas Seltenreich <seltenreich@gmx.de> wrote: > Robert Haas writes: > >> Add hash partitioning. > > sqlsmith triggers coredumps calling satisfies_hash_partition(). > ISTM this function is lacking argument validation. Example: > Thanks for the bug report. Please find attached patch does the fix. Regards, Amul
-
Re: pgsql: Add hash partitioning.
Amul Sul <sulamul@gmail.com> — 2017-11-13T08:24:27Z
On Mon, Nov 13, 2017 at 12:41 PM, amul sul <sulamul@gmail.com> wrote: > On Sun, Nov 12, 2017 at 3:01 AM, Andreas Seltenreich <seltenreich@gmx.de> wrote: >> Robert Haas writes: >> >>> Add hash partitioning. >> >> sqlsmith triggers coredumps calling satisfies_hash_partition(). >> ISTM this function is lacking argument validation. Example: >> > > Thanks for the bug report. Please find attached patch does the fix. Updated patch attached -- Adjusted code comment to survive against pgindent. Regards, Amul
-
Re: [COMMITTERS] pgsql: Add hash partitioning.
Robert Haas <robertmhaas@gmail.com> — 2017-11-13T18:59:02Z
On Mon, Nov 13, 2017 at 3:24 AM, amul sul <sulamul@gmail.com> wrote: > Updated patch attached -- Adjusted code comment to survive against pgindent. That's not the right fix, or at least it's not complete. You shouldn't call PG_GETARG_...(n) until you've verified that PG_ARGISNULL(n) returns false. Also, I don't think moving the heap_open() earlier helps anything, but you do need to replace Assert(key->partnatts == nkeys) with an ereport() -- or just return false, but I think ereport() is probably better. Otherwise someone calling satisfies_hash_function() with a wrong number of arguments for the partitioned table can cause an assertion failure, which is bad. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [COMMITTERS] pgsql: Add hash partitioning.
Michael Paquier <michael.paquier@gmail.com> — 2017-11-14T00:32:51Z
On Tue, Nov 14, 2017 at 3:59 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Nov 13, 2017 at 3:24 AM, amul sul <sulamul@gmail.com> wrote: >> Updated patch attached -- Adjusted code comment to survive against pgindent. > > That's not the right fix, or at least it's not complete. You > shouldn't call PG_GETARG_...(n) until you've verified that > PG_ARGISNULL(n) returns false. > > Also, I don't think moving the heap_open() earlier helps anything, but > you do need to replace Assert(key->partnatts == nkeys) with an > ereport() -- or just return false, but I think ereport() is probably > better. Otherwise someone calling satisfies_hash_function() with a > wrong number of arguments for the partitioned table can cause an > assertion failure, which is bad. Yeah, this patch needs more work. There is no need to do much efforts on HEAD to crash it: =# create table aa (a int); CREATE TABLE =# select satisfies_hash_partition('aa'::regclass, 0, NULL, 'po'); server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. Could you add regression tests calling directly this function? elog() can also be triggered easily, which should not happen with user-callable functions: =# select satisfies_hash_partition(0, 0, NULL, 'po'); ERROR: XX000: could not open relation with OID 0 Thanks, -- Michael -
Re: [COMMITTERS] pgsql: Add hash partitioning.
Amul Sul <sulamul@gmail.com> — 2017-11-16T14:37:07Z
Thanks, Michael & Robert for your suggestions, and apologize for delayed response On Tue, Nov 14, 2017 at 6:02 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Nov 14, 2017 at 3:59 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Mon, Nov 13, 2017 at 3:24 AM, amul sul <sulamul@gmail.com> wrote: >>> Updated patch attached -- Adjusted code comment to survive against pgindent. >> >> That's not the right fix, or at least it's not complete. You >> shouldn't call PG_GETARG_...(n) until you've verified that >> PG_ARGISNULL(n) returns false. >> >> Also, I don't think moving the heap_open() earlier helps anything, but >> you do need to replace Assert(key->partnatts == nkeys) with an >> ereport() -- or just return false, but I think ereport() is probably >> better. Otherwise someone calling satisfies_hash_function() with a >> wrong number of arguments for the partitioned table can cause an >> assertion failure, which is bad. > Understood, fixed in the 001 patch. > Yeah, this patch needs more work. There is no need to do much efforts > on HEAD to crash it: > =# create table aa (a int); > CREATE TABLE > =# select satisfies_hash_partition('aa'::regclass, 0, NULL, 'po'); > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > Fixed in the 001 patch. > Could you add regression tests calling directly this function? > Yes sure, but I am not sure that we really need this and also not sure about which regression file is well suitable for these tests (to be honest, I haven't browsed regression directory in detail due to lack of time today), so created new file in 002 patch which is WIP. Do let me know your thoughts will try to improve 0002 patch, tomorrow. > elog() can also be triggered easily, which should not happen with > user-callable functions: > =# select satisfies_hash_partition(0, 0, NULL, 'po'); > ERROR: XX000: could not open relation with OID 0 > Fixed in the 001 patch. IMHO, this function is not meant for a user, so that instead of ereport() cant we simply return false? TWIW, I have attached 003 patch which replaces all erepots() by return false. Regards, Amul Sul -
Re: [COMMITTERS] pgsql: Add hash partitioning.
Robert Haas <robertmhaas@gmail.com> — 2017-11-17T19:49:23Z
On Thu, Nov 16, 2017 at 9:37 AM, amul sul <sulamul@gmail.com> wrote: > Fixed in the 001 patch. > > IMHO, this function is not meant for a user, so that instead of ereport() cant > we simply return false? TWIW, I have attached 003 patch which replaces all > erepots() by return false. I don't think just returning false is very helpful behavior, because the user may not realize that the problem is that the function is being called incorrectly rather than that the call is correct and the answer is false. I took your 0001 patch and made extensive modifications to it. I replaced your regression tests from 0002 with a new set that I wrote myself. The result is attached here. This version makes different decisions about how to handle the various problem cases than you did; it returns NULL for a NULL input or an OID for which no relation exists, and throws specific error messages for the other cases, matching the parser error messages that CREATE TABLE would issue where possible, but with a different error code. It also checks that the types match (which your patch did not, and which I'm fairly sure could crash the server), makes the function work when invoked using the explicit VARIADIC syntax (which seems fairly useless here but there's no in-core precedent for variadic function which doesn't support that case), and fixes the function header comment to describe the behavior we committed rather than the older behavior you had in earlier patch versions. As far as I can tell, this should nail things down pretty tight, but it would be great if someone can try to find a case where it still breaks. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [COMMITTERS] pgsql: Add hash partitioning.
Amul Sul <sulamul@gmail.com> — 2017-11-20T12:46:51Z
On Sat, Nov 18, 2017 at 1:19 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Nov 16, 2017 at 9:37 AM, amul sul <sulamul@gmail.com> wrote: >> Fixed in the 001 patch. >> >> IMHO, this function is not meant for a user, so that instead of ereport() cant >> we simply return false? TWIW, I have attached 003 patch which replaces all >> erepots() by return false. > > I don't think just returning false is very helpful behavior, because > the user may not realize that the problem is that the function is > being called incorrectly rather than that the call is correct and the > answer is false. > > I took your 0001 patch and made extensive modifications to it. I > replaced your regression tests from 0002 with a new set that I wrote > myself. The result is attached here. This version makes different > decisions about how to handle the various problem cases than you did; > it returns NULL for a NULL input or an OID for which no relation > exists, and throws specific error messages for the other cases, > matching the parser error messages that CREATE TABLE would issue where > possible, but with a different error code. It also checks that the > types match (which your patch did not, and which I'm fairly sure could > crash the server), makes the function work when invoked using the > explicit VARIADIC syntax (which seems fairly useless here but there's > no in-core precedent for variadic function which doesn't support that > case), and fixes the function header comment to describe the behavior > we committed rather than the older behavior you had in earlier patch > versions. > > As far as I can tell, this should nail things down pretty tight, but > it would be great if someone can try to find a case where it still > breaks. > Thanks for fixing this function. Patch looks good to me, except column number in the following errors message should to be 2. 354 +SELECT satisfies_hash_partition('mchash'::regclass, 2, 1, NULL::int, NULL::int); 355 +ERROR: column 1 of the partition key has type "text", but supplied value is of type "integer" Same at the line # 374 & 401 in the patch. Regards, Amul -
Re: [COMMITTERS] pgsql: Add hash partitioning.
Robert Haas <robertmhaas@gmail.com> — 2017-11-21T18:09:58Z
On Mon, Nov 20, 2017 at 7:46 AM, amul sul <sulamul@gmail.com> wrote: > Thanks for fixing this function. Patch looks good to me, except column number > in the following errors message should to be 2. > > 354 +SELECT satisfies_hash_partition('mchash'::regclass, 2, 1, > NULL::int, NULL::int); > 355 +ERROR: column 1 of the partition key has type "text", but > supplied value is of type "integer" > > Same at the line # 374 & 401 in the patch. Oops. Looks like the indexing should be 1-based rather than 0-based. Committed with that change. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company