Boolean partitions syntax

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
73 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

Boolean partitions syntax

Amit Langote-2
Hi.

Horiguchi-san pointed out [1] on a nearby thread that the partitioning
syntax (the FOR VALUES clause) doesn't accept true and false as valid
partition bound datums, which seems to me like an oversight.  Attached a
patch to fix that.

create table bools (a bool) partition by list (a);

Before patch:

create table bools_t partition of bools for values in (true);
ERROR:  syntax error at or near "true"
LINE 1: ...reate table bools_t partition of bools for values in (true);

After:

create table bools_t partition of bools for values in (true);
CREATE TABLE
\d bools_t
              Table "public.bools_t"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | boolean |           |          |
Partition of: bools FOR VALUES IN (true)

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/20171128.203915.26713586.horiguchi.kyotaro%40lab.ntt.co.jp

0001-Allow-Boolean-values-in-partition-FOR-VALUES-clause.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Dilip Kumar-2

On Tue, Dec 12, 2017 at 7:19 AM, Amit Langote <[hidden email]> wrote:
Hi.

Horiguchi-san pointed out [1] on a nearby thread that the partitioning
syntax (the FOR VALUES clause) doesn't accept true and false as valid
partition bound datums, which seems to me like an oversight.  Attached a
patch to fix that.

create table bools (a bool) partition by list (a);

Before patch:

create table bools_t partition of bools for values in (true);
ERROR:  syntax error at or near "true"
LINE 1: ...reate table bools_t partition of bools for values in (true);

After:

create table bools_t partition of bools for values in (true);
CREATE TABLE
\d bools_t
              Table "public.bools_t"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | boolean |           |          |
Partition of: bools FOR VALUES IN (true)

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/20171128.203915.26713586.horiguchi.kyotaro%40lab.ntt.co.jp

+makeBoolAConstNoCast(bool state, int location)
+{
+ A_Const *n = makeNode(A_Const);
+
+ n->val.type = T_String;
+ n->val.val.str = (state ? "t" : "f");
+ n->location = location;
+
+ return (Node *) n;
+}
+

I think we can change makeBoolAConst as below so that we can avoid duplicate code.

static Node *
makeBoolAConst(bool state, int location)
{
Node *n = makeBoolAConstNoCast(state, location);

return makeTypeCast(n, SystemTypeName("bool"), -1);
}

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Amit Langote-2
Hi Dilip.

Thanks for the review.

On 2017/12/12 15:03, Dilip Kumar wrote:

> On Tue, Dec 12, 2017 at 7:19 AM, Amit Langote wrote:
>> Horiguchi-san pointed out [1] on a nearby thread that the partitioning
>> syntax (the FOR VALUES clause) doesn't accept true and false as valid
>> partition bound datums, which seems to me like an oversight.  Attached a
>> patch to fix that.
>>
>> create table bools (a bool) partition by list (a);
>>
>> Before patch:
>>
>> create table bools_t partition of bools for values in (true);
>> ERROR:  syntax error at or near "true"
>> LINE 1: ...reate table bools_t partition of bools for values in (true);
>>
>> After:
>>
>> create table bools_t partition of bools for values in (true);
>> CREATE TABLE
>> \d bools_t
>>               Table "public.bools_t"
>>  Column |  Type   | Collation | Nullable | Default
>> --------+---------+-----------+----------+---------
>>  a      | boolean |           |          |
>> Partition of: bools FOR VALUES IN (true)
>
> +makeBoolAConstNoCast(bool state, int location)
> +{
> + A_Const *n = makeNode(A_Const);
> +
> + n->val.type = T_String;
> + n->val.val.str = (state ? "t" : "f");
> + n->location = location;
> +
> + return (Node *) n;
> +}
> +
>
> I think we can change makeBoolAConst as below so that we can avoid
> duplicate code.
>
> static Node *
> makeBoolAConst(bool state, int location)
> {
> Node *n = makeBoolAConstNoCast(state, location);
>
> return makeTypeCast(n, SystemTypeName("bool"), -1);
> }
Works for me, updated patch attached.

Thanks,
Amit

0001-Allow-Boolean-values-in-partition-FOR-VALUES-clause-v2.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Amit Langote-2
On 2017/12/12 15:35, Amit Langote wrote:
> Works for me, updated patch attached.

Oops, attached the old one with the last email.

Updated one really attached this time.

Thanks,
Amit

0001-Allow-Boolean-values-in-partition-FOR-VALUES-clause-v2.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Ashutosh Bapat
In reply to this post by Amit Langote-2
On Tue, Dec 12, 2017 at 7:19 AM, Amit Langote
<[hidden email]> wrote:

> Hi.
>
> Horiguchi-san pointed out [1] on a nearby thread that the partitioning
> syntax (the FOR VALUES clause) doesn't accept true and false as valid
> partition bound datums, which seems to me like an oversight.  Attached a
> patch to fix that.
>
> create table bools (a bool) partition by list (a);
>
> Before patch:
>
> create table bools_t partition of bools for values in (true);
> ERROR:  syntax error at or near "true"
> LINE 1: ...reate table bools_t partition of bools for values in (true);
>
> After:
>
> create table bools_t partition of bools for values in (true);
> CREATE TABLE
> \d bools_t
>               Table "public.bools_t"
>  Column |  Type   | Collation | Nullable | Default
> --------+---------+-----------+----------+---------
>  a      | boolean |           |          |
> Partition of: bools FOR VALUES IN (true)
>
> Thanks,
> Amit
>
> [1]
> https://www.postgresql.org/message-id/20171128.203915.26713586.horiguchi.kyotaro%40lab.ntt.co.jp

May be you should use opt_boolean_or_string instead of TRUE_P and
FALSE_P. It also supports ON and OFF, which will be bonus.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Amit Langote-2
On 2017/12/12 18:12, Ashutosh Bapat wrote:
> On Tue, Dec 12, 2017 at 7:19 AM, Amit Langote wrote:
>> Horiguchi-san pointed out [1] on a nearby thread that the partitioning
>> syntax (the FOR VALUES clause) doesn't accept true and false as valid
>> partition bound datums, which seems to me like an oversight.  Attached a
>> patch to fix that.
>
> May be you should use opt_boolean_or_string instead of TRUE_P and
> FALSE_P. It also supports ON and OFF, which will be bonus.

Thanks for the suggestion.  I tried that but NonReservedWord_or_Sconst
conflicts with Sconst that partbound_datum itself has a rule for,
resulting in the following error:

gram.y: conflicts: 6 reduce/reduce
gram.y: expected 0 reduce/reduce conflicts
gram.y:2769.25-81: warning: rule useless in parser due to conflicts:
partbound_datum: Sconst

Moreover, it seems like on/off are not being accepted as valid Boolean
values like true/false are.

insert into rp values (true);
INSERT 0 1
insert into rp values (on);
ERROR:  syntax error at or near "on"
LINE 1: insert into rp values (on);
                               ^
What's going on with that?   Maybe on/off values work only with SET
statements?

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Ashutosh Bapat
On Tue, Dec 12, 2017 at 3:13 PM, Amit Langote
<[hidden email]> wrote:

> On 2017/12/12 18:12, Ashutosh Bapat wrote:
>> On Tue, Dec 12, 2017 at 7:19 AM, Amit Langote wrote:
>>> Horiguchi-san pointed out [1] on a nearby thread that the partitioning
>>> syntax (the FOR VALUES clause) doesn't accept true and false as valid
>>> partition bound datums, which seems to me like an oversight.  Attached a
>>> patch to fix that.
>>
>> May be you should use opt_boolean_or_string instead of TRUE_P and
>> FALSE_P. It also supports ON and OFF, which will be bonus.
>
> Thanks for the suggestion.  I tried that but NonReservedWord_or_Sconst
> conflicts with Sconst that partbound_datum itself has a rule for,
> resulting in the following error:
>
> gram.y: conflicts: 6 reduce/reduce
> gram.y: expected 0 reduce/reduce conflicts
> gram.y:2769.25-81: warning: rule useless in parser due to conflicts:
> partbound_datum: Sconst

Probably that would get fixed if you remove Sconst from the
partition_datum and leave NonReservedWord_or_Sconst.

>
> Moreover, it seems like on/off are not being accepted as valid Boolean
> values like true/false are.
>
> insert into rp values (true);
> INSERT 0 1
> insert into rp values (on);
> ERROR:  syntax error at or near "on"
> LINE 1: insert into rp values (on);
>                                ^
> What's going on with that?   Maybe on/off values work only with SET
> statements?


But this is more important observation. Looks like  on/off work with
SET and EXPLAIN only, not in normal SQL. So probably my suggestion was
wrongheaded.


--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Peter Eisentraut-6
In reply to this post by Ashutosh Bapat
On 12/12/17 04:12, Ashutosh Bapat wrote:
> May be you should use opt_boolean_or_string instead of TRUE_P and
> FALSE_P. It also supports ON and OFF, which will be bonus.

But ON and OFF are not valid boolean literals in SQL.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Amit Langote-2
In reply to this post by Amit Langote-2
On 2017/12/12 15:39, Amit Langote wrote:
> On 2017/12/12 15:35, Amit Langote wrote:
>> Works for me, updated patch attached.
>
> Oops, attached the old one with the last email.
>
> Updated one really attached this time.

Added to CF: https://commitfest.postgresql.org/16/1410/

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Mark Dilger-4

> On Dec 12, 2017, at 10:32 PM, Amit Langote <[hidden email]> wrote:
>
> On 2017/12/12 15:39, Amit Langote wrote:
>> On 2017/12/12 15:35, Amit Langote wrote:
>>> Works for me, updated patch attached.
>>
>> Oops, attached the old one with the last email.
>>
>> Updated one really attached this time.
>
> Added to CF: https://commitfest.postgresql.org/16/1410/

This compiles and passes the regression tests for me.

I extended your test a bit to check whether partitions over booleans are useful.
Note specifically the 'explain' output, which does not seem to restrict the scan
to just the relevant partitions.  You could easily argue that this is beyond the scope
of your patch (and therefore not your problem), but I doubt it makes much sense
to have boolean partitions without planner support for skipping partitions like is
done for tables partitioned over other data types.

mark



-- boolean partitions
create table boolspart (a bool, b text) partition by list (a);
create table boolspart_t partition of boolspart for values in (true);
create table boolspart_f partition of boolspart for values in (false);
\d+ boolspart
                                 Table "public.boolspart"
 Column |  Type   | Collation | Nullable | Default | Storage  | Stats target | Description
--------+---------+-----------+----------+---------+----------+--------------+-------------
 a      | boolean |           |          |         | plain    |              |
 b      | text    |           |          |         | extended |              |
Partition key: LIST (a)
Partitions: boolspart_f FOR VALUES IN (false),
            boolspart_t FOR VALUES IN (true)

insert into boolspart (a, b) values (false, 'false');
insert into boolspart (a, b) values (true, 'true');
explain select * from boolspart where a is true;
                             QUERY PLAN                              
---------------------------------------------------------------------
 Append  (cost=0.00..46.60 rows=1330 width=33)
   ->  Seq Scan on boolspart_f  (cost=0.00..23.30 rows=665 width=33)
         Filter: (a IS TRUE)
   ->  Seq Scan on boolspart_t  (cost=0.00..23.30 rows=665 width=33)
         Filter: (a IS TRUE)
(5 rows)

explain select * from boolspart where a is false;
                             QUERY PLAN                              
---------------------------------------------------------------------
 Append  (cost=0.00..46.60 rows=1330 width=33)
   ->  Seq Scan on boolspart_f  (cost=0.00..23.30 rows=665 width=33)
         Filter: (a IS FALSE)
   ->  Seq Scan on boolspart_t  (cost=0.00..23.30 rows=665 width=33)
         Filter: (a IS FALSE)
(5 rows)

drop table boolspart;
create table multiboolspart (a bool, b bool, c bool, d float, e text) partition by range (a, b, c);
create table multiboolspart_fff partition of multiboolspart for values from (minvalue, minvalue, minvalue) to (false, false, false);
create table multiboolspart_fft partition of multiboolspart for values from (false, false, false) to (false, false, true);
create table multiboolspart_ftf partition of multiboolspart for values from (false, false, true) to (false, true, false);
create table multiboolspart_ftt partition of multiboolspart for values from (false, true, false) to (false, true, true);
create table multiboolspart_tff partition of multiboolspart for values from (false, true, true) to (true, false, false);
create table multiboolspart_tft partition of multiboolspart for values from (true, false, false) to (true, false, true);
create table multiboolspart_ttf partition of multiboolspart for values from (true, false, true) to (true, true, false);
create table multiboolspart_ttt partition of multiboolspart for values from (true, true, false) to (true, true, true);
create table multiboolspart_max partition of multiboolspart for values from (true, true, true) to (maxvalue, maxvalue, maxvalue);
\d+ multiboolspart;
                                   Table "public.multiboolspart"
 Column |       Type       | Collation | Nullable | Default | Storage  | Stats target | Description
--------+------------------+-----------+----------+---------+----------+--------------+-------------
 a      | boolean          |           |          |         | plain    |              |
 b      | boolean          |           |          |         | plain    |              |
 c      | boolean          |           |          |         | plain    |              |
 d      | double precision |           |          |         | plain    |              |
 e      | text             |           |          |         | extended |              |
Partition key: RANGE (a, b, c)
Partitions: multiboolspart_fff FOR VALUES FROM (MINVALUE, MINVALUE, MINVALUE) TO (false, false, false),
            multiboolspart_fft FOR VALUES FROM (false, false, false) TO (false, false, true),
            multiboolspart_ftf FOR VALUES FROM (false, false, true) TO (false, true, false),
            multiboolspart_ftt FOR VALUES FROM (false, true, false) TO (false, true, true),
            multiboolspart_max FOR VALUES FROM (true, true, true) TO (MAXVALUE, MAXVALUE, MAXVALUE),
            multiboolspart_tff FOR VALUES FROM (false, true, true) TO (true, false, false),
            multiboolspart_tft FOR VALUES FROM (true, false, false) TO (true, false, true),
            multiboolspart_ttf FOR VALUES FROM (true, false, true) TO (true, true, false),
            multiboolspart_ttt FOR VALUES FROM (true, true, false) TO (true, true, true)

insert into multiboolspart (a, b, c, d, e) values (true, false, true, 1.7, 'hello');
explain select * from multiboolspart where a is true and b is false and c is true;
                                 QUERY PLAN                                
----------------------------------------------------------------------------
 Append  (cost=0.00..150.50 rows=1008 width=43)
   ->  Seq Scan on multiboolspart_fff  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_fft  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_ftf  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_tff  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_tft  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_ttf  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_max  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE))
(15 rows)

explain select * from multiboolspart where a is true and b is true and c is true;
                                 QUERY PLAN                                
----------------------------------------------------------------------------
 Append  (cost=0.00..193.50 rows=1296 width=43)
   ->  Seq Scan on multiboolspart_fff  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_fft  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_ftf  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_ftt  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_tff  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_tft  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_ttf  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_ttt  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE))
   ->  Seq Scan on multiboolspart_max  (cost=0.00..21.50 rows=144 width=43)
         Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE))
(19 rows)

select * from multiboolspart;
 a | b | c |  d  |   e  
---+---+---+-----+-------
 t | f | t | 1.7 | hello
(1 row)

drop table multiboolspart;



Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Amit Langote-2
Hi Mark,

On 2017/12/20 6:46, Mark Dilger wrote:
>> On Dec 12, 2017, at 10:32 PM, Amit Langote <[hidden email]> wrote:
>> Added to CF: https://commitfest.postgresql.org/16/1410/
>
> This compiles and passes the regression tests for me.

Thanks for the review.

> I extended your test a bit to check whether partitions over booleans are useful.
> Note specifically the 'explain' output, which does not seem to restrict the scan
> to just the relevant partitions.  You could easily argue that this is beyond the scope
> of your patch (and therefore not your problem), but I doubt it makes much sense
> to have boolean partitions without planner support for skipping partitions like is
> done for tables partitioned over other data types.

Yeah.  Actually, I'm aware that the planner doesn't work this.  While
constraint exclusion (planner's current method of skipping partitions)
does not work with IS TRUE/FALSE/UNKNOWN clauses, the new partition
pruning patch [1] addresses that.  In fact, I started this thread prompted
by some discussion about Boolean partitions on that thread [2].

That said, someone might argue that we should also fix constraint
exclusion (the current method of partition pruning) so that partition
skipping works correctly for Boolean partitions.

Thanks,
Amit

[1] https://commitfest.postgresql.org/15/1272/

[2]
https://www.postgresql.org/message-id/9b98fc47-34b8-0ab6-27fc-c8a0889f2e5b%40lab.ntt.co.jp


Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Stephen Frost
Greetings Amit,

* Amit Langote ([hidden email]) wrote:
> On 2017/12/20 6:46, Mark Dilger wrote:
> >> On Dec 12, 2017, at 10:32 PM, Amit Langote <[hidden email]> wrote:
> >> Added to CF: https://commitfest.postgresql.org/16/1410/
> >
> > This compiles and passes the regression tests for me.
>
> Thanks for the review.

Still compiles and passes regression tests, which is good.

> > I extended your test a bit to check whether partitions over booleans are useful.
> > Note specifically the 'explain' output, which does not seem to restrict the scan
> > to just the relevant partitions.  You could easily argue that this is beyond the scope
> > of your patch (and therefore not your problem), but I doubt it makes much sense
> > to have boolean partitions without planner support for skipping partitions like is
> > done for tables partitioned over other data types.
>
> Yeah.  Actually, I'm aware that the planner doesn't work this.  While
> constraint exclusion (planner's current method of skipping partitions)
> does not work with IS TRUE/FALSE/UNKNOWN clauses, the new partition
> pruning patch [1] addresses that.  In fact, I started this thread prompted
> by some discussion about Boolean partitions on that thread [2].
>
> That said, someone might argue that we should also fix constraint
> exclusion (the current method of partition pruning) so that partition
> skipping works correctly for Boolean partitions.
For my 2c, at least, I don't think we need to fix constraint exclusion
to work for this case and hopefully we'll get the partition pruning
patch in but I'm not sure that we really need to wait for that either.
Worst case, we can simply document that the planner won't actually
exclude boolean-based partitions in this release and then fix it in the
future.

Looking over this patch, it seems to be in pretty good shape to me
except that I'm not sure why you went with the approach of naming the
function 'NoCast'.  There's a number of other functions just above
makeBoolAConst() that don't include a TypeCast and it seems like having
makeBoolConst() and makeBoolConstCast() would be more in-line with the
existing code (see makeStringConst() and makeStringConstCast() for
example, but also makeIntConst(), makeFloatConst(), et al).  That would
require updating the existing callers that really want a TypeCast result
even though they're calling makeBoolAConst(), but that seems like a good
improvement to be making.

I could see an argument that we should have two patches (one to rename
the existing function, another to add support for boolean) but that's
really up to whatever committer picks this up.  For my 2c, I don't think
it's really necessary to split it into two patches.

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Amit Langote-2
Hi Stephen.

On 2018/01/26 10:16, Stephen Frost wrote:
> * Amit Langote ([hidden email]) wrote:
> Still compiles and passes regression tests, which is good.

Thanks for looking at this.

>>> I extended your test a bit to check whether partitions over booleans are useful.
>>> Note specifically the 'explain' output, which does not seem to restrict the scan
>>> to just the relevant partitions.  You could easily argue that this is beyond the scope
>>> of your patch (and therefore not your problem), but I doubt it makes much sense
>>> to have boolean partitions without planner support for skipping partitions like is
>>> done for tables partitioned over other data types.
>>
>> Yeah.  Actually, I'm aware that the planner doesn't work this.  While
>> constraint exclusion (planner's current method of skipping partitions)
>> does not work with IS TRUE/FALSE/UNKNOWN clauses, the new partition
>> pruning patch [1] addresses that.  In fact, I started this thread prompted
>> by some discussion about Boolean partitions on that thread [2].
>>
>> That said, someone might argue that we should also fix constraint
>> exclusion (the current method of partition pruning) so that partition
>> skipping works correctly for Boolean partitions.
>
> For my 2c, at least, I don't think we need to fix constraint exclusion
> to work for this case and hopefully we'll get the partition pruning
> patch in but I'm not sure that we really need to wait for that either.
> Worst case, we can simply document that the planner won't actually
> exclude boolean-based partitions in this release and then fix it in the
> future.
Yeah, I meant this just as a tiny syntax extension patch.

> Looking over this patch, it seems to be in pretty good shape to me
> except that I'm not sure why you went with the approach of naming the
> function 'NoCast'.  There's a number of other functions just above
> makeBoolAConst() that don't include a TypeCast and it seems like having
> makeBoolConst() and makeBoolConstCast() would be more in-line with the
> existing code (see makeStringConst() and makeStringConstCast() for
> example, but also makeIntConst(), makeFloatConst(), et al).  That would
> require updating the existing callers that really want a TypeCast result
> even though they're calling makeBoolAConst(), but that seems like a good
> improvement to be making.
Agreed, done.

> I could see an argument that we should have two patches (one to rename
> the existing function, another to add support for boolean) but that's
> really up to whatever committer picks this up.  For my 2c, I don't think
> it's really necessary to split it into two patches.

OK, I kept the function name change part with the main patch.

Attached updated patch.

Thanks,
Amit

v3-0001-Allow-Boolean-values-in-partition-FOR-VALUES-clau.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Robert Haas
On Thu, Jan 25, 2018 at 8:44 PM, Amit Langote
<[hidden email]> wrote:
> Attached updated patch.

I wonder if this patch is just parser bloat without any real benefit.
It can't be very common to want to partition on a Boolean column, and
if you do, all this patch does is let you drop the quotes.  That's not
really a big deal, is it?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Stephen Frost
Robert,

* Robert Haas ([hidden email]) wrote:
> On Thu, Jan 25, 2018 at 8:44 PM, Amit Langote
> <[hidden email]> wrote:
> > Attached updated patch.
>
> I wonder if this patch is just parser bloat without any real benefit.
> It can't be very common to want to partition on a Boolean column, and
> if you do, all this patch does is let you drop the quotes.  That's not
> really a big deal, is it?

I've already had two people mention that it'd be neat to have PG support
it, so I don't believe it'd go unused.  As for if we should force people
to use quotes, my vote would be no because we don't require that for
other usage of true/false in the parser and I don't see any reason why
this should be different.

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Robert Haas
On Fri, Jan 26, 2018 at 11:07 AM, Stephen Frost <[hidden email]> wrote:
> I've already had two people mention that it'd be neat to have PG support
> it, so I don't believe it'd go unused.  As for if we should force people
> to use quotes, my vote would be no because we don't require that for
> other usage of true/false in the parser and I don't see any reason why
> this should be different.

OK.  Let's wait a bit and see if anyone else wants to weigh in.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Tom Lane-2
Robert Haas <[hidden email]> writes:
> On Fri, Jan 26, 2018 at 11:07 AM, Stephen Frost <[hidden email]> wrote:
>> I've already had two people mention that it'd be neat to have PG support
>> it, so I don't believe it'd go unused.  As for if we should force people
>> to use quotes, my vote would be no because we don't require that for
>> other usage of true/false in the parser and I don't see any reason why
>> this should be different.

> OK.  Let's wait a bit and see if anyone else wants to weigh in.

I dunno, this patch seems quite bizarre to me.  IIUC, it results in
TRUE/FALSE behaving differently in a partbound_datum than they do
anywhere else in the grammar, to wit that they're effectively just
another spelling for the undecorated literals 't' and 'f', without
anything indicating that they're boolean.  That seems wrong from a
data typing standpoint.  And even if it's really true that we'd
rather lose type information for partbound literals, a naive observer
would probably think that these should decay to the strings "true"
and "false" not "t" and "f" (cf. opt_boolean_or_string).

I've not paid any attention to this thread up to now, so maybe there's
a rational explanation for this choice that I missed.  But mucking
with makeBoolAConst like that doesn't seem to me to pass the smell
test.  I'm on board with the stated goal of allowing TRUE/FALSE here,
but having to contort the grammar output like this suggests that
there's something wrong in parse analysis of partbound_datum.

                        regards, tom lane

PS: the proposed documentation wording is too verbose by half.
I'd just cut it down to "<literal constant>".

Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Amit Langote-2
In reply to this post by Robert Haas
On 2018/01/27 0:30, Robert Haas wrote:
> On Thu, Jan 25, 2018 at 8:44 PM, Amit Langote
> <[hidden email]> wrote:
>> Attached updated patch.
>
> I wonder if this patch is just parser bloat without any real benefit.
> It can't be very common to want to partition on a Boolean column, and
> if you do, all this patch does is let you drop the quotes.  That's not
> really a big deal, is it?

Yeah, maybe it isn't because Boolean partitioning is rarely used, but I
thought it wasn't nice that only the partition bound syntax requires
specifying the quotes around Boolean values.  Apparently others felt the
same, because I only found out about this oversight after someone pointed
it out to me [1].

I agree that the patch is bigger than it had to be, which I have tried to
fix in the patch that I will post in reply to Tom's email on this thread.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/20171128.203915.26713586.horiguchi.kyotaro%40lab.ntt.co.jp


Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Amit Langote-2
In reply to this post by Tom Lane-2
On 2018/01/27 1:31, Tom Lane wrote:

> Robert Haas <[hidden email]> writes:
>> On Fri, Jan 26, 2018 at 11:07 AM, Stephen Frost <[hidden email]> wrote:
>>> I've already had two people mention that it'd be neat to have PG support
>>> it, so I don't believe it'd go unused.  As for if we should force people
>>> to use quotes, my vote would be no because we don't require that for
>>> other usage of true/false in the parser and I don't see any reason why
>>> this should be different.
>
>> OK.  Let's wait a bit and see if anyone else wants to weigh in.
>
> I dunno, this patch seems quite bizarre to me.  IIUC, it results in
> TRUE/FALSE behaving differently in a partbound_datum than they do
> anywhere else in the grammar, to wit that they're effectively just
> another spelling for the undecorated literals 't' and 'f', without
> anything indicating that they're boolean.  That seems wrong from a
> data typing standpoint.  And even if it's really true that we'd
> rather lose type information for partbound literals, a naive observer
> would probably think that these should decay to the strings "true"
> and "false" not "t" and "f" (cf. opt_boolean_or_string).
Partition bound literals as captured gram.y don't have any type
information attached.  They're carried over in a A_Const to
transformPartitionBoundValue() and coerced to the target partition key
type there.  Note that each of the the partition bound literal datums
received from gram.y is castNode(A_Const)'d in parse_utilcmds.c.

I agree that it's better to simply makeStringConst("true"/"false") for
TRUE_P and FALSE_P, instead of makingBoolAConst(true/false).

> I've not paid any attention to this thread up to now, so maybe there's
> a rational explanation for this choice that I missed.  But mucking
> with makeBoolAConst like that doesn't seem to me to pass the smell
> test.  I'm on board with the stated goal of allowing TRUE/FALSE here,
> but having to contort the grammar output like this suggests that
> there's something wrong in parse analysis of partbound_datum.

Attached updated patch doesn't change anything about makeBoolAConst and
now is just a 2-line change to gram.y.

> PS: the proposed documentation wording is too verbose by half.
> I'd just cut it down to "<literal constant>".

Yeah, I was getting nervous about the lines in syntax synopsis getting
unwieldily long after this change.  I changed all of them to use
literal_constant for anything other than special keywords MINVALUE and
MAXVALUE and a paragraph in the description to clarify.

Attached updated patch.  Thanks for the comments.

Regards,
Amit

v4-0001-Allow-Boolean-values-in-partition-FOR-VALUES-clau.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Boolean partitions syntax

Tom Lane-2
Amit Langote <[hidden email]> writes:
> Partition bound literals as captured gram.y don't have any type
> information attached.

Isn't that design broken by definition?  TRUE is not the same thing
as 't', nor as 'true'.  Nor are 1 and '1' the same thing; it's true
that in some contexts we'll let '1' convert to an integer 1, but the
reverse is not true.  Moreover, this approach doesn't have any hope
of ever extending to bound values that aren't bare literals.

I think you are fixing this at the wrong level.  Ideally the bound values
ought to be expressions that get coerced to the partition column type.
It's fine to require them to be constants for now, but not to invent
an off-the-cuff set of syntactic restrictions that substitute for the
semantic notion of "must be a constant".  That path will lead to nasty
backwards compatibility issues whenever somebody tries to extend the
feature.

A concrete example of that is that the code currently accepts:

regression=# create table textpart (a text) partition by list (a);
CREATE TABLE
regression=# create table textpart_t partition of textpart for values in (1);
CREATE TABLE

Since there's no implicit conversion from int to text, this seems
pretty broken to me: there's no way for this behavior to be upward
compatible to an implementation that treats the partition bound
values as anything but text strings.  We should fix that before the
behavior gets completely set in concrete.

                        regards, tom lane

1234