Re: [HACKERS] multi-column range partition constraint
On 2017/05/14 1:07, Robert Haas wrote: > On Sat, May 13, 2017 at 12:42 AM, Amit Langote > wrote: >> Attached is the correct version. > > Thank you! I committed 0001 with a couple of cosmetic tweaks and with > the change I previously suggested around partexprs_item. You argued > that wouldn't work because the contents of partexprs_item was > modified, but that's not so: partexprs_item in > get_range_key_properties is a pointer the partexprs_item in the > caller. When it modifies *partexprs_item, it's not changing the > contents of the ListCell itself, just the caller's ListCell *. I see. > I also ran pgindent over the patch. Oops, had forgotten about pgindent. > Also committed 0002. In that case, I removed CHECK (...) from the > output; the caller can always add that if it's desired, but since a > partitioning constraint is NOT a CHECK constraint I don't actually > think it's useful in most cases. I also tweaked the regression tests > slightly. Thanks for reviewing and committing. Agree about not including CHECK (). Regards, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] multi-column range partition constraint
On Sat, May 13, 2017 at 12:42 AM, Amit Langote wrote: > Attached is the correct version. Thank you! I committed 0001 with a couple of cosmetic tweaks and with the change I previously suggested around partexprs_item. You argued that wouldn't work because the contents of partexprs_item was modified, but that's not so: partexprs_item in get_range_key_properties is a pointer the partexprs_item in the caller. When it modifies *partexprs_item, it's not changing the contents of the ListCell itself, just the caller's ListCell *. I also ran pgindent over the patch. Also committed 0002. In that case, I removed CHECK (...) from the output; the caller can always add that if it's desired, but since a partitioning constraint is NOT a CHECK constraint I don't actually think it's useful in most cases. I also tweaked the regression tests slightly. Thanks again. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] multi-column range partition constraint
On Sat, May 13, 2017 at 11:01 AM, Robert Haas wrote: > On Fri, May 12, 2017 at 12:46 PM, Robert Haas wrote: >> On Fri, May 12, 2017 at 3:26 AM, Amit Langote >> wrote: >>> Fixed. >> >> This seems to be the same patch version as last time, so none of the >> things you mention as fixed are, in fact, fixed. Really sorry about the mix-up. > We are kind of running out of time here before beta1, but Amit thinks > he can get the correct patch posted within about 24 hours, so I'm > going to wait for that to happen rather than trying to revise his > previous version myself. Hence, next update tomorrow. Argh. Attached is the correct version. Thanks, Amit 0001-Emit-correct-range-partition-constraint-expression.patch Description: Binary data 0002-Add-pg_get_partition_constraintdef.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] multi-column range partition constraint
On Fri, May 12, 2017 at 12:46 PM, Robert Haas wrote: > On Fri, May 12, 2017 at 3:26 AM, Amit Langote > wrote: >> Fixed. > > This seems to be the same patch version as last time, so none of the > things you mention as fixed are, in fact, fixed. We are kind of running out of time here before beta1, but Amit thinks he can get the correct patch posted within about 24 hours, so I'm going to wait for that to happen rather than trying to revise his previous version myself. Hence, next update tomorrow. Argh. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] multi-column range partition constraint
On Fri, May 12, 2017 at 3:26 AM, Amit Langote wrote: > Fixed. This seems to be the same patch version as last time, so none of the things you mention as fixed are, in fact, fixed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] multi-column range partition constraint
On 2017/05/12 12:22, Robert Haas wrote: > On Wed, May 10, 2017 at 10:21 PM, Amit Langote > wrote: >>> Next update on this issue by Thursday 5/11. >> >> Attached updated patches. > > Thanks. 0001, at least, really needs a pgindent run. Also, my > compiler has this apparently-justifiable complaint: > > partition.c:1767:5: error: variable 'cur_op_intp' is used uninitialized > whenever > 'for' loop exits because its condition is false > [-Werror,-Wsometimes-uninitialized] > foreach(opic, op_infos) > ^~~ > ../../../src/include/nodes/pg_list.h:162:30: note: expanded from macro > 'foreach' > for ((cell) = list_head(l); (cell) != NULL; (cell) = lnext(cell)) > ^~ > > If by some mischance the condition intp->opfamily_id == > key->partopfamily[l - 1] is never satisfied, this is going to seg > fault, which isn't good. It's pretty easy to imagine how this could > be caused by corrupted system catalog contents or some other bug, so I > think you should initialize the variable to NULL and elog() if it's > still NULL when the loop exits. There is a similar problem in one > other place. > > But actually, I think maybe this logic should just go away altogether, > because backtracking when we realize that an unbounded bound follows > is pretty ugly. Can't we just fix the loop that precedes it so that it > treats next-bound-unbounded the same as this-is-the-last-bound (i.e. > when it is not the case that j - i < num_or_arms)? I think your next-bound-unbounded same as this-is-the-last-bound idea is better. So, done that way. > > +if (partexprs_item) > +partexprs_item_saved = *partexprs_item; > > Is there a reason why you're saving the contents of the ListCell > instead of just a pointer to it? That's because get_range_key_properties() modifies what partexprs_item points to. If we had saved the pointer partexprs_item in partexpr_item_saved, the latter will start pointing to the next cell too upon return from that function, whereas we would want it to keep pointing to the cell that partexprs_item originally pointed to. Am I missing something? > If key->partexprs == NIL, > partexprs_item_saved will not get initialized, but the next loop will > still point to it. That's dangerously close to a live bug, and at any > rate a compiler warning seems likely. Fixed. > I don't really understand the motivation behind the > nulltest_generated[] array. In the upper loop, we'll use any given > value of i in only one loop iteration, so having logic to prevent a > nulltest more than once doesn't seem like it will ever actually do > anything. In the lower loop, it's actually doing something, but if > (num_or_arms == 0) /* Only do this the first time through */ would > have the same effect, I think. Actually, I modified things so that neither of the two loops generate any NullTests. In fact, now they are generated for all the expressions even before the first loop begins. So any required IS NOT NULL expressions appear at the head of the result list. > I suggest num_or_arms -> current_or_arm and maxnum_or_arms -> num_or_arms. Done. > The for_both_cell loop seems to have two different exit conditions: > either we can run off the lists, or we can find j - i sufficiently > large. But shouldn't those things happen at the same time? They will happen at the same time only when current_or_arm (after renaming from num_or_arms per above comment) becomes same as the number of columns we are building the OR expression for. For example, for a partition key (a, b, c) and bounds from (1, 1, 1) to (1, 10, 10), we will be building the OR expressions over b and c. The first iteration of the outer while loop (current_or_arm = 0) only builds b > 1 and b < 10 and exits due to j-i becoming sufficiently large, even though for_both_cell itself didn't run off the lists. In the next iteration of the while loop (current_or_arm = 1), we will consider both b and c and (j-i)'s becoming sufficiently large will happen at the same time as for_both_cell's running off the lists. > + * If both lower_or_arms and upper_or_arms are empty, we append a > + * constant-true expression. That happens if all of the literal values > in > + * both the lower and upper bound lists are UNBOUNDED. > */ > -if (!OidIsValid(operoid)) > +if (lower_or_arms == NIL && upper_or_arms == NIL) > +result = lappend(result, makeBoolConst(true, false)); > > I think it would be better to instead say, at the very end after all > else is done: > > if (result == NIL) > result = make_list1(makeBoolConst(true, false)); This makes sense. So, I guess if there are any IS NOT NULL expressions in the list, we don't need to add the constant-true predicate. > > Next update by tomorrow. Attached updated patches. Thanks, Amit >From 17ae801b3f3fa5e89ab9f2332a825382281522ad Mon
Re: [HACKERS] multi-column range partition constraint
On Wed, May 10, 2017 at 10:21 PM, Amit Langote wrote: >> Next update on this issue by Thursday 5/11. > > Attached updated patches. Thanks. 0001, at least, really needs a pgindent run. Also, my compiler has this apparently-justifiable complaint: partition.c:1767:5: error: variable 'cur_op_intp' is used uninitialized whenever 'for' loop exits because its condition is false [-Werror,-Wsometimes-uninitialized] foreach(opic, op_infos) ^~~ ../../../src/include/nodes/pg_list.h:162:30: note: expanded from macro 'foreach' for ((cell) = list_head(l); (cell) != NULL; (cell) = lnext(cell)) ^~ If by some mischance the condition intp->opfamily_id == key->partopfamily[l - 1] is never satisfied, this is going to seg fault, which isn't good. It's pretty easy to imagine how this could be caused by corrupted system catalog contents or some other bug, so I think you should initialize the variable to NULL and elog() if it's still NULL when the loop exits. There is a similar problem in one other place. But actually, I think maybe this logic should just go away altogether, because backtracking when we realize that an unbounded bound follows is pretty ugly. Can't we just fix the loop that precedes it so that it treats next-bound-unbounded the same as this-is-the-last-bound (i.e. when it is not the case that j - i < num_or_arms)? +if (partexprs_item) +partexprs_item_saved = *partexprs_item; Is there a reason why you're saving the contents of the ListCell instead of just a pointer to it? If key->partexprs == NIL, partexprs_item_saved will not get initialized, but the next loop will still point to it. That's dangerously close to a live bug, and at any rate a compiler warning seems likely. I don't really understand the motivation behind the nulltest_generated[] array. In the upper loop, we'll use any given value of i in only one loop iteration, so having logic to prevent a nulltest more than once doesn't seem like it will ever actually do anything. In the lower loop, it's actually doing something, but if (num_or_arms == 0) /* Only do this the first time through */ would have the same effect, I think. I suggest num_or_arms -> current_or_arm and maxnum_or_arms -> num_or_arms. The for_both_cell loop seems to have two different exit conditions: either we can run off the lists, or we can find j - i sufficiently large. But shouldn't those things happen at the same time? + * If both lower_or_arms and upper_or_arms are empty, we append a + * constant-true expression. That happens if all of the literal values in + * both the lower and upper bound lists are UNBOUNDED. */ -if (!OidIsValid(operoid)) +if (lower_or_arms == NIL && upper_or_arms == NIL) +result = lappend(result, makeBoolConst(true, false)); I think it would be better to instead say, at the very end after all else is done: if (result == NIL) result = make_list1(makeBoolConst(true, false)); Next update by tomorrow. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] multi-column range partition constraint
On 2017/05/10 12:08, Robert Haas wrote: > On Mon, May 8, 2017 at 2:59 AM, Amit Langote > wrote: >> Yes, disallowing this in the first place is the best thing to do. >> Attached patch 0001 implements that. With the patch: > > Committed. Thanks. > With regard to 0002, some of the resulting constraints are a bit more > complicated than seems desirable: > > create table foo1 partition of foo for values from (unbounded, > unbounded, unbounded) to (1, unbounded, unbounded); > yields: > Partition constraint: CHECK (((a < 1) OR (a = 1) OR (a = 1))) > > It would be better not to have (a = 1) in there twice, and better > still to have the whole thing as (a <= 1). > > create table foo2 partition of foo for values from (3, 4, 5) to (6, 7, > unbounded); > yields: > Partition constraint: CHECK a > 3) OR ((a = 3) AND (b > 4)) OR ((a > = 3) AND (b = 4) AND (c >= 5))) AND ((a < 6) OR ((a = 6) AND (b < 7)) > OR ((a = 6) AND (b = 7) > > The first half of that (for the lower bound) is of course fine, but > the second half could be written better using <=, like instead of > > ((a = 6) AND (b < 7)) OR ((a = 6) AND (b = 7)) > you could have: > ((a = 6) AND (b <= 7) > > This isn't purely cosmetic because the simpler constraint is probably > noticeably faster to evaluate. I think that makes sense. I modified things such that a simpler constraint expression as you described above results in the presence of UNBOUNDED values. > I think you should have a few test cases like this in the patch - that > is, cases where some but not all bounds are finite. Added tests like this in insert.sql and then in the second patch as well. > >> BTW, is it strange that the newly added pg_get_partition_constraintdef() >> requires the relcache entry to be created for the partition and all of its >> ancestor relations up to the root (I mean the fact that the relcache entry >> needs to be created at all)? I can see only one other function, >> set_relation_column_names(), creating the relcache entry at all. > > I suggest that you display this information only when "verbose" is set > - i.e. \d+ not just \d. I don't intrinsically care think that forcing > the relcache entry to be built here, but note that it means this will > block when the parent is locked. Between that and the fact that this > information will only sometimes be of interest, restricting it to \d+ > seems preferable. OK, done. > Next update on this issue by Thursday 5/11. Attached updated patches. Thanks, Amit >From 17ae801b3f3fa5e89ab9f2332a825382281522ad Mon Sep 17 00:00:00 2001 From: amit Date: Tue, 2 May 2017 11:03:54 +0900 Subject: [PATCH 1/2] Emit "correct" range partition constraint expression Currently emitted expression does not always describe the constraint correctly, especially in the case of multi-column range key. Code surrounding get_qual_for_*() has been rearranged a little to move common code to a couple of new functions. Original issue reported by Olaf Gawenda (olaf...@googlemail.com) --- src/backend/catalog/partition.c | 733 -- src/include/nodes/pg_list.h | 14 + src/test/regress/expected/inherit.out | 90 + src/test/regress/expected/insert.out | 59 +++ src/test/regress/sql/inherit.sql | 18 + src/test/regress/sql/insert.sql | 43 ++ 6 files changed, 748 insertions(+), 209 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 8641ae16a2..b05ffd2d90 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -118,10 +118,19 @@ static int32 qsort_partition_list_value_cmp(const void *a, const void *b, static int32 qsort_partition_rbound_cmp(const void *a, const void *b, void *arg); -static List *get_qual_for_list(PartitionKey key, PartitionBoundSpec *spec); -static List *get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec); static Oid get_partition_operator(PartitionKey key, int col, StrategyNumber strategy, bool *need_relabel); +static Expr* make_partition_op_expr(PartitionKey key, int keynum, + uint16 strategy, Expr *arg1, Expr *arg2); +static void get_range_key_properties(PartitionKey key, int keynum, + PartitionRangeDatum *ldatum, + PartitionRangeDatum *udatum, + ListCell **partexprs_item, + Expr **keyCol, + Const **lower_val, Const **upper_val, + NullTest **nulltest); +static List *get_qual_for_list(PartitionKey key, PartitionBoundSpec *spec); +static List *get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec); static List *generate_partition_qual(Relation rel); static PartitionRangeBound *make_one_range_bound(PartitionKey key, int index, @@ -1146,6 +1155,123 @@ RelationGetPartitionDispatchInfo(Relation rel, int lockmode, /* Module-local functions */ /* + * get_partition_operator + * + * Return oid of the operator of given strategy for a given partition key + * column. + */ +static Oid +get_partition_op
Re: [HACKERS] multi-column range partition constraint
On Mon, May 8, 2017 at 2:59 AM, Amit Langote wrote: > Yes, disallowing this in the first place is the best thing to do. > Attached patch 0001 implements that. With the patch: Committed. With regard to 0002, some of the resulting constraints are a bit more complicated than seems desirable: create table foo1 partition of foo for values from (unbounded, unbounded, unbounded) to (1, unbounded, unbounded); yields: Partition constraint: CHECK (((a < 1) OR (a = 1) OR (a = 1))) It would be better not to have (a = 1) in there twice, and better still to have the whole thing as (a <= 1). create table foo2 partition of foo for values from (3, 4, 5) to (6, 7, unbounded); yields: Partition constraint: CHECK a > 3) OR ((a = 3) AND (b > 4)) OR ((a = 3) AND (b = 4) AND (c >= 5))) AND ((a < 6) OR ((a = 6) AND (b < 7)) OR ((a = 6) AND (b = 7) The first half of that (for the lower bound) is of course fine, but the second half could be written better using <=, like instead of ((a = 6) AND (b < 7)) OR ((a = 6) AND (b = 7)) you could have: ((a = 6) AND (b <= 7) This isn't purely cosmetic because the simpler constraint is probably noticeably faster to evaluate. I think you should have a few test cases like this in the patch - that is, cases where some but not all bounds are finite. > BTW, is it strange that the newly added pg_get_partition_constraintdef() > requires the relcache entry to be created for the partition and all of its > ancestor relations up to the root (I mean the fact that the relcache entry > needs to be created at all)? I can see only one other function, > set_relation_column_names(), creating the relcache entry at all. I suggest that you display this information only when "verbose" is set - i.e. \d+ not just \d. I don't intrinsically care think that forcing the relcache entry to be built here, but note that it means this will block when the parent is locked. Between that and the fact that this information will only sometimes be of interest, restricting it to \d+ seems preferable. Next update on this issue by Thursday 5/11. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] multi-column range partition constraint
On 2017/05/03 6:30, Robert Haas wrote: > On Tue, May 2, 2017 at 2:51 AM, Amit Langote > wrote: >> Per an off-list report from Olaf Gawenda (thanks Olaf), it seems that the >> range partition's constraint is sometimes incorrect, at least in the case >> of multi-column range partitioning. See below: >> >> create table p (a int, b int) partition by range (a, b); >> create table p1 partition of p for values from (1, 1) to (10 ,10); >> create table p2 partition of p for values from (11, 1) to (20, 10); >> >> Perhaps unusual, but it's still a valid definition. Tuple-routing puts >> rows where they belong correctly. >> >> -- ok >> insert into p values (10, 9); >> select tableoid::regclass, * from p; >> tableoid | a | b >> --++--- >> p1 | 10 | 9 >> (1 row) >> >> -- but see this >> select tableoid::regclass, * from p where a = 10; >> tableoid | a | b >> --+---+--- >> (0 rows) >> >> explain select tableoid::regclass, * from p where a = 10; >> QUERY PLAN >> --- >> Result (cost=0.00..0.00 rows=0 width=12) >>One-Time Filter: false >> (2 rows) >> >> -- or this >> insert into p1 values (10, 9); >> ERROR: new row for relation "p1" violates partition constraint >> DETAIL: Failing row contains (10, 9). >> >> This is because of the constraint being generated is not correct in this >> case. p1's constraint is currently: >> >> a >= 1 and a < 10 >> >> where it should really be the following: >> >> (a > 1 OR (a = 1 AND b >= 1)) >> AND >> (a < 10 OR (a = 10 AND b < 10)) >> >> Attached patch rewrites get_qual_for_range() for the same, along with some >> code rearrangement for reuse. I also added some new tests to insert.sql >> and inherit.sql, but wondered (maybe, too late now) whether there should >> really be a declarative_partition.sql for these, moving in some of the old >> tests too. >> >> Adding to the open items list. > > I think there are more problems here. With the patch: > > rhaas=# create table p (a int, b int) partition by range (a, b); > CREATE TABLE > rhaas=# create table p1 partition of p for values from (unbounded,0) > to (unbounded,1); > CREATE TABLE > rhaas=# insert into p1 values (-2,-2); > ERROR: new row for relation "p1" violates partition constraint > DETAIL: Failing row contains (-2, -2). > rhaas=# insert into p1 values (2,2); > ERROR: new row for relation "p1" violates partition constraint > DETAIL: Failing row contains (2, 2). > > Really, the whole CREATE TABLE .. PARTITION statement is meaningless > and should be disallowed, because it's not meaningful to have a > partition bound specification with a non-unbounded value following an > unbounded value. Yes, disallowing this in the first place is the best thing to do. Attached patch 0001 implements that. With the patch: create table p1 partition of p for values from (unbounded,0) to (unbounded,1); ERROR: cannot specify finite value after UNBOUNDED LINE 1: ...able p1 partition of p for values from (unbounded,0) to (unb... ^ > BTW, I think we should also add a function that prints the partition > constraint, and have psql display that in the \d+ output, because > people might need that - e.g. if you want to attach a partition > without having to validate it, you need to be able to apply an > appropriate constraint to it in advance, so you'll want to see what > the existing partition constraints look like. Agree that that would be helpful, so done in the attached 0003. With the patch: create table p (a int, b int) partition by range (a, b); create table p1 partition of p for values from (1, 1) to (10 ,10); create table p2 partition of p for values from (11, 1) to (20, 10); \d p2 Table "public.p2" Column | Type | Collation | Nullable | Default +-+---+--+- a | integer | | not null | b | integer | | not null | Partition of: p FOR VALUES FROM (11, 1) TO (20, 10) Partition constraint: CHECK a > 11) OR ((a = 11) AND (b >= 1))) AND ((a < 20) OR ((a = 20) AND (b < 10) create table p3 (like p); alter table p3 add constraint partcheck check a > 21) OR ((a = 21) AND (b >= 1))) AND ((a < 30) OR ((a = 30) AND (b < 5); alter table p attach partition p3 for values from (21, 1) to (30, 10); INFO: partition constraint for table "p3" is implied by existing constraints ALTER TABLE BTW, is it strange that the newly added pg_get_partition_constraintdef() requires the relcache entry to be created for the partition and all of its ancestor relations up to the root (I mean the fact that the relcache entry needs to be created at all)? I can see only one other function, set_relation_column_names(), creating the relcache entry at all. > While I'm glad we have partitioning has a feature, I'm starting to get > a bit depressed by the number of bugs that are turning up here. This > was committ
Re: [HACKERS] multi-column range partition constraint
On Tue, May 2, 2017 at 2:51 AM, Amit Langote wrote: > Per an off-list report from Olaf Gawenda (thanks Olaf), it seems that the > range partition's constraint is sometimes incorrect, at least in the case > of multi-column range partitioning. See below: > > create table p (a int, b int) partition by range (a, b); > create table p1 partition of p for values from (1, 1) to (10 ,10); > create table p2 partition of p for values from (11, 1) to (20, 10); > > Perhaps unusual, but it's still a valid definition. Tuple-routing puts > rows where they belong correctly. > > -- ok > insert into p values (10, 9); > select tableoid::regclass, * from p; > tableoid | a | b > --++--- > p1 | 10 | 9 > (1 row) > > -- but see this > select tableoid::regclass, * from p where a = 10; > tableoid | a | b > --+---+--- > (0 rows) > > explain select tableoid::regclass, * from p where a = 10; > QUERY PLAN > --- > Result (cost=0.00..0.00 rows=0 width=12) >One-Time Filter: false > (2 rows) > > -- or this > insert into p1 values (10, 9); > ERROR: new row for relation "p1" violates partition constraint > DETAIL: Failing row contains (10, 9). > > This is because of the constraint being generated is not correct in this > case. p1's constraint is currently: > > a >= 1 and a < 10 > > where it should really be the following: > > (a > 1 OR (a = 1 AND b >= 1)) > AND > (a < 10 OR (a = 10 AND b < 10)) > > Attached patch rewrites get_qual_for_range() for the same, along with some > code rearrangement for reuse. I also added some new tests to insert.sql > and inherit.sql, but wondered (maybe, too late now) whether there should > really be a declarative_partition.sql for these, moving in some of the old > tests too. > > Adding to the open items list. I think there are more problems here. With the patch: rhaas=# create table p (a int, b int) partition by range (a, b); CREATE TABLE rhaas=# create table p1 partition of p for values from (unbounded,0) to (unbounded,1); CREATE TABLE rhaas=# insert into p1 values (-2,-2); ERROR: new row for relation "p1" violates partition constraint DETAIL: Failing row contains (-2, -2). rhaas=# insert into p1 values (2,2); ERROR: new row for relation "p1" violates partition constraint DETAIL: Failing row contains (2, 2). Really, the whole CREATE TABLE .. PARTITION statement is meaningless and should be disallowed, because it's not meaningful to have a partition bound specification with a non-unbounded value following an unbounded value. BTW, I think we should also add a function that prints the partition constraint, and have psql display that in the \d+ output, because people might need that - e.g. if you want to attach a partition without having to validate it, you need to be able to apply an appropriate constraint to it in advance, so you'll want to see what the existing partition constraints look like. While I'm glad we have partitioning has a feature, I'm starting to get a bit depressed by the number of bugs that are turning up here. This was committed in early December, and ideally ought to have been stable long before now. Since Amit is back from vacation May 8th, I'll update no later than May 9th. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] multi-column range partition constraint
On Tue, May 2, 2017 at 2:47 PM, Amit Langote wrote: > Hi Beena, > > On 2017/05/02 17:47, Beena Emerson wrote: > > Hello Amit, > > > > On Tue, May 2, 2017 at 12:21 PM, Amit Langote < > langote_amit...@lab.ntt.co.jp > >> wrote: > > > >> Per an off-list report from Olaf Gawenda (thanks Olaf), it seems that > the > >> range partition's constraint is sometimes incorrect, at least in the > case > >> of multi-column range partitioning. See below: > >> > >> create table p (a int, b int) partition by range (a, b); > >> create table p1 partition of p for values from (1, 1) to (10 ,10); > >> create table p2 partition of p for values from (11, 1) to (20, 10); > >> > >> Perhaps unusual, but it's still a valid definition. Tuple-routing puts > >> rows where they belong correctly. > >> > >> -- ok > >> insert into p values (10, 9); > >> select tableoid::regclass, * from p; > >> tableoid | a | b > >> --++--- > >> p1 | 10 | 9 > >> (1 row) > >> > >> -- but see this > >> select tableoid::regclass, * from p where a = 10; > >> tableoid | a | b > >> --+---+--- > >> (0 rows) > >> > >> explain select tableoid::regclass, * from p where a = 10; > >> QUERY PLAN > >> --- > >> Result (cost=0.00..0.00 rows=0 width=12) > >>One-Time Filter: false > >> (2 rows) > >> > >> -- or this > >> insert into p1 values (10, 9); > >> ERROR: new row for relation "p1" violates partition constraint > >> DETAIL: Failing row contains (10, 9). > >> > >> This is because of the constraint being generated is not correct in this > >> case. p1's constraint is currently: > >> > >> a >= 1 and a < 10 > >> > >> where it should really be the following: > >> > >> (a > 1 OR (a = 1 AND b >= 1)) > >> AND > >> (a < 10 OR (a = 10 AND b < 10)) > >> > > > > > > IIUC, when we say range 1 to 10 we allow values from 1 to 9. Here we are > > allowing a=10 be stored in p1 Is it okay? > > Yes it is. Consider that the multi-column range partitioning uses > tuple-comparison logic to determine if a row's partition key is >= > lower_bound and < upper_bound tuple. > > In this case, p1's lower bound is a "tuple" (1, 1) and (10, 10) its upper > bound. Consider an input row with (2, -1) as its partition key. > Tuple-comparison logic puts this row into p1, because: > > select (1, 1) <= (2, -1) and (2, -1) < (10, 10); > ?column? > -- > t > (1 row) > > When performing tuple-comparison with the lower bound, since 2 > 1, the > tuple comparison is done and the whole tuple is concluded to be > (1, 1); > no attention is paid to the second column (or to the fact that -1 not >= > 1). > Now consider an input row with (10, 9) as its partition key, which too > fits in p1, because: > > select (1, 1) <= (10, 9) and (10, 9) < (10, 10); > ?column? > -- > t > (1 row) > > In this case, since 10 = 10, tuple-comparison considers the next column to > determine the result. In this case, since the second column 9 < 10, the > whole tuple is concluded to be < (10, 10). > > However, neither of (1, 0), (10, 10), or (10, 11) is admissible into p1 by > the above comparison logic: > > select (1, 1) <= (1, 0) and (1, 0) < (10, 10); > ?column? > -- > f > (1 row) > > select (1, 1) <= (10, 10) and (10, 10) < (10, 10); > ?column? > -- > f > (1 row) > > select (1, 1) <= (10, 11) and (10, 11) < (10, 10); > ?column? > -- > f > (1 row) > > I havent been following these partition mails much. Sorry if I am missing > > something obvious. > > No problem. > I have recently started looking at partition. I was wondering why 2nd column is ignored and the exceptions. For following partitions: create table p1 partition of p for values from (1, 1) to (10 ,10); create table p2 partition of p for values from (11, 1) to (20, 10); IIUC, we can store values a = 1 to 9 , 11 to 19 and any value in b. But can store 10 and 20 only when b <=9. This still seems a bit confusing to me but thank you for your explanation. These are just rules u have to abide by I guess! > > >> Attached patch rewrites get_qual_for_range() for the same, along with > some > >> code rearrangement for reuse. I also added some new tests to insert.sql > >> and inherit.sql, but wondered (maybe, too late now) whether there should > >> really be a declarative_partition.sql for these, moving in some of the > old > >> tests too. > >> > > I got the following warning on compiling: > > partition.c: In function ‘make_partition_op_expr’: > > partition.c:1267:2: warning: ‘result’ may be used uninitialized in this > > function [-Wmaybe-uninitialized] > > return result; > > Oops, fixed. Updated patch attached. > > Thank you, -- Beena Emerson EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] multi-column range partition constraint
Hi Beena, On 2017/05/02 17:47, Beena Emerson wrote: > Hello Amit, > > On Tue, May 2, 2017 at 12:21 PM, Amit Langote > wrote: > >> Per an off-list report from Olaf Gawenda (thanks Olaf), it seems that the >> range partition's constraint is sometimes incorrect, at least in the case >> of multi-column range partitioning. See below: >> >> create table p (a int, b int) partition by range (a, b); >> create table p1 partition of p for values from (1, 1) to (10 ,10); >> create table p2 partition of p for values from (11, 1) to (20, 10); >> >> Perhaps unusual, but it's still a valid definition. Tuple-routing puts >> rows where they belong correctly. >> >> -- ok >> insert into p values (10, 9); >> select tableoid::regclass, * from p; >> tableoid | a | b >> --++--- >> p1 | 10 | 9 >> (1 row) >> >> -- but see this >> select tableoid::regclass, * from p where a = 10; >> tableoid | a | b >> --+---+--- >> (0 rows) >> >> explain select tableoid::regclass, * from p where a = 10; >> QUERY PLAN >> --- >> Result (cost=0.00..0.00 rows=0 width=12) >>One-Time Filter: false >> (2 rows) >> >> -- or this >> insert into p1 values (10, 9); >> ERROR: new row for relation "p1" violates partition constraint >> DETAIL: Failing row contains (10, 9). >> >> This is because of the constraint being generated is not correct in this >> case. p1's constraint is currently: >> >> a >= 1 and a < 10 >> >> where it should really be the following: >> >> (a > 1 OR (a = 1 AND b >= 1)) >> AND >> (a < 10 OR (a = 10 AND b < 10)) >> > > > IIUC, when we say range 1 to 10 we allow values from 1 to 9. Here we are > allowing a=10 be stored in p1 Is it okay? Yes it is. Consider that the multi-column range partitioning uses tuple-comparison logic to determine if a row's partition key is >= lower_bound and < upper_bound tuple. In this case, p1's lower bound is a "tuple" (1, 1) and (10, 10) its upper bound. Consider an input row with (2, -1) as its partition key. Tuple-comparison logic puts this row into p1, because: select (1, 1) <= (2, -1) and (2, -1) < (10, 10); ?column? -- t (1 row) When performing tuple-comparison with the lower bound, since 2 > 1, the tuple comparison is done and the whole tuple is concluded to be > (1, 1); no attention is paid to the second column (or to the fact that -1 not >= 1). Now consider an input row with (10, 9) as its partition key, which too fits in p1, because: select (1, 1) <= (10, 9) and (10, 9) < (10, 10); ?column? -- t (1 row) In this case, since 10 = 10, tuple-comparison considers the next column to determine the result. In this case, since the second column 9 < 10, the whole tuple is concluded to be < (10, 10). However, neither of (1, 0), (10, 10), or (10, 11) is admissible into p1 by the above comparison logic: select (1, 1) <= (1, 0) and (1, 0) < (10, 10); ?column? -- f (1 row) select (1, 1) <= (10, 10) and (10, 10) < (10, 10); ?column? -- f (1 row) select (1, 1) <= (10, 11) and (10, 11) < (10, 10); ?column? -- f (1 row) > I havent been following these partition mails much. Sorry if I am missing > something obvious. No problem. >> Attached patch rewrites get_qual_for_range() for the same, along with some >> code rearrangement for reuse. I also added some new tests to insert.sql >> and inherit.sql, but wondered (maybe, too late now) whether there should >> really be a declarative_partition.sql for these, moving in some of the old >> tests too. >> > I got the following warning on compiling: > partition.c: In function ‘make_partition_op_expr’: > partition.c:1267:2: warning: ‘result’ may be used uninitialized in this > function [-Wmaybe-uninitialized] > return result; Oops, fixed. Updated patch attached. Thanks, Amit >From d31d6d885a600cafc74b3068388905ae9e635ab0 Mon Sep 17 00:00:00 2001 From: amit Date: Tue, 2 May 2017 11:03:54 +0900 Subject: [PATCH] Emit "correct" range partition constraint expression Currently emitted expression does not always describe the constraint correctly, especially in the case of multi-column range key. Code surrounding get_qual_for_*() has been rearranged a little to move common code to a couple of new functions. Original issue reported by Olaf Gawenda (olaf...@googlemail.com) --- src/backend/catalog/partition.c | 620 ++ src/include/nodes/pg_list.h | 10 + src/test/regress/expected/inherit.out | 90 + src/test/regress/expected/insert.out | 59 src/test/regress/sql/inherit.sql | 18 + src/test/regress/sql/insert.sql | 43 +++ 6 files changed, 619 insertions(+), 221 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index e0d2665a91..25f51e3278 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -118,10 +118,19 @@ static int32 qsort_partition_list_value_cmp(const
Re: [HACKERS] multi-column range partition constraint
Hello Amit, On Tue, May 2, 2017 at 12:21 PM, Amit Langote wrote: > Per an off-list report from Olaf Gawenda (thanks Olaf), it seems that the > range partition's constraint is sometimes incorrect, at least in the case > of multi-column range partitioning. See below: > > create table p (a int, b int) partition by range (a, b); > create table p1 partition of p for values from (1, 1) to (10 ,10); > create table p2 partition of p for values from (11, 1) to (20, 10); > > Perhaps unusual, but it's still a valid definition. Tuple-routing puts > rows where they belong correctly. > > -- ok > insert into p values (10, 9); > select tableoid::regclass, * from p; > tableoid | a | b > --++--- > p1 | 10 | 9 > (1 row) > > -- but see this > select tableoid::regclass, * from p where a = 10; > tableoid | a | b > --+---+--- > (0 rows) > > explain select tableoid::regclass, * from p where a = 10; > QUERY PLAN > --- > Result (cost=0.00..0.00 rows=0 width=12) >One-Time Filter: false > (2 rows) > > -- or this > insert into p1 values (10, 9); > ERROR: new row for relation "p1" violates partition constraint > DETAIL: Failing row contains (10, 9). > > This is because of the constraint being generated is not correct in this > case. p1's constraint is currently: > > a >= 1 and a < 10 > > where it should really be the following: > > (a > 1 OR (a = 1 AND b >= 1)) > AND > (a < 10 OR (a = 10 AND b < 10)) > IIUC, when we say range 1 to 10 we allow values from 1 to 9. Here we are allowing a=10 be stored in p1 Is it okay? I havent been following these partition mails much. Sorry if I am missing something obvious. > > Attached patch rewrites get_qual_for_range() for the same, along with some > code rearrangement for reuse. I also added some new tests to insert.sql > and inherit.sql, but wondered (maybe, too late now) whether there should > really be a declarative_partition.sql for these, moving in some of the old > tests too. > > Adding to the open items list. > > Thanks, > Amit > > PS: due to vacation, I won't be able to reply promptly until Monday 05/08. > > I got the following warning on compiling: partition.c: In function ‘make_partition_op_expr’: partition.c:1267:2: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized] return result; -- Beena Emerson EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company