Horton

Thread

Commits

  1. Docs: improve CREATE TABLE ref page's discussion of partition bounds.

  2. Code review focused on new node types added by partitioning support.

  1. BUG #14666: Question on money type as the key of partitioned table

    tianbing@highgo.com — 2017-05-24T02:45:50Z

    The following bug has been logged on the website:

Bug reference:      14666
Logged by:          tian bing
Email address:      tianbing@highgo.com
PostgreSQL version: 10beta1
Operating system:   CentOS 6.4（64bits）
Description:        

Hi,
When I use the money type as the key to create the partition table as
follows:

postgres=# create table test(m money) partition by list(m);
CREATE TABLE
postgres=# create table test_1 partition of test for values in (10);
CREATE TABLE

Partition bounds without apostrophe can be createed, but it store the null
value，not '10' value.

In fact, Correct grammar for creating partition is with apostrophe like
this:
postgres=# create table test_1 partition of test for values in ('10');

But the first creating partition without apostrophe should report an error
like "ERROR:  operator does not exist: money = integer"  as adding a check
constraint.

Looking forward to your reply.

  2. Re: BUG #14666: Question on money type as the key of partitioned table

    Michael Paquier <michael.paquier@gmail.com> — 2017-05-24T14:54:30Z

    On Tue, May 23, 2017 at 10:45 PM,  <tianbing@highgo.com> wrote:
> When I use the money type as the key to create the partition table as
> follows:
>
> postgres=# create table test(m money) partition by list(m);
> CREATE TABLE
> postgres=# create table test_1 partition of test for values in (10);
> CREATE TABLE
>
> Partition bounds without apostrophe can be created, but it store the null
> value，not '10' value.
>
> In fact, Correct grammar for creating partition is with apostrophe like
> this:
> postgres=# create table test_1 partition of test for values in ('10');
>
> But the first creating partition without apostrophe should report an error
> like "ERROR:  operator does not exist: money = integer"  as adding a check
> constraint.
>
> Looking forward to your reply.

This looks like a justified complain to me, so added to the open item
list. Those INSERTs should work:
=# insert into test_1 values (10);
ERROR:  23514: new row for relation "test_1" violates partition constraint
DETAIL:  Failing row contains ($10.00).
LOCATION:  ExecConstraints, execMain.c:2037
=# insert into test values (10);
ERROR:  23514: no partition of relation "test" found for row
DETAIL:  Partition key of the failing row contains (m) = ($10.00).
LOCATION:  ExecFindPartition, execMain.c:3343
=# insert into test values ('10');
ERROR:  23514: no partition of relation "test" found for row
DETAIL:  Partition key of the failing row contains (m) = ($10.00).
LOCATION:  ExecFindPartition, execMain.c:3343
The shape of the partition definition looks broken.
-- 
Michael

  3. Re: BUG #14666: Question on money type as the key of partitioned table

    Amit Langote <amitlangote09@gmail.com> — 2017-05-24T20:22:16Z

    On Wed, May 24, 2017 at 10:54 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, May 23, 2017 at 10:45 PM,  <tianbing@highgo.com> wrote:
>> When I use the money type as the key to create the partition table as
>> follows:
>>
>> postgres=# create table test(m money) partition by list(m);
>> CREATE TABLE
>> postgres=# create table test_1 partition of test for values in (10);
>> CREATE TABLE
>>
>> Partition bounds without apostrophe can be created, but it store the null
>> value，not '10' value.
>>
>> In fact, Correct grammar for creating partition is with apostrophe like
>> this:
>> postgres=# create table test_1 partition of test for values in ('10');
>>
>> But the first creating partition without apostrophe should report an error
>> like "ERROR:  operator does not exist: money = integer"  as adding a check
>> constraint.
>>
>> Looking forward to your reply.
>
> This looks like a justified complain to me, so added to the open item
> list. Those INSERTs should work:
> =# insert into test_1 values (10);
> ERROR:  23514: new row for relation "test_1" violates partition constraint
> DETAIL:  Failing row contains ($10.00).
> LOCATION:  ExecConstraints, execMain.c:2037
> =# insert into test values (10);
> ERROR:  23514: no partition of relation "test" found for row
> DETAIL:  Partition key of the failing row contains (m) = ($10.00).
> LOCATION:  ExecFindPartition, execMain.c:3343
> =# insert into test values ('10');
> ERROR:  23514: no partition of relation "test" found for row
> DETAIL:  Partition key of the failing row contains (m) = ($10.00).
> LOCATION:  ExecFindPartition, execMain.c:3343
> The shape of the partition definition looks broken.

Yep, looks like a bug; will look into it.

Thanks for adding to the open items list.

Regards,
Amit

  4. Re: BUG #14666: Question on money type as the key of partitioned table

    Michael Paquier <michael.paquier@gmail.com> — 2017-05-26T20:29:06Z

    On Wed, May 24, 2017 at 4:22 PM, Amit Langote <amitlangote09@gmail.com> wrote:
> Yep, looks like a bug; will look into it.

Cool.

Just mentioning... I found surprising to see that failing with a
partition error:
=# create table test(m money) partition by list(m);
CREATE TABLE
=#  create table test_1 partition of test for values in (10::money);
ERROR:  42601: syntax error at or near "::"
Likely the price to pay to get a minimal feature in 10.
-- 
Michael

  5. Re: BUG #14666: Question on money type as the key of partitioned table

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-28T21:04:25Z

    Michael Paquier <michael.paquier@gmail.com> writes:
> Just mentioning... I found surprising to see that failing with a
> partition error:
> =# create table test(m money) partition by list(m);
> CREATE TABLE
> =#  create table test_1 partition of test for values in (10::money);
> ERROR:  42601: syntax error at or near "::"

Well, this is a misunderstanding of the syntax: the values in the
values list have to be simple literals, not expressions.

However, it's arguably not your fault, because the CREATE TABLE
reference page says that the list elements are "bound_literal"s,
a term which AFAICS is defined nowhere.  I'm inclined to change
that to something like

	IN ( { numeric_literal | string_literal | NULL } [, ...] ) 

which seems less likely to leave the reader wondering what is meant.

			regards, tom lane

  6. Re: BUG #14666: Question on money type as the key of partitioned table

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-28T21:53:29Z

    tianbing@highgo.com writes:
> When I use the money type as the key to create the partition table as
> follows:

> postgres=# create table test(m money) partition by list(m);
> CREATE TABLE
> postgres=# create table test_1 partition of test for values in (10);
> CREATE TABLE

> Partition bounds without apostrophe can be createed, but it store the null
> value, not '10' value.

That's not actually what it's doing.  A look into pg_class shows that
while, for an integer partitioning column, you'd get something like this
for relpartbound:

 test1p                              | {PARTITIONBOUND :strategy l :listdatums (
{CONST :consttype 23 :consttypmod -1 :constcollid 0 :constlen 4 :constbyval true
 :constisnull false :location 54 :constvalue 4 [ 10 0 0 0 0 0 0 0 ]}) :lowerdatu
ms <> :upperdatums <>}

in the case at hand you'd get

 test2p                              | {PARTITIONBOUND :strategy l :listdatums (
{FUNCEXPR :funcid 3811 :funcresulttype 790 :funcretset false :funcvariadic false
 :funcformat 2 :funccollid 0 :inputcollid 0 :args ({CONST :consttype 23 :constty
pmod -1 :constcollid 0 :constlen 4 :constbyval true :constisnull false :location
 54 :constvalue 4 [ 10 0 0 0 0 0 0 0 ]}) :location -1}) :lowerdatums <> :upperda
tums <>}

that is, what we have is a run-time coercion of integer to money.
The partitioning code utterly fails to consider that what it might
get from the partition list syntax is not a constant --- but since
casts are not required to be immutable, it might not.

This is exacerbated by the fact that subsequent code naively assumes
that the elements of PartitionBoundSpec.listdatums are Consts, without
any checking.  It's a wonder you don't get runtime crashes.  (You might
if the partition column type is pass-by-ref, I suspect.)  And I'm
unimpressed by the fact that this assumption is nowhere documented, too.

What we need to do here (at least in the short term) is throw an error
if we don't get a simple Const out of const-simplification.  I'm not
sure if we need a separate error message for that case, or if we can
get away with just re-using the existing text about "specified value
cannot be cast to type ...".  The point here would be that the cast
exists but is not immutable.  Maybe use the same primary message
but explain that in an errdetail?

			regards, tom lane

  7. Re: BUG #14666: Question on money type as the key of partitioned table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-05-29T07:20:03Z

    Thanks a lot for getting to fixing this bug before I did.

On 2017/05/29 6:53, Tom Lane wrote:
> tianbing@highgo.com writes:
>> When I use the money type as the key to create the partition table as
>> follows:
> 
>> postgres=# create table test(m money) partition by list(m);
>> CREATE TABLE
>> postgres=# create table test_1 partition of test for values in (10);
>> CREATE TABLE
> 
>> Partition bounds without apostrophe can be createed, but it store the null
>> value, not '10' value.
> 
> That's not actually what it's doing.  A look into pg_class shows that
> while, for an integer partitioning column, you'd get something like this
> for relpartbound:
> 
>  test1p                              | {PARTITIONBOUND :strategy l :listdatums (
> {CONST :consttype 23 :consttypmod -1 :constcollid 0 :constlen 4 :constbyval true
>  :constisnull false :location 54 :constvalue 4 [ 10 0 0 0 0 0 0 0 ]}) :lowerdatu
> ms <> :upperdatums <>}
> 
> in the case at hand you'd get
> 
>  test2p                              | {PARTITIONBOUND :strategy l :listdatums (
> {FUNCEXPR :funcid 3811 :funcresulttype 790 :funcretset false :funcvariadic false
>  :funcformat 2 :funccollid 0 :inputcollid 0 :args ({CONST :consttype 23 :constty
> pmod -1 :constcollid 0 :constlen 4 :constbyval true :constisnull false :location
>  54 :constvalue 4 [ 10 0 0 0 0 0 0 0 ]}) :location -1}) :lowerdatums <> :upperda
> tums <>}
> 
> that is, what we have is a run-time coercion of integer to money.
> The partitioning code utterly fails to consider that what it might
> get from the partition list syntax is not a constant --- but since
> casts are not required to be immutable, it might not.

Oops, that's right.  I had failed to consider that the cast step might
introduce a coercion function call.

> This is exacerbated by the fact that subsequent code naively assumes
> that the elements of PartitionBoundSpec.listdatums are Consts, without
> any checking.  It's a wonder you don't get runtime crashes.  (You might
> if the partition column type is pass-by-ref, I suspect.)  And I'm
> unimpressed by the fact that this assumption is nowhere documented, too.
> 
> What we need to do here (at least in the short term) is throw an error
> if we don't get a simple Const out of const-simplification.  I'm not
> sure if we need a separate error message for that case, or if we can
> get away with just re-using the existing text about "specified value
> cannot be cast to type ...".  The point here would be that the cast
> exists but is not immutable.  Maybe use the same primary message
> but explain that in an errdetail?

The message as committed in 76a3df6e5e621928fbf0cddf347e16a62e9433ec looks
on point to me.

Thanks again.

Regards,
Amit