Обсуждение: [HACKERS] multi-column range partition constraint

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.

Thanks,
Amit

PS: due to vacation, I won't be able to reply promptly until Monday 05/08.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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_f8@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.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, May 2, 2017 at 2:51 AM, Amit Langote
<Langote_Amit_f8@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))
>
> 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

On 2017/05/03 6:30, Robert Haas wrote:
> On Tue, May 2, 2017 at 2:51 AM, Amit Langote
> <Langote_Amit_f8@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))
>>
>> 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 committed in early December, and ideally ought to have been stable
> long before now.

I would think so too.  This bug was particularly unsettling for me,
although it is not to say that other bugs that turned up were any less
serious.

> Since Amit is back from vacation May 8th, I'll update no later than May 9th.

Attached updated 0002 and 0001, 0003 as mentioned above.  Thanks a lot for
your patience.

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

On Mon, May 8, 2017 at 2:59 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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

On 2017/05/10 12:08, Robert Haas wrote:
> On Mon, May 8, 2017 at 2:59 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> 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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, May 10, 2017 at 10:21 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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
conditionis 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

On 2017/05/12 12:22, Robert Haas wrote:
> On Wed, May 10, 2017 at 10:21 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> 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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, May 12, 2017 at 3:26 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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

On Fri, May 12, 2017 at 12:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, May 12, 2017 at 3:26 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> 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

On Sat, May 13, 2017 at 11:01 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, May 12, 2017 at 12:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, May 12, 2017 at 3:26 AM, Amit Langote
>> <Langote_Amit_f8@lab.ntt.co.jp> 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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, May 13, 2017 at 12:42 AM, Amit Langote <amitlangote09@gmail.com> 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

On 2017/05/14 1:07, Robert Haas wrote:
> On Sat, May 13, 2017 at 12:42 AM, Amit Langote <amitlangote09@gmail.com> 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