Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

At Tue, 24 Apr 2018 09:23:03 +0900, Amit Langote 
 wrote in 

> On 2018/04/24 4:33, Tom Lane wrote:
> > Amit Langote  writes:
> >> On 2018/04/22 2:29, Tom Lane wrote:
> >>> I propose the attached slightly-less-invasive version of Amit's original
> >>> patch as what we should do in v10 and v11, and push the patch currently
> >>> under discussion out to v12.

Agreed that it is too-much for v10. (I hoped that something more
might be introduced to v11:p)

> >> Here too.
> > 
> > Pushed.  It occurred to me at the last moment that we could partially
> > address one of my original concerns about this hack by converting TRUE
> > and FALSE to strings 'true' and 'false' not just 't' and 'f'.  Those
> > will be accepted by boolin just as well, and doing it like that slightly
> > reduces the astonishment factor if somebody writes TRUE for, say, a
> > text column's partbound.  I'd still prefer that we throw an error for
> > such a case, but that'll have to wait for v12.
> 
> Thanks for making that change and committing.

+1, and thank you for discussing. I'll add this item to the next
CF.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Boolean partitions syntax

[Jonathan S. Katz]

> On Apr 23, 2018, at 12:33 PM, Tom Lane  wrote:
> 
> Amit Langote  writes:
>> On 2018/04/22 2:29, Tom Lane wrote:
>>> I propose the attached slightly-less-invasive version of Amit's original
>>> patch as what we should do in v10 and v11, and push the patch currently
>>> under discussion out to v12.
> 
>> Here too.
> 
> Pushed.  It occurred to me at the last moment that we could partially
> address one of my original concerns about this hack by converting TRUE
> and FALSE to strings 'true' and 'false' not just 't' and 'f’.

I tried  the earlier test case I presented against HEAD and it worked
like a charm. Thank you!

Jonathan

Re: Boolean partitions syntax

[Tom Lane]

Amit Langote  writes:
> On 2018/04/22 2:29, Tom Lane wrote:
>> I propose the attached slightly-less-invasive version of Amit's original
>> patch as what we should do in v10 and v11, and push the patch currently
>> under discussion out to v12.

> Here too.

Pushed.  It occurred to me at the last moment that we could partially
address one of my original concerns about this hack by converting TRUE
and FALSE to strings 'true' and 'false' not just 't' and 'f'.  Those
will be accepted by boolin just as well, and doing it like that slightly
reduces the astonishment factor if somebody writes TRUE for, say, a
text column's partbound.  I'd still prefer that we throw an error for
such a case, but that'll have to wait for v12.

regards, tom lane

Re: Boolean partitions syntax

[Amit Langote]

On 2018/04/22 2:29, Tom Lane wrote:
> Amit Langote  writes:
>> I think if this bug/open item can be resolved by adopting the minimal
>> patch, then we should use it for that.  Maybe, we can discuss the rest of
>> the changes independently.  If they make things better overall, we should
>> definitely try to adopt them.
> 
> Yeah.  While I think that getting rid of the grammar restrictions on what
> a partbound can be is a good idea, it seems like this is not the sort of
> improvement to be making post-feature-freeze.  And it's certainly not
> something to back-patch to v10.

Agreed.

> I propose the attached slightly-less-invasive version of Amit's original
> patch as what we should do in v10 and v11, and push the patch currently
> under discussion out to v12.

Here too.

>> About the changes in transformPartitionBoundValue() to check for collation
>> conflict, I think that seems unnecessary.
> 
> I agree.  We can document that the partbound expression is reduced to a
> simple constant and leave it at that.  Nobody has yet been confused by
> the possibility of putting COLLATE in a default expression, and I don't
> believe that anybody will be confused here.

Yes, I think so.

> (Speaking of documentation, nobody seems to have noticed that
> partition_bound_spec appears in alter_table.sgml too.)

Oops, thanks for fixing that.  Actually, partition_bound_spec wasn't
expanded like it is now in the synopsis of alter_table.sgml at the time
the original patch was written.  Commit a2a22057617 (dated Feb 2) added
it, whereas the patch was posted on Jan 29.

Thanks,
Amit

Re: using expression syntax for partition bounds (was: Re: Boolean partitions syntax)

[Amit Langote]

On 2018/04/23 11:37, Amit Langote wrote:
> I tried to update the patch to do things that way.  I'm going to create a
> new entry in the next CF titled "generalized expression syntax for
> partition bounds" and add the patch there.

Tweaked the commit message to credit all the authors.

Thanks,
Amit
From c3b78623fdd37c785eb676484945d15448357591 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 18 Apr 2018 18:53:44 +0900
Subject: [PATCH v2] Allow generalized expression syntax for partition bounds

This fixes the bug that Boolean literals true/false are not allowed
in partition bound syntax.

Authors: Kyotaro Horiguchi, Tom Lane, Amit Langote
---
 doc/src/sgml/ref/alter_table.sgml  |   6 +-
 doc/src/sgml/ref/create_table.sgml |  14 ++--
 src/backend/optimizer/util/clauses.c   |   4 +-
 src/backend/parser/gram.y  |  58 +
 src/backend/parser/parse_agg.c |  10 +++
 src/backend/parser/parse_expr.c|   5 ++
 src/backend/parser/parse_func.c|   3 +
 src/backend/parser/parse_utilcmd.c | 126 ++---
 src/include/optimizer/clauses.h|   2 +
 src/include/parser/parse_node.h|   1 +
 src/include/utils/partcache.h  |   6 ++
 src/test/regress/expected/create_table.out |  16 +---
 src/test/regress/sql/create_table.sql  |   5 +-
 13 files changed, 142 insertions(+), 114 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml 
b/doc/src/sgml/ref/alter_table.sgml
index 8c7b9619b8..2c11a5ad75 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -87,9 +87,9 @@ ALTER TABLE [ IF EXISTS ] name
 
 and partition_bound_spec 
is:
 
-IN ( { numeric_literal | 
string_literal | NULL } [, ...] ) |
-FROM ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] )
-  TO ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] ) |
+IN ( partition_bound_expr [, ...] 
) |
+FROM ( { partition_bound_expr | 
MINVALUE | MAXVALUE } [, ...] )
+  TO ( { partition_bound_expr | 
MINVALUE | MAXVALUE } [, ...] ) |
 WITH ( MODULUS numeric_literal, 
REMAINDER numeric_literal )
 
 and column_constraint 
is:
diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index f2bd562d55..b7d5562345 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -86,9 +86,9 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] 
TABLE [ IF NOT EXI
 
 and partition_bound_spec 
is:
 
-IN ( { numeric_literal | 
string_literal | NULL } [, ...] ) |
-FROM ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] )
-  TO ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] ) |
+IN ( partition_bound_expr [, ...] 
) |
+FROM ( { partition_bound_expr | 
MINVALUE | MAXVALUE } [, ...] )
+  TO ( { partition_bound_expr | 
MINVALUE | MAXVALUE } [, ...] ) |
 WITH ( MODULUS numeric_literal, 
REMAINDER numeric_literal )
 
 index_parameters in 
UNIQUE, PRIMARY KEY, and 
EXCLUDE constraints are:
@@ -275,10 +275,10 @@ WITH ( MODULUS numeric_literal, REM
  
   Each of the values specified in
   the partition_bound_spec is
-  a literal, NULL, MINVALUE, or
-  MAXVALUE.  Each literal value must be either a
-  numeric constant that is coercible to the corresponding partition key
-  column's type, or a string literal that is valid input for that type.
+  any variable-free expression (subqueries, window functions, aggregate
+  functions, and set-returning functions are not allowed).  The data type
+  of the partition bound expression must match the data type of the
+  corresponding partition key column.
  
 
  
diff --git a/src/backend/optimizer/util/clauses.c 
b/src/backend/optimizer/util/clauses.c
index 505ae0af85..77ebb40ef2 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -150,8 +150,6 @@ static Node *substitute_actual_parameters(Node *expr, int 
nargs, List *args,
 static Node *substitute_actual_parameters_mutator(Node *node,
 
substitute_actual_parameters_context *context);
 static void sql_inline_error_callback(void *arg);
-static Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
- Oid result_collation);
 static Query *substitute_actual_srf_parameters(Query *expr,
 int nargs, 
List *args);
 static Node *substitute_actual_srf_parameters_mutator(Node *node,
@@ -4840,7 +4838,7 @@ sql_inline_error_callback(void *arg)
  * We use the executor's routine ExecEvalExpr() to avoid duplication of
  * code and ensure we get the same result as the executor would get.
  */
-static Expr *
+Expr *
 evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
  Oid 

using expression syntax for partition bounds (was: Re: Boolean partitions syntax)

[Amit Langote]

(patch and discussion for PG 12)

On 2018/04/22 1:28, Tom Lane wrote:
> Amit Langote  writes:
>> [ v8-0001-Allow-generalized-expression-syntax-for-partition.patch ]
>
> I find what you did to a_expr here to be pretty horrid.

Thanks for the review.

> I think what you should do is lose the partbound_datum and
> PartitionRangeDatum productions altogether, replacing those with just
> a_expr, as in the attached grammar-only patch.  This would result in
> needing to identify MINVALUE and MAXVALUE during parse analysis, since
> the grammar would just treat them as ColId expressions.  But since we're
> not intending to ever allow any actual column references in partition
> expressions, I don't see any harm in allowing parse analysis to treat
> ColumnRefs containing those names as meaning the special items.

I have to agree this is better.

> This is a little bit grotty, in that both MINVALUE and "minvalue" would
> be recognized as the keyword, but it's sure a lot less messy than what's
> there now.  And IIRC there are some other places where we're a bit
> squishy about the difference between identifiers and keywords, too.

Hmm, yes.

I tried to update the patch to do things that way.  I'm going to create a
new entry in the next CF titled "generalized expression syntax for
partition bounds" and add the patch there.

Thanks,
Amit
From e82738affc8f3e4c52b1ee6dd6a54cf23b991d6c Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 18 Apr 2018 18:53:44 +0900
Subject: [PATCH v1] Allow generalized expression syntax for partition bounds

This fixes the bug that Boolean literals true/false are not allowed
in partition bound syntax.

Authors: Kyotaro Horiguchi, Amit Langote
---
 doc/src/sgml/ref/alter_table.sgml  |   6 +-
 doc/src/sgml/ref/create_table.sgml |  14 ++--
 src/backend/optimizer/util/clauses.c   |   4 +-
 src/backend/parser/gram.y  |  58 +
 src/backend/parser/parse_agg.c |  10 +++
 src/backend/parser/parse_expr.c|   5 ++
 src/backend/parser/parse_func.c|   3 +
 src/backend/parser/parse_utilcmd.c | 126 ++---
 src/include/optimizer/clauses.h|   2 +
 src/include/parser/parse_node.h|   1 +
 src/include/utils/partcache.h  |   6 ++
 src/test/regress/expected/create_table.out |  16 +---
 src/test/regress/sql/create_table.sql  |   5 +-
 13 files changed, 142 insertions(+), 114 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml 
b/doc/src/sgml/ref/alter_table.sgml
index 8c7b9619b8..2c11a5ad75 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -87,9 +87,9 @@ ALTER TABLE [ IF EXISTS ] name
 
 and partition_bound_spec 
is:
 
-IN ( { numeric_literal | 
string_literal | NULL } [, ...] ) |
-FROM ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] )
-  TO ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] ) |
+IN ( partition_bound_expr [, ...] 
) |
+FROM ( { partition_bound_expr | 
MINVALUE | MAXVALUE } [, ...] )
+  TO ( { partition_bound_expr | 
MINVALUE | MAXVALUE } [, ...] ) |
 WITH ( MODULUS numeric_literal, 
REMAINDER numeric_literal )
 
 and column_constraint 
is:
diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index f2bd562d55..b7d5562345 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -86,9 +86,9 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] 
TABLE [ IF NOT EXI
 
 and partition_bound_spec 
is:
 
-IN ( { numeric_literal | 
string_literal | NULL } [, ...] ) |
-FROM ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] )
-  TO ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] ) |
+IN ( partition_bound_expr [, ...] 
) |
+FROM ( { partition_bound_expr | 
MINVALUE | MAXVALUE } [, ...] )
+  TO ( { partition_bound_expr | 
MINVALUE | MAXVALUE } [, ...] ) |
 WITH ( MODULUS numeric_literal, 
REMAINDER numeric_literal )
 
 index_parameters in 
UNIQUE, PRIMARY KEY, and 
EXCLUDE constraints are:
@@ -275,10 +275,10 @@ WITH ( MODULUS numeric_literal, REM
  
   Each of the values specified in
   the partition_bound_spec is
-  a literal, NULL, MINVALUE, or
-  MAXVALUE.  Each literal value must be either a
-  numeric constant that is coercible to the corresponding partition key
-  column's type, or a string literal that is valid input for that type.
+  any variable-free expression (subqueries, window functions, aggregate
+  functions, and set-returning functions are not allowed).  The data type
+  of the partition bound expression must match the data type of the
+  corresponding partition key column.
  
 
  
diff --git a/src/backend/optimizer/util/clauses.c 
b/src/backend/optimizer/util/clauses.c
index 505ae0af85..77ebb40ef2 100644
--- 

Re: Boolean partitions syntax

[Tom Lane]

Amit Langote  writes:
> I think if this bug/open item can be resolved by adopting the minimal
> patch, then we should use it for that.  Maybe, we can discuss the rest of
> the changes independently.  If they make things better overall, we should
> definitely try to adopt them.

Yeah.  While I think that getting rid of the grammar restrictions on what
a partbound can be is a good idea, it seems like this is not the sort of
improvement to be making post-feature-freeze.  And it's certainly not
something to back-patch to v10.

I propose the attached slightly-less-invasive version of Amit's original
patch as what we should do in v10 and v11, and push the patch currently
under discussion out to v12.

> About the changes in transformPartitionBoundValue() to check for collation
> conflict, I think that seems unnecessary.

I agree.  We can document that the partbound expression is reduced to a
simple constant and leave it at that.  Nobody has yet been confused by
the possibility of putting COLLATE in a default expression, and I don't
believe that anybody will be confused here.

(Speaking of documentation, nobody seems to have noticed that
partition_bound_spec appears in alter_table.sgml too.)

regards, tom lane

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 8c7b961..9ce59e3 100644
*** a/doc/src/sgml/ref/alter_table.sgml
--- b/doc/src/sgml/ref/alter_table.sgml
*** ALTER TABLE [ IF EXISTS ] and partition_bound_spec is:
  
! IN ( { numeric_literal | string_literal | NULL } [, ...] ) |
! FROM ( { numeric_literal | string_literal | MINVALUE | MAXVALUE } [, ...] )
!   TO ( { numeric_literal | string_literal | MINVALUE | MAXVALUE } [, ...] ) |
  WITH ( MODULUS numeric_literal, REMAINDER numeric_literal )
  
  and column_constraint is:
--- 87,95 
  
  and partition_bound_spec is:
  
! IN ( { numeric_literal | string_literal | TRUE | FALSE | NULL } [, ...] ) |
! FROM ( { numeric_literal | string_literal | TRUE | FALSE | MINVALUE | MAXVALUE } [, ...] )
!   TO ( { numeric_literal | string_literal | TRUE | FALSE | MINVALUE | MAXVALUE } [, ...] ) |
  WITH ( MODULUS numeric_literal, REMAINDER numeric_literal )
  
  and column_constraint is:
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index f2bd562..763b4f5 100644
*** a/doc/src/sgml/ref/create_table.sgml
--- b/doc/src/sgml/ref/create_table.sgml
*** CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY 
*** 86,94 
  
  and partition_bound_spec is:
  
! IN ( { numeric_literal | string_literal | NULL } [, ...] ) |
! FROM ( { numeric_literal | string_literal | MINVALUE | MAXVALUE } [, ...] )
!   TO ( { numeric_literal | string_literal | MINVALUE | MAXVALUE } [, ...] ) |
  WITH ( MODULUS numeric_literal, REMAINDER numeric_literal )
  
  index_parameters in UNIQUE, PRIMARY KEY, and EXCLUDE constraints are:
--- 86,94 
  
  and partition_bound_spec is:
  
! IN ( { numeric_literal | string_literal | TRUE | FALSE | NULL } [, ...] ) |
! FROM ( { numeric_literal | string_literal | TRUE | FALSE | MINVALUE | MAXVALUE } [, ...] )
!   TO ( { numeric_literal | string_literal | TRUE | FALSE | MINVALUE | MAXVALUE } [, ...] ) |
  WITH ( MODULUS numeric_literal, REMAINDER numeric_literal )
  
  index_parameters in UNIQUE, PRIMARY KEY, and EXCLUDE constraints are:
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e548476..6bfd22c 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
*** partbound_datum:
*** 2799,2804 
--- 2799,2806 
  			Sconst			{ $$ = makeStringConst($1, @1); }
  			| NumericOnly	{ $$ = makeAConst($1, @1); }
  			| NULL_P		{ $$ = makeNullAConst(@1); }
+ 			| TRUE_P		{ $$ = makeStringConst(pstrdup("t"), @1); }
+ 			| FALSE_P		{ $$ = makeStringConst(pstrdup("f"), @1); }
  		;
  
  partbound_datum_list:
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 654464c..470fca0 100644
*** a/src/test/regress/expected/create_table.out
--- b/src/test/regress/expected/create_table.out
*** Partition of: arrlp FOR VALUES IN ('{1}'
*** 885,887 
--- 885,901 
  Partition constraint: ((a IS NOT NULL) AND (((a)::anyarray OPERATOR(pg_catalog.=) '{1}'::integer[]) OR ((a)::anyarray OPERATOR(pg_catalog.=) '{2}'::integer[])))
  
  DROP TABLE arrlp;
+ -- partition on boolean column
+ create table boolspart (a bool) 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   |  | 
+ 

Re: Boolean partitions syntax

[Tom Lane]

Amit Langote  writes:
> [ v8-0001-Allow-generalized-expression-syntax-for-partition.patch ]

I find what you did to a_expr here to be pretty horrid.

I think what you should do is lose the partbound_datum and
PartitionRangeDatum productions altogether, replacing those with just
a_expr, as in the attached grammar-only patch.  This would result in
needing to identify MINVALUE and MAXVALUE during parse analysis, since
the grammar would just treat them as ColId expressions.  But since we're
not intending to ever allow any actual column references in partition
expressions, I don't see any harm in allowing parse analysis to treat
ColumnRefs containing those names as meaning the special items.
This is a little bit grotty, in that both MINVALUE and "minvalue" would
be recognized as the keyword, but it's sure a lot less messy than what's
there now.  And IIRC there are some other places where we're a bit
squishy about the difference between identifiers and keywords, too.

regards, tom lane

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e548476..8b0680a 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
*** static Node *makeRecursiveViewSelect(cha
*** 581,588 
  %type 	part_elem
  %type 		part_params
  %type  PartitionBoundSpec
! %type 		partbound_datum PartitionRangeDatum
! %type 		hash_partbound partbound_datum_list range_datum_list
  %type 		hash_partbound_elem
  
  /*
--- 581,587 
  %type 	part_elem
  %type 		part_params
  %type  PartitionBoundSpec
! %type 		hash_partbound
  %type 		hash_partbound_elem
  
  /*
*** PartitionBoundSpec:
*** 2739,2745 
  }
  
  			/* a LIST partition */
! 			| FOR VALUES IN_P '(' partbound_datum_list ')'
  {
  	PartitionBoundSpec *n = makeNode(PartitionBoundSpec);
  
--- 2738,2744 
  }
  
  			/* a LIST partition */
! 			| FOR VALUES IN_P '(' expr_list ')'
  {
  	PartitionBoundSpec *n = makeNode(PartitionBoundSpec);
  
*** PartitionBoundSpec:
*** 2752,2758 
  }
  
  			/* a RANGE partition */
! 			| FOR VALUES FROM '(' range_datum_list ')' TO '(' range_datum_list ')'
  {
  	PartitionBoundSpec *n = makeNode(PartitionBoundSpec);
  
--- 2751,2757 
  }
  
  			/* a RANGE partition */
! 			| FOR VALUES FROM '(' expr_list ')' TO '(' expr_list ')'
  {
  	PartitionBoundSpec *n = makeNode(PartitionBoundSpec);
  
*** hash_partbound:
*** 2795,2851 
  			}
  		;
  
- partbound_datum:
- 			Sconst			{ $$ = makeStringConst($1, @1); }
- 			| NumericOnly	{ $$ = makeAConst($1, @1); }
- 			| NULL_P		{ $$ = makeNullAConst(@1); }
- 		;
- 
- partbound_datum_list:
- 			partbound_datum		{ $$ = list_make1($1); }
- 			| partbound_datum_list ',' partbound_datum
- { $$ = lappend($1, $3); }
- 		;
- 
- range_datum_list:
- 			PartitionRangeDatum	{ $$ = list_make1($1); }
- 			| range_datum_list ',' PartitionRangeDatum
- { $$ = lappend($1, $3); }
- 		;
- 
- PartitionRangeDatum:
- 			MINVALUE
- {
- 	PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
- 
- 	n->kind = PARTITION_RANGE_DATUM_MINVALUE;
- 	n->value = NULL;
- 	n->location = @1;
- 
- 	$$ = (Node *) n;
- }
- 			| MAXVALUE
- {
- 	PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
- 
- 	n->kind = PARTITION_RANGE_DATUM_MAXVALUE;
- 	n->value = NULL;
- 	n->location = @1;
- 
- 	$$ = (Node *) n;
- }
- 			| partbound_datum
- {
- 	PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
- 
- 	n->kind = PARTITION_RANGE_DATUM_VALUE;
- 	n->value = $1;
- 	n->location = @1;
- 
- 	$$ = (Node *) n;
- }
- 		;
- 
  /*
   *
   *	ALTER TYPE
--- 2794,2799 

Re: Boolean partitions syntax

[Amit Langote]

On 2018/04/19 20:35, Kyotaro HORIGUCHI wrote:
> At Wed, 18 Apr 2018 19:27:16 +0900, Amit Langote wrote:
>> On 2018/04/16 16:17, Kyotaro HORIGUCHI wrote:
>>> It was a bother that some rules used c_expr directly but I
>>> managed to replace all of them with a_expr by lowering precedence
>>> of some ordinary keywords (PASSING, BY, COLUMNS and ROW). c_expr
>>> is no loger used elsewhere so we can just remove columnref from
>>> c_expr. Finally [abc]0_expr was eliminated and we have only
>>> a_expr, b_expr, u_expr and c_expr. This seems simple enough.
>>>
>>> The relationship among the rules after this change is as follows.
>>>
>>> a_expr --+-- columnref
>>>  +-- u_expr ---+-- c_expr -- (all old c_expr stuff except columnref)
>>>+-- (all old a_expr stuff)
>>>
>>> b_expr --+-- columnref
>>>  +-- c_expr -- (ditto)
>>>  +-- (all old b_expr stuff)
>>>
>>> On the way fixing this, I noticed that the precedence of some
>>> keywords (PRESERVE, STRIP_P) that was more than necessary and
>>> corrected it.
>>
>> Thank you for simplifying gram.y changes.  I think I was able to
>> understand them.  Having said that, I think your proposed patch to gram.y
>> could be rewritten such that they do not *appear* to impact the syntax for
>> other features like XML, etc.  For example, allowing a_expr in places
>> where previously only c_expr was allowed seems to me a dangerous, although
>> I don't have any examples to show for it.
> 
> It cannot be dangerous, since "(a_expr)" is a c_expr. The
> difference is just the way resolve conflicts. The words of which
> I changed precedence are used only in the the problematic
> contexts.

Oh, okay.  But the point I was trying to make is that if a change that the
patch makes is not necessary to solve the problem at hand, we should try
to pursue it separately.  At first, I thought *all* the changes to gram.y
in your proposed patch were necessary, but after your clarifications, it
became clear to me that the only change needed was to refactor a_expr,
c_expr such that the we can use a_expr for partition bound expressions
without shift/reduce conflicts.

>> It seems that we would like to use a_expr syntax but without columnref for
>> partition_bound expressions.  No columnrefs because they cause conflicts
>> with unreserved keywords MINVALUE and MAXVALUE that are used in range
> 
> Right.
> 
>> bound productions.  I think that can be implemented with minimal changes
>> to expression syntax productions, as shown in the attached patch.
> 
> Agreed that the refactor stuff can be separated, but I'm a bit
> uneasy with the increased number of new syntax. The purpose of
> the change of c_expr in v6 was to minimize the *structure* change
> of *_expr syntax. I agree that v8 style is preferable to
> backpatch to PG10, but I'd still like to use v6 style for PG11.

I think if this bug/open item can be resolved by adopting the minimal
patch, then we should use it for that.  Maybe, we can discuss the rest of
the changes independently.  If they make things better overall, we should
definitely try to adopt them.

>> About the changes in transformPartitionBoundValue() to check for collation
>> conflict, I think that seems unnecessary.  We're not going to use the
>> partition bound expression's collation anywhere, so trying to validate it
>> seems unnecessary.  I wondered if we should issue a WARNING to warn the
>> user that COLLATE specified in a partition bound expression is ignored,
> 
> That's definitely wrong.

Sorry, there may have been a misunderstanding.

> Partition key expression is inheriting the collation of the base
> column and any collation can be specifiable on the expression.
> Similary we can specify collation on partition bound values with
> this patch. If a key expression and a bound value have
> contradicting collations, we cannot determine the ordering of the
> values and should error out.

There is infrastructure in place to store the collation of the partition
key (partcollation column of pg_partitioned_table catalog), which is used
when routing tuples, generating partition constraint expression, etc.  So,
we do remember the collation, either the default one derived from the
underlying column's definition or a user-specified one for partitioning.
And it *is* used when routing data (a partitioning action) and when
applying partition constraints.  I will borrow an example from an email I
had sent on the pruning thread:

create table p (a text) partition by range (a collate "en_US");
create table p1 partition of p for values from ('a') to ('m');
create table p2 partition of p for values from ('m') to ('z ');

create table q (a text) partition by range (a collate "C");
create table q1 partition of q for values from ('a') to ('m');
create table q2 partition of q for values from ('m') to ('z ');

-- per "en_US", 'a' <= 'A' < 'm'
insert into p values ('A');
INSERT 0 1

-- per "C", that's not true
insert into q values 

Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

Thanks for reviewing.

At Wed, 18 Apr 2018 19:27:16 +0900, Amit Langote 
 wrote in 
<7ac6b44e-4638-3320-1512-f6c03a28d...@lab.ntt.co.jp>
> Horiguchi-san,
> 
> Thank you for updating the patch.
> 
> On 2018/04/16 16:17, Kyotaro HORIGUCHI wrote:
> > the attached v6 patch differs only in gram.y since v5.
> 
> Patch fails to compile, because it adds get_partition_col_collation to
> rel.h instead of partcache.h:
> 
> src/include/utils/rel.h: In function ‘get_partition_col_collation’:
> src/include/utils/rel.h:594:12: error: dereferencing pointer to incomplete
> type ‘struct PartitionKeyData’
> 
> PartitionKeyData definition was recently moved to partcache.h as part of
> the big partition code reorganization.

Ugg.. Thanks.

> > At Fri, 13 Apr 2018 18:55:30 +0900, Amit Langote wrote:
> >> Looking at the gram.y changes in the latest patch, I think there needs to
> >> be some explanatory comments about about the new productions -- u_expr,
> >> b0_expr, and c0_expr.
> > 
> > I think I did that. And refactord the rules.
> > 
> > It was a bother that some rules used c_expr directly but I
> > managed to replace all of them with a_expr by lowering precedence
> > of some ordinary keywords (PASSING, BY, COLUMNS and ROW). c_expr
> > is no loger used elsewhere so we can just remove columnref from
> > c_expr. Finally [abc]0_expr was eliminated and we have only
> > a_expr, b_expr, u_expr and c_expr. This seems simple enough.
> > 
> > The relationship among the rules after this change is as follows.
> > 
> > a_expr --+-- columnref
> >  +-- u_expr ---+-- c_expr -- (all old c_expr stuff except columnref)
> >+-- (all old a_expr stuff)
> >
> > b_expr --+-- columnref
> >  +-- c_expr -- (ditto)
> >  +-- (all old b_expr stuff)
> > 
> > On the way fixing this, I noticed that the precedence of some
> > keywords (PRESERVE, STRIP_P) that was more than necessary and
> > corrected it.
> 
> Thank you for simplifying gram.y changes.  I think I was able to
> understand them.  Having said that, I think your proposed patch to gram.y
> could be rewritten such that they do not *appear* to impact the syntax for
> other features like XML, etc.  For example, allowing a_expr in places
> where previously only c_expr was allowed seems to me a dangerous, although
> I don't have any examples to show for it.

It cannot be dangerous, since "(a_expr)" is a c_expr. The
difference is just the way resolve conflicts. The words of which
I changed precedence are used only in the the problematic
contexts.

> It seems that we would like to use a_expr syntax but without columnref for
> partition_bound expressions.  No columnrefs because they cause conflicts
> with unreserved keywords MINVALUE and MAXVALUE that are used in range

Right.

> bound productions.  I think that can be implemented with minimal changes
> to expression syntax productions, as shown in the attached patch.

Agreed that the refactor stuff can be separated, but I'm a bit
uneasy with the increased number of new syntax. The purpose of
the change of c_expr in v6 was to minimize the *structure* change
of *_expr syntax. I agree that v8 style is preferable to
backpatch to PG10, but I'd still like to use v6 style for PG11.

> About the changes in transformPartitionBoundValue() to check for collation
> conflict, I think that seems unnecessary.  We're not going to use the
> partition bound expression's collation anywhere, so trying to validate it
> seems unnecessary.  I wondered if we should issue a WARNING to warn the
> user that COLLATE specified in a partition bound expression is ignored,

That's definitely wrong.

Partition key expression is inheriting the collation of the base
column and any collation can be specifiable on the expression.
Similary we can specify collation on partition bound values with
this patch. If a key expression and a bound value have
contradicting collations, we cannot determine the ordering of the
values and should error out.

Collation is requried for those languages where the letter
ordering is different from ordinary order. For example,

=# create table p (a text collate "sv_SE") partition by range (a);
=# create table c1 partiton of p for values from ('x') to ('ö');

The above is accepted in V10 but,

=# create table p (a text collate "en_US") partition by range (a);
postgres=# create table c1 partiton of p for values from ('x') to ('ö');
ERROR:  syntax error at or near "partiton"
LINE 1: create table c1 partiton of p for values from ('x') to ('ö')...

This is because the collation of the key column is actually
*used* to check the range bounds. With this fix partition bound
values can have collate specification and if it differs from key
collation, it is just a mess.

> but not sure after seeing that we issue none when a user specifies
> COLLATION in the expression for a column's DEFAULT value.  We do still
> store the COLLATION expression in pg_attrdef, but it 

Re: Boolean partitions syntax

[Amit Langote]

On 2018/04/18 19:27, Amit Langote wrote:
> Please find attached an updated version of your patch.  I think we'll need
> to make some documentation changes and think about a way to back-patch
> this to PG10.

Added documentation changes.  Also, noticed that there was no need to
change the production for b_expr, so fixed that.

See attached updated patch.

Thanks,
Amit
>From 75f9577e2167460666d7e2d2d991d7f5e143bed0 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 18 Apr 2018 18:53:44 +0900
Subject: [PATCH v8] Allow generalized expression syntax for partition bounds

This fixes the bug that Boolean literals true/false are not allowed
in partition bound syntax.

Author: Kyotaro Horiguchi
---
 doc/src/sgml/ref/create_table.sgml | 14 +++
 src/backend/optimizer/util/clauses.c   |  4 +-
 src/backend/parser/gram.y  | 28 +
 src/backend/parser/parse_agg.c | 10 +
 src/backend/parser/parse_expr.c|  5 +++
 src/backend/parser/parse_func.c|  3 ++
 src/backend/parser/parse_utilcmd.c | 66 +-
 src/include/optimizer/clauses.h|  2 +
 src/include/parser/parse_node.h|  1 +
 src/include/utils/partcache.h  |  6 +++
 src/test/regress/expected/create_table.out | 16 +---
 src/test/regress/sql/create_table.sql  |  5 +--
 12 files changed, 96 insertions(+), 64 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index f2bd562d55..b7d5562345 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -86,9 +86,9 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] 
TABLE [ IF NOT EXI
 
 and partition_bound_spec 
is:
 
-IN ( { numeric_literal | 
string_literal | NULL } [, ...] ) |
-FROM ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] )
-  TO ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] ) |
+IN ( partition_bound_expr [, ...] 
) |
+FROM ( { partition_bound_expr | 
MINVALUE | MAXVALUE } [, ...] )
+  TO ( { partition_bound_expr | 
MINVALUE | MAXVALUE } [, ...] ) |
 WITH ( MODULUS numeric_literal, 
REMAINDER numeric_literal )
 
 index_parameters in 
UNIQUE, PRIMARY KEY, and 
EXCLUDE constraints are:
@@ -275,10 +275,10 @@ WITH ( MODULUS numeric_literal, REM
  
   Each of the values specified in
   the partition_bound_spec is
-  a literal, NULL, MINVALUE, or
-  MAXVALUE.  Each literal value must be either a
-  numeric constant that is coercible to the corresponding partition key
-  column's type, or a string literal that is valid input for that type.
+  any variable-free expression (subqueries, window functions, aggregate
+  functions, and set-returning functions are not allowed).  The data type
+  of the partition bound expression must match the data type of the
+  corresponding partition key column.
  
 
  
diff --git a/src/backend/optimizer/util/clauses.c 
b/src/backend/optimizer/util/clauses.c
index 505ae0af85..77ebb40ef2 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -150,8 +150,6 @@ static Node *substitute_actual_parameters(Node *expr, int 
nargs, List *args,
 static Node *substitute_actual_parameters_mutator(Node *node,
 
substitute_actual_parameters_context *context);
 static void sql_inline_error_callback(void *arg);
-static Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
- Oid result_collation);
 static Query *substitute_actual_srf_parameters(Query *expr,
 int nargs, 
List *args);
 static Node *substitute_actual_srf_parameters_mutator(Node *node,
@@ -4840,7 +4838,7 @@ sql_inline_error_callback(void *arg)
  * We use the executor's routine ExecEvalExpr() to avoid duplication of
  * code and ensure we get the same result as the executor would get.
  */
-static Expr *
+Expr *
 evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
  Oid result_collation)
 {
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e548476623..4b2fc8c80b 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -468,8 +468,9 @@ static Node *makeRecursiveViewSelect(char *relname, List 
*aliases, Node *query);
 %typecolumnDef columnOptions
 %type  def_elem reloption_elem old_aggr_elem operator_def_elem
 %typedef_arg columnElem where_clause where_or_current_clause
-   a_expr b_expr c_expr AexprConst indirection_el 
opt_slice_bound
-   columnref in_expr having_clause func_table 
xmltable array_expr
+   a_expr a_expr_no_columnref b_expr c_expr 
c_expr_no_columnref
+   AexprConst 

Re: Boolean partitions syntax

[Amit Langote]

Horiguchi-san,

Thank you for updating the patch.

On 2018/04/16 16:17, Kyotaro HORIGUCHI wrote:
> the attached v6 patch differs only in gram.y since v5.

Patch fails to compile, because it adds get_partition_col_collation to
rel.h instead of partcache.h:

src/include/utils/rel.h: In function ‘get_partition_col_collation’:
src/include/utils/rel.h:594:12: error: dereferencing pointer to incomplete
type ‘struct PartitionKeyData’

PartitionKeyData definition was recently moved to partcache.h as part of
the big partition code reorganization.

> At Fri, 13 Apr 2018 18:55:30 +0900, Amit Langote wrote:
>> Looking at the gram.y changes in the latest patch, I think there needs to
>> be some explanatory comments about about the new productions -- u_expr,
>> b0_expr, and c0_expr.
> 
> I think I did that. And refactord the rules.
> 
> It was a bother that some rules used c_expr directly but I
> managed to replace all of them with a_expr by lowering precedence
> of some ordinary keywords (PASSING, BY, COLUMNS and ROW). c_expr
> is no loger used elsewhere so we can just remove columnref from
> c_expr. Finally [abc]0_expr was eliminated and we have only
> a_expr, b_expr, u_expr and c_expr. This seems simple enough.
> 
> The relationship among the rules after this change is as follows.
> 
> a_expr --+-- columnref
>  +-- u_expr ---+-- c_expr -- (all old c_expr stuff except columnref)
>+-- (all old a_expr stuff)
>
> b_expr --+-- columnref
>  +-- c_expr -- (ditto)
>  +-- (all old b_expr stuff)
> 
> On the way fixing this, I noticed that the precedence of some
> keywords (PRESERVE, STRIP_P) that was more than necessary and
> corrected it.

Thank you for simplifying gram.y changes.  I think I was able to
understand them.  Having said that, I think your proposed patch to gram.y
could be rewritten such that they do not *appear* to impact the syntax for
other features like XML, etc.  For example, allowing a_expr in places
where previously only c_expr was allowed seems to me a dangerous, although
I don't have any examples to show for it.

It seems that we would like to use a_expr syntax but without columnref for
partition_bound expressions.  No columnrefs because they cause conflicts
with unreserved keywords MINVALUE and MAXVALUE that are used in range
bound productions.  I think that can be implemented with minimal changes
to expression syntax productions, as shown in the attached patch.

About the changes in transformPartitionBoundValue() to check for collation
conflict, I think that seems unnecessary.  We're not going to use the
partition bound expression's collation anywhere, so trying to validate it
seems unnecessary.  I wondered if we should issue a WARNING to warn the
user that COLLATE specified in a partition bound expression is ignored,
but not sure after seeing that we issue none when a user specifies
COLLATION in the expression for a column's DEFAULT value.  We do still
store the COLLATION expression in pg_attrdef, but it doesn't seem to be
used anywhere.

Please find attached an updated version of your patch.  I think we'll need
to make some documentation changes and think about a way to back-patch
this to PG10.

Thanks,
Amit
>From 33b64b30394880dd2701359a4903dce377f41d00 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 18 Apr 2018 18:53:44 +0900
Subject: [PATCH v7] Allow generalized expression syntax for partition bounds

This fixes the bug that Boolean literals true/false are allowed in
partition bound syntax.
---
 src/backend/optimizer/util/clauses.c   |  4 +-
 src/backend/parser/gram.y  | 32 +++
 src/backend/parser/parse_agg.c | 10 +
 src/backend/parser/parse_expr.c|  5 +++
 src/backend/parser/parse_func.c|  3 ++
 src/backend/parser/parse_utilcmd.c | 66 +-
 src/include/optimizer/clauses.h|  2 +
 src/include/parser/parse_node.h|  1 +
 src/include/utils/partcache.h  |  6 +++
 src/test/regress/expected/create_table.out | 16 +---
 src/test/regress/sql/create_table.sql  |  5 +--
 11 files changed, 92 insertions(+), 58 deletions(-)

diff --git a/src/backend/optimizer/util/clauses.c 
b/src/backend/optimizer/util/clauses.c
index 505ae0af85..77ebb40ef2 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -150,8 +150,6 @@ static Node *substitute_actual_parameters(Node *expr, int 
nargs, List *args,
 static Node *substitute_actual_parameters_mutator(Node *node,
 
substitute_actual_parameters_context *context);
 static void sql_inline_error_callback(void *arg);
-static Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
- Oid result_collation);
 static Query *substitute_actual_srf_parameters(Query *expr,
 

Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

Sorry for a silly typo.

At Mon, 16 Apr 2018 16:17:40 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20180416.161740.51264437.horiguchi.kyot...@lab.ntt.co.jp>
> Hello. Thank you for the comment.
> 
> the attached v6 patch differs only in gram.y since v5.
> 
> At Fri, 13 Apr 2018 18:55:30 +0900, Amit Langote 
>  wrote in 
> 
> > Horiguchi-san,
> > 
> > Thanks for the latest patch.
> > 
> > On 2018/04/12 13:12, Kyotaro HORIGUCHI wrote:
> > > Thank you for verification and the revised patch. The format is
> > > fine and the fix is correct but I noticed that I forgot to remove
> > > plural S's from error messages. The attached is the version with
> > > the fix.
> > 
> > Looking at the gram.y changes in the latest patch, I think there needs to
> > be some explanatory comments about about the new productions -- u_expr,
> > b0_expr, and c0_expr.
> 
> I think I did that. And refactord the rules.
> 
> It was a bother that some rules used c_expr directly but I
> managed to replace all of them with a_expr by lowering precedence

s/lowering/increasing/;

> of some ordinary keywords (PASSING, BY, COLUMNS and ROW). c_expr
> is no loger used elsewhere so we can just remove columnref from
> c_expr. Finally [abc]0_expr was eliminated and we have only
> a_expr, b_expr, u_expr and c_expr. This seems simple enough.
> 
> The relationship among the rules after this change is as follows.
> 
> a_expr --+-- columnref
>  +-- u_expr ---+-- c_expr -- (all old c_expr stuff except columnref)
>+-- (all old a_expr stuff)
>
> b_expr --+-- columnref
>  +-- c_expr -- (ditto)
>  +-- (all old b_expr stuff)
> 
> On the way fixing this, I noticed that the precedence of some
> keywords (PRESERVE, STRIP_P) that was more than necessary and
> corrected it.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

Hello. Thank you for the comment.

the attached v6 patch differs only in gram.y since v5.

At Fri, 13 Apr 2018 18:55:30 +0900, Amit Langote 
 wrote in 

> Horiguchi-san,
> 
> Thanks for the latest patch.
> 
> On 2018/04/12 13:12, Kyotaro HORIGUCHI wrote:
> > Thank you for verification and the revised patch. The format is
> > fine and the fix is correct but I noticed that I forgot to remove
> > plural S's from error messages. The attached is the version with
> > the fix.
> 
> Looking at the gram.y changes in the latest patch, I think there needs to
> be some explanatory comments about about the new productions -- u_expr,
> b0_expr, and c0_expr.

I think I did that. And refactord the rules.

It was a bother that some rules used c_expr directly but I
managed to replace all of them with a_expr by lowering precedence
of some ordinary keywords (PASSING, BY, COLUMNS and ROW). c_expr
is no loger used elsewhere so we can just remove columnref from
c_expr. Finally [abc]0_expr was eliminated and we have only
a_expr, b_expr, u_expr and c_expr. This seems simple enough.

The relationship among the rules after this change is as follows.

a_expr --+-- columnref
 +-- u_expr ---+-- c_expr -- (all old c_expr stuff except columnref)
   +-- (all old a_expr stuff)
   
b_expr --+-- columnref
 +-- c_expr -- (ditto)
 +-- (all old b_expr stuff)

On the way fixing this, I noticed that the precedence of some
keywords (PRESERVE, STRIP_P) that was more than necessary and
corrected it.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index ed6b680ed8..cfb0984100 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -152,8 +152,6 @@ static Node *substitute_actual_parameters(Node *expr, int nargs, List *args,
 static Node *substitute_actual_parameters_mutator(Node *node,
 	 substitute_actual_parameters_context *context);
 static void sql_inline_error_callback(void *arg);
-static Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
-			  Oid result_collation);
 static Query *substitute_actual_srf_parameters(Query *expr,
  int nargs, List *args);
 static Node *substitute_actual_srf_parameters_mutator(Node *node,
@@ -4842,7 +4840,7 @@ sql_inline_error_callback(void *arg)
  * We use the executor's routine ExecEvalExpr() to avoid duplication of
  * code and ensure we get the same result as the executor would get.
  */
-static Expr *
+Expr *
 evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
 			  Oid result_collation)
 {
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e548476623..ebdb0f7b18 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -180,6 +180,8 @@ static Node *makeXmlExpr(XmlExprOp op, char *name, List *named_args,
 static List *mergeTableFuncParameters(List *func_args, List *columns);
 static TypeName *TableFuncTypeName(List *columns);
 static RangeVar *makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner);
+static Node *makePartRangeDatum(PartitionRangeDatumKind kind, Node *value,
+int location);
 static void SplitColQualList(List *qualList,
 			 List **constraintList, CollateClause **collClause,
 			 core_yyscan_t yyscanner);
@@ -468,7 +470,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type 	columnDef columnOptions
 %type 	def_elem reloption_elem old_aggr_elem operator_def_elem
 %type 	def_arg columnElem where_clause where_or_current_clause
-a_expr b_expr c_expr AexprConst indirection_el opt_slice_bound
+a_expr u_expr b_expr c_expr
+AexprConst indirection_el opt_slice_bound
 columnref in_expr having_clause func_table xmltable array_expr
 ExclusionWhereClause operator_def_arg
 %type 	rowsfrom_item rowsfrom_list opt_col_def_list
@@ -581,7 +584,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type 	part_elem
 %type 		part_params
 %type  PartitionBoundSpec
-%type 		partbound_datum PartitionRangeDatum
+%type 		PartitionRangeDatum
 %type 		hash_partbound partbound_datum_list range_datum_list
 %type 		hash_partbound_elem
 
@@ -734,8 +737,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
  * for RANGE, ROWS, GROUPS so that they can follow a_expr without creating
  * postfix-operator problems;
  * for GENERATED so that it can follow b_expr;
- * and for NULL so that it can follow b_expr in ColQualList without creating
- * postfix-operator problems.
+ * for NULL so that it can follow b_expr in ColQualList without creating
+ * postfix-operator problems;
+ * for ROWS to support opt_existing_window_frame;
+ * for ROW to support row;
+ * for PASSING, BY and COLUMNS to support xmltable;

Re: Boolean partitions syntax

[Amit Langote]

Horiguchi-san,

Thanks for the latest patch.

On 2018/04/12 13:12, Kyotaro HORIGUCHI wrote:
> Thank you for verification and the revised patch. The format is
> fine and the fix is correct but I noticed that I forgot to remove
> plural S's from error messages. The attached is the version with
> the fix.

Looking at the gram.y changes in the latest patch, I think there needs to
be some explanatory comments about about the new productions -- u_expr,
b0_expr, and c0_expr.

Thanks,
Amit

Re: Boolean partitions syntax

[Jonathan S. Katz]

Hi,

> On Apr 12, 2018, at 12:12 AM, Kyotaro HORIGUCHI 
>  wrote:
> 
> Hello.
> 
> At Wed, 11 Apr 2018 09:43:37 -0400, "Jonathan S. Katz" 
> > 
> wrote in  >
>>case EXPR_KIND_PARTITION_BOUNDS:
>> ^~
> ..
>> 2 errors generated.
>> 
>> The attached patch fixes the error.
> 
> Sorry for the silly mistake.
> 
>> I ran the following cases:
>> 
>> Case #1: My Original Test Case
>> 
>>  CREATE TABLE records (
>>  id int GENERATED BY DEFAULT AS IDENTITY NOT NULL,
>>  record_date date NOT NULL,
>>  record_text text,
>>  archived bool NOT NULL DEFAULT FALSE
>>  ) PARTITION BY LIST(archived);
>> 
>>  CREATE TABLE records_archive
>>  PARTITION OF records
>>  FOR VALUES IN (TRUE);
>> 
>>  CREATE TABLE records_active
>>  PARTITION OF records
>>  FOR VALUES IN (FALSE);
>> 
>> Everything created like a charm.
>> 
>> Case #2: random()
>> 
>>  CREATE TABLE oddity (
>>  id int GENERATED BY DEFAULT AS IDENTITY NOT NULL,
>>  random_filter int
>>  ) PARTITION BY LIST(random_filter);
>> 
>>  CREATE TABLE oddity_random
>>  PARTITION OF oddity
>>  FOR VALUES IN ((random() * 100)::int);
>> 
>> I did a \d+ on oddity and:
>> 
>>  partitions=# \d+ oddity
>>  (truncated)
>>  Partition key: LIST (random_filter)
>>  Partitions: oddity_random FOR VALUES IN (19)
>> 
>> So this appears to behave as described above.
>> 
>> Attached is the patch with the fix for the build.  This is the first time 
>> I’m attaching
>> a patch for the core server, so apologizes if I missed up the formatting.
> 
> Thank you for verification and the revised patch. The format is
> fine and the fix is correct but I noticed that I forgot to remove
> plural S's from error messages. The attached is the version with
> the fix.

I applied the new version of the patch and ran through the above scenarios
again.  Everything behaved as expected from a user standpoint.

Thanks,

Jonathan

Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

Hello.

At Wed, 11 Apr 2018 09:43:37 -0400, "Jonathan S. Katz" 
 wrote in 

> case EXPR_KIND_PARTITION_BOUNDS:
>  ^~
..
> 2 errors generated.
> 
> The attached patch fixes the error.

Sorry for the silly mistake.

> I ran the following cases:
> 
> Case #1: My Original Test Case
> 
>   CREATE TABLE records (
>   id int GENERATED BY DEFAULT AS IDENTITY NOT NULL,
>   record_date date NOT NULL,
>   record_text text,
>   archived bool NOT NULL DEFAULT FALSE
>   ) PARTITION BY LIST(archived);
> 
>   CREATE TABLE records_archive
>   PARTITION OF records
>   FOR VALUES IN (TRUE);
> 
>   CREATE TABLE records_active
>   PARTITION OF records
>   FOR VALUES IN (FALSE);
> 
> Everything created like a charm.
> 
> Case #2: random()
> 
>   CREATE TABLE oddity (
>   id int GENERATED BY DEFAULT AS IDENTITY NOT NULL,
>   random_filter int
>   ) PARTITION BY LIST(random_filter);
> 
>   CREATE TABLE oddity_random
>   PARTITION OF oddity
>   FOR VALUES IN ((random() * 100)::int);
> 
> I did a \d+ on oddity and:
> 
>   partitions=# \d+ oddity
>   (truncated)
>   Partition key: LIST (random_filter)
>   Partitions: oddity_random FOR VALUES IN (19)
> 
> So this appears to behave as described above.
> 
> Attached is the patch with the fix for the build.  This is the first time I’m 
> attaching
> a patch for the core server, so apologizes if I missed up the formatting.

Thank you for verification and the revised patch. The format is
fine and the fix is correct but I noticed that I forgot to remove
plural S's from error messages. The attached is the version with
the fix.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index ed6b680ed8..cfb0984100 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -152,8 +152,6 @@ static Node *substitute_actual_parameters(Node *expr, int nargs, List *args,
 static Node *substitute_actual_parameters_mutator(Node *node,
 	 substitute_actual_parameters_context *context);
 static void sql_inline_error_callback(void *arg);
-static Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
-			  Oid result_collation);
 static Query *substitute_actual_srf_parameters(Query *expr,
  int nargs, List *args);
 static Node *substitute_actual_srf_parameters_mutator(Node *node,
@@ -4842,7 +4840,7 @@ sql_inline_error_callback(void *arg)
  * We use the executor's routine ExecEvalExpr() to avoid duplication of
  * code and ensure we get the same result as the executor would get.
  */
-static Expr *
+Expr *
 evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
 			  Oid result_collation)
 {
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index dd0c26c11b..2745c4b3da 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -180,6 +180,8 @@ static Node *makeXmlExpr(XmlExprOp op, char *name, List *named_args,
 static List *mergeTableFuncParameters(List *func_args, List *columns);
 static TypeName *TableFuncTypeName(List *columns);
 static RangeVar *makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner);
+static Node *makePartRangeDatum(PartitionRangeDatumKind kind, Node *value,
+int location);
 static void SplitColQualList(List *qualList,
 			 List **constraintList, CollateClause **collClause,
 			 core_yyscan_t yyscanner);
@@ -472,7 +474,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type 	columnDef columnOptions
 %type 	def_elem reloption_elem old_aggr_elem operator_def_elem
 %type 	def_arg columnElem where_clause where_or_current_clause
-a_expr b_expr c_expr AexprConst indirection_el opt_slice_bound
+a_expr u_expr b_expr b0_expr c_expr c0_expr
+AexprConst indirection_el opt_slice_bound
 columnref in_expr having_clause func_table xmltable array_expr
 ExclusionWhereClause operator_def_arg
 %type 	rowsfrom_item rowsfrom_list opt_col_def_list
@@ -585,7 +588,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type 	part_elem
 %type 		part_params
 %type  PartitionBoundSpec
-%type 		partbound_datum PartitionRangeDatum
+%type 		PartitionRangeDatum
 %type 		hash_partbound partbound_datum_list range_datum_list
 %type 		hash_partbound_elem
 
@@ -2804,15 +2807,9 @@ hash_partbound:
 			}
 		;
 
-partbound_datum:
-			Sconst			{ $$ = makeStringConst($1, @1); }
-			| NumericOnly	{ $$ = makeAConst($1, @1); }
-			| NULL_P		{ $$ = makeNullAConst(@1); }
-		;
-
 partbound_datum_list:
-			partbound_datum		{ $$ = list_make1($1); }
-			| partbound_datum_list ',' partbound_datum
+			u_expr		{ $$ = list_make1($1); }
+			| 

Re: Boolean partitions syntax

[Jonathan S. Katz]

> On Apr 11, 2018, at 4:33 AM, Kyotaro HORIGUCHI 
>  wrote:
> 
> Thank you for the comments.
> 
> At Wed, 11 Apr 2018 13:51:55 +0900, Amit Langote 
>  wrote in 
> <3d0fda29-986c-d970-a22c-b4bd44f56...@lab.ntt.co.jp>
>> Horiguchi-san,
>> 
>> Thanks for working on this.
>> 
>> On 2018/04/11 13:20, Kyotaro HORIGUCHI wrote:
>>> At Wed, 11 Apr 2018 11:27:17 +0900, Amit Langote wrote:
 On 2018/04/11 10:44, Tom Lane wrote:
> Kyotaro HORIGUCHI  writes:
>> At least partition bound *must* be a constant. Any expression
>> that can be reduced to a constant at parse time ought to be
>> accepted but must not be accepted if not.
> 
> My point is that *any* expression can be reduced to a constant,
> we just have to do so.
>>> 
>>> Agreed in that sense. What was in my mind was something like
>>> column reference, random() falls into reducible category of
>>> course.
>>> 
>>> # I regard the change in gram.y is regarded as acceptable as a
>>> # direction, so I'll continue to working on this.
>> 
>> I haven't yet reviewed the grammar changes in detail yet...
> 
 I think what Tom is proposing here, instead of bailing out upon
 eval_const_expressions() failing to simplify the input expression to a
 Const, is to *invoke the executor* to evaluate the expression, like the
 optimizer does in evaluate_expr, and cook up a Const with whatever comes
 out to store it into the catalog (that is, in relpartbound).
>>> 
>>> Yes. In the attached I used evaluate_expr by making it non-static
>>> function. a_expr used instead of partbound_datum is changed to
>>> u_expr, which is the same with range bounds.
>>> 
 =# create table c1 partition of p for values in (random() * 100);
 CREATE TABLE
 =# \d c1
>>> ...
 Partition of: p FOR VALUES IN (97)
>> 
>> I looked at the non-gram.y portions of the patch for now as I was also
>> playing with this.  Some comments on your patch:
>> 
>> * You missed adding a break here for the EXPR_KIND_PARTITION_EXPRESSION case
>> 
>> case EXPR_KIND_PARTITION_EXPRESSION:
>> err = _("window functions are not allowed in partition key
>> expressions");
>> +case EXPR_KIND_PARTITION_BOUNDS:
>> +err = _("window functions are not allowed in partition bounds");
>> break;
>> 
>> So, the following is the wrong error message that you probably failed to
>> notice:
> 
> Oops! Fixed along with another one. I haven't looked the
> difference so seriously.
> 
>> --- a/src/test/regress/expected/create_table.out
>> +++ b/src/test/regress/expected/create_table.out
>> @@ -308,7 +308,7 @@ CREATE TABLE partitioned (
>> a int,
>> b int
>> ) PARTITION BY RANGE ((avg(a) OVER (PARTITION BY b)));
>> -ERROR:  window functions are not allowed in partition key expressions
>> +ERROR:  window functions are not allowed in partition bounds
> 
> I felt a bit uneasy to saw that in the very corner of my mind..
> 
>> * I think the new ParseExprKind you added should omit the last "S", that
>> is, name it EXPR_KIND_PARTITION_BOUND, because these are expressions to
>> represent individual bound values.  And so adjust the comment to say "bound".
>> 
>> +EXPR_KIND_PARTITION_BOUNDS, /* partition bounds value */
> 
> Agreed.
> 
>> * When looking at the changes to transformPartitionBoundValue, I noticed
>> that there is no new argument Oid colTypColl
>> 
>> static Const *
>> -transformPartitionBoundValue(ParseState *pstate, A_Const *con,
>> +transformPartitionBoundValue(ParseState *pstate, Node *val,
>>  const char *colName, Oid colType, int32
>> colTypmod)
>> 
>> that's because you seem to pass the expression's type, typmod, and typcoll
>> to the newly added call to evaluate_expr.  I wonder if we should really
>> pass the partition key specified values here.  We already receive first
>> two from the caller.
> 
> I overlooked that the value (Expr) is already coerced at the
> time. Added collation handling and this would be back-patchable.
> 
> I'm not sure how we should handle collate in this case but the
> patch rejects bound values with non-default collation if it is
> different from partition key.
> 
> Collation check works
> 
> =# create table p (a text collate "de_DE") partition by (a)
> =# create table c1 partition of p for values in (('a' collate "ja_JP"));
> ERROR:  collation mismatch between partition key expression (12622) and 
> partition bound value (12684)
> LINE 1: create table c1 partition of p for values in (('a' collate "...
> 
>> * In the changes to create_table.out
>> 
>> @@ -450,13 +450,9 @@ CREATE TABLE part_1 PARTITION OF list_parted FOR
>> VALUES IN ('1');
>> CREATE TABLE part_2 PARTITION OF list_parted FOR VALUES IN (2);
>> CREATE TABLE part_null PARTITION OF list_parted FOR VALUES IN (null);
>> CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN (int '1');
>> -ERROR:  

Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

Thank you for the comments.

At Wed, 11 Apr 2018 13:51:55 +0900, Amit Langote 
 wrote in 
<3d0fda29-986c-d970-a22c-b4bd44f56...@lab.ntt.co.jp>
> Horiguchi-san,
> 
> Thanks for working on this.
> 
> On 2018/04/11 13:20, Kyotaro HORIGUCHI wrote:
> > At Wed, 11 Apr 2018 11:27:17 +0900, Amit Langote wrote:
> >> On 2018/04/11 10:44, Tom Lane wrote:
> >>> Kyotaro HORIGUCHI  writes:
>  At least partition bound *must* be a constant. Any expression
>  that can be reduced to a constant at parse time ought to be
>  accepted but must not be accepted if not.
> >>>
> >>> My point is that *any* expression can be reduced to a constant,
> >>> we just have to do so.
> > 
> > Agreed in that sense. What was in my mind was something like
> > column reference, random() falls into reducible category of
> > course.
> > 
> > # I regard the change in gram.y is regarded as acceptable as a
> > # direction, so I'll continue to working on this.
> 
> I haven't yet reviewed the grammar changes in detail yet...

> >> I think what Tom is proposing here, instead of bailing out upon
> >> eval_const_expressions() failing to simplify the input expression to a
> >> Const, is to *invoke the executor* to evaluate the expression, like the
> >> optimizer does in evaluate_expr, and cook up a Const with whatever comes
> >> out to store it into the catalog (that is, in relpartbound).
> > 
> > Yes. In the attached I used evaluate_expr by making it non-static
> > function. a_expr used instead of partbound_datum is changed to
> > u_expr, which is the same with range bounds.
> > 
> >> =# create table c1 partition of p for values in (random() * 100);
> >> CREATE TABLE
> >> =# \d c1
> > ...
> >> Partition of: p FOR VALUES IN (97)
> 
> I looked at the non-gram.y portions of the patch for now as I was also
> playing with this.  Some comments on your patch:
> 
> * You missed adding a break here for the EXPR_KIND_PARTITION_EXPRESSION case
> 
>  case EXPR_KIND_PARTITION_EXPRESSION:
>  err = _("window functions are not allowed in partition key
> expressions");
> +case EXPR_KIND_PARTITION_BOUNDS:
> +err = _("window functions are not allowed in partition bounds");
>  break;
> 
> So, the following is the wrong error message that you probably failed to
> notice:

Oops! Fixed along with another one. I haven't looked the
difference so seriously.

> --- a/src/test/regress/expected/create_table.out
> +++ b/src/test/regress/expected/create_table.out
> @@ -308,7 +308,7 @@ CREATE TABLE partitioned (
>  a int,
>  b int
>  ) PARTITION BY RANGE ((avg(a) OVER (PARTITION BY b)));
> -ERROR:  window functions are not allowed in partition key expressions
> +ERROR:  window functions are not allowed in partition bounds

I felt a bit uneasy to saw that in the very corner of my mind..

> * I think the new ParseExprKind you added should omit the last "S", that
> is, name it EXPR_KIND_PARTITION_BOUND, because these are expressions to
> represent individual bound values.  And so adjust the comment to say "bound".
> 
>  +EXPR_KIND_PARTITION_BOUNDS, /* partition bounds value */

Agreed.

> * When looking at the changes to transformPartitionBoundValue, I noticed
> that there is no new argument Oid colTypColl
> 
>  static Const *
> -transformPartitionBoundValue(ParseState *pstate, A_Const *con,
> +transformPartitionBoundValue(ParseState *pstate, Node *val,
>   const char *colName, Oid colType, int32
> colTypmod)
> 
> that's because you seem to pass the expression's type, typmod, and typcoll
> to the newly added call to evaluate_expr.  I wonder if we should really
> pass the partition key specified values here.  We already receive first
> two from the caller.

I overlooked that the value (Expr) is already coerced at the
time. Added collation handling and this would be back-patchable.

I'm not sure how we should handle collate in this case but the
patch rejects bound values with non-default collation if it is
different from partition key.

Collation check works

=# create table p (a text collate "de_DE") partition by (a)
=# create table c1 partition of p for values in (('a' collate "ja_JP"));
ERROR:  collation mismatch between partition key expression (12622) and 
partition bound value (12684)
LINE 1: create table c1 partition of p for values in (('a' collate "...

> * In the changes to create_table.out
> 
> @@ -450,13 +450,9 @@ CREATE TABLE part_1 PARTITION OF list_parted FOR
> VALUES IN ('1');
>  CREATE TABLE part_2 PARTITION OF list_parted FOR VALUES IN (2);
>  CREATE TABLE part_null PARTITION OF list_parted FOR VALUES IN (null);
>  CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN (int '1');
> -ERROR:  syntax error at or near "int"
> -LINE 1: ... fail_part PARTITION OF list_parted FOR VALUES IN (int '1');
> -  ^
> +ERROR:  partition "fail_part" would 

Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

At Wed, 11 Apr 2018 14:22:29 +0900, Amit Langote 
 wrote in 
<6e929961-4160-7338-3d26-ccf84f416...@lab.ntt.co.jp>
> On 2018/04/11 13:39, David Rowley wrote:
> > On 11 April 2018 at 05:22, Tom Lane  wrote:
> >> David Rowley  writes:
> >>> On 11 April 2018 at 03:34, Tom Lane  wrote:
>  Well, that just begs the question: why do these expressions need to
>  be immutable?  What we really want, I think, is to evaluate them
>  and reduce them to constants.  After that, it hardly matters whether
>  the original expression was volatile.  I see absolutely no moral
>  difference between "for values in (random())" and cases like
>  this, which works today:
> >>
> >>> I'd personally be pretty surprised if this worked.
> >>
> >> Well, my point is that we're *already* behaving that way in some cases,
> >> simply because of the existence of macro-like inputs for some of these
> >> datatypes.  I'm not sure that users are going to perceive a big difference
> >> between 'now'::timestamptz and now(), for example.  If we take one but
> >> not the other, I don't think anybody will see that as a feature.
> > 
> > To me, it seems a bit inconsistent to treat 'now'::timestamp and now()
> > the same way.
> > 
> > I found this in the 7.4 release notes [1]:
> > 
> > "String literals specifying time-varying date/time values, such as
> > 'now' or 'today' will no longer work as expected in column default
> > expressions; they now cause the time of the table creation to be the
> > default, not the time of the insertion. Functions such as now(),
> > current_timestamp, or current_dateshould be used instead.
> > 
> > In previous releases, there was special code so that strings such as
> > 'now' were interpreted at INSERT time and not at table creation time,
> > but this work around didn't cover all cases. Release 7.4 now requires
> > that defaults be defined properly using functions such as now() or
> > current_timestamp. These will work in all situations."
> > 
> > So isn't 'now' being different from now() in DDL something users
> > should be quite used to by now?
> > 
> > I've got to admit, I'm somewhat less concerned about evaluating
> > volatile functions in this case because you don't seem that concerned,
> > but if you happen to be wrong, then it's going to be a much harder
> > thing to fix.  Really, is anyone going to complain if we don't
> > evaluate these and reject them with an error instead? It seems like a
> > safer option to me, also less work, and we're probably less likely to
> > regret it.

That is found in the current documentation.

https://www.postgresql.org/docs/devel/static/datatype-datetime.html

(now, today and so)
|  are simply notational shorthands that will be converted to
|  ordinary date/time values when read.

> As someone said upthread, we should just document that we *always*
> evaluate the expression specified for a partition bound during create
> table and not some other time.  That seems easier than figuring out what
> to say in the error message; saying "cannot use immutable expression for
> partition bound" is likely to confuse a user even more by introducing the
> ambiguity about when partition bounds are evaluated.  Most users would
> expect it to be create table time anyway.

+1

> > We also need to decide what of this we can backpatch to PG10 to fix
> > [2].  Ideally what goes into PG10 and PG11 would be the same, so
> > perhaps that's another reason to keep it more simple.
> 
> Backpatch all of it?  Newly introduced syntax and evaluation semantics
> does not break inputs that PG 10 allows.  But I may be missing something.

My understanding is that it is not back-patchable since it
introduces different behavior explicitly mentioned in
documentation.

https://www.postgresql.org/docs/10/static/sql-createtable.html

| and partition_bound_spec is:
| 
| IN ( { numeric_literal | string_literal | NULL } [, ...] ) |
| FROM ( { numeric_literal | string_literal | MINVALUE | MAXVALUE } [, ...] )
|   TO ( { numeric_literal | string_literal | MINVALUE | MAXVALUE } [, ...] )

Boolean literals are explicitly excluded. If we back-port only
the boolean literal stuff, documentation will need updated.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Boolean partitions syntax

[Amit Langote]

On 2018/04/11 13:39, David Rowley wrote:
> On 11 April 2018 at 05:22, Tom Lane  wrote:
>> David Rowley  writes:
>>> On 11 April 2018 at 03:34, Tom Lane  wrote:
 Well, that just begs the question: why do these expressions need to
 be immutable?  What we really want, I think, is to evaluate them
 and reduce them to constants.  After that, it hardly matters whether
 the original expression was volatile.  I see absolutely no moral
 difference between "for values in (random())" and cases like
 this, which works today:
>>
>>> I'd personally be pretty surprised if this worked.
>>
>> Well, my point is that we're *already* behaving that way in some cases,
>> simply because of the existence of macro-like inputs for some of these
>> datatypes.  I'm not sure that users are going to perceive a big difference
>> between 'now'::timestamptz and now(), for example.  If we take one but
>> not the other, I don't think anybody will see that as a feature.
> 
> To me, it seems a bit inconsistent to treat 'now'::timestamp and now()
> the same way.
> 
> I found this in the 7.4 release notes [1]:
> 
> "String literals specifying time-varying date/time values, such as
> 'now' or 'today' will no longer work as expected in column default
> expressions; they now cause the time of the table creation to be the
> default, not the time of the insertion. Functions such as now(),
> current_timestamp, or current_dateshould be used instead.
> 
> In previous releases, there was special code so that strings such as
> 'now' were interpreted at INSERT time and not at table creation time,
> but this work around didn't cover all cases. Release 7.4 now requires
> that defaults be defined properly using functions such as now() or
> current_timestamp. These will work in all situations."
> 
> So isn't 'now' being different from now() in DDL something users
> should be quite used to by now?
> 
> I've got to admit, I'm somewhat less concerned about evaluating
> volatile functions in this case because you don't seem that concerned,
> but if you happen to be wrong, then it's going to be a much harder
> thing to fix.  Really, is anyone going to complain if we don't
> evaluate these and reject them with an error instead? It seems like a
> safer option to me, also less work, and we're probably less likely to
> regret it.

As someone said upthread, we should just document that we *always*
evaluate the expression specified for a partition bound during create
table and not some other time.  That seems easier than figuring out what
to say in the error message; saying "cannot use immutable expression for
partition bound" is likely to confuse a user even more by introducing the
ambiguity about when partition bounds are evaluated.  Most users would
expect it to be create table time anyway.

> We also need to decide what of this we can backpatch to PG10 to fix
> [2].  Ideally what goes into PG10 and PG11 would be the same, so
> perhaps that's another reason to keep it more simple.

Backpatch all of it?  Newly introduced syntax and evaluation semantics
does not break inputs that PG 10 allows.  But I may be missing something.

Thanks,
Amit

Re: Boolean partitions syntax

[Amit Langote]

Horiguchi-san,

Thanks for working on this.

On 2018/04/11 13:20, Kyotaro HORIGUCHI wrote:
> At Wed, 11 Apr 2018 11:27:17 +0900, Amit Langote wrote:
>> On 2018/04/11 10:44, Tom Lane wrote:
>>> Kyotaro HORIGUCHI  writes:
 At least partition bound *must* be a constant. Any expression
 that can be reduced to a constant at parse time ought to be
 accepted but must not be accepted if not.
>>>
>>> My point is that *any* expression can be reduced to a constant,
>>> we just have to do so.
> 
> Agreed in that sense. What was in my mind was something like
> column reference, random() falls into reducible category of
> course.
> 
> # I regard the change in gram.y is regarded as acceptable as a
> # direction, so I'll continue to working on this.

I haven't yet reviewed the grammar changes in detail yet...

>> Currently transformPartitionBoundValue() applies eval_const_expressions()
>> by way of calling expression_planner().  However passing to it, say, an
>> expression representing random() is unable to reduce it to a Const because
>> simplify_function/evaluate_function won't compute a mutable function like
>> random().  So, that currently results in an error like this (note that
>> this is after applying Horiguchi-san's latest patch that enables
>> specifying random() as a partition bound in the first place):
>>
>> create table foo_part partition of foo for values in ((random())::int);
>> ERROR:  specified value cannot be cast to type integer for column "a"
>> LINE 1: ...table foo_random partition of foo for values in ((random()):...
>>  ^
>> DETAIL:  The cast requires a non-immutable conversion.
>> HINT:  Try putting the literal value in single quotes.
>>
>> The error is output after the following if check in
>> transformPartitionBoundValue fails:
>>
>> /* Fail if we don't have a constant (i.e., non-immutable coercion) */
>> if (!IsA(value, Const))
>>
>> I think what Tom is proposing here, instead of bailing out upon
>> eval_const_expressions() failing to simplify the input expression to a
>> Const, is to *invoke the executor* to evaluate the expression, like the
>> optimizer does in evaluate_expr, and cook up a Const with whatever comes
>> out to store it into the catalog (that is, in relpartbound).
> 
> Yes. In the attached I used evaluate_expr by making it non-static
> function. a_expr used instead of partbound_datum is changed to
> u_expr, which is the same with range bounds.
> 
>> =# create table c1 partition of p for values in (random() * 100);
>> CREATE TABLE
>> =# \d c1
> ...
>> Partition of: p FOR VALUES IN (97)

I looked at the non-gram.y portions of the patch for now as I was also
playing with this.  Some comments on your patch:

* You missed adding a break here for the EXPR_KIND_PARTITION_EXPRESSION case

 case EXPR_KIND_PARTITION_EXPRESSION:
 err = _("window functions are not allowed in partition key
expressions");
+case EXPR_KIND_PARTITION_BOUNDS:
+err = _("window functions are not allowed in partition bounds");
 break;

So, the following is the wrong error message that you probably failed to
notice:

--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -308,7 +308,7 @@ CREATE TABLE partitioned (
 a int,
 b int
 ) PARTITION BY RANGE ((avg(a) OVER (PARTITION BY b)));
-ERROR:  window functions are not allowed in partition key expressions
+ERROR:  window functions are not allowed in partition bounds

* I think the new ParseExprKind you added should omit the last "S", that
is, name it EXPR_KIND_PARTITION_BOUND, because these are expressions to
represent individual bound values.  And so adjust the comment to say "bound".

 +EXPR_KIND_PARTITION_BOUNDS, /* partition bounds value */

* When looking at the changes to transformPartitionBoundValue, I noticed
that there is no new argument Oid colTypColl

 static Const *
-transformPartitionBoundValue(ParseState *pstate, A_Const *con,
+transformPartitionBoundValue(ParseState *pstate, Node *val,
  const char *colName, Oid colType, int32
colTypmod)

that's because you seem to pass the expression's type, typmod, and typcoll
to the newly added call to evaluate_expr.  I wonder if we should really
pass the partition key specified values here.  We already receive first
two from the caller.

* In the changes to create_table.out

@@ -450,13 +450,9 @@ CREATE TABLE part_1 PARTITION OF list_parted FOR
VALUES IN ('1');
 CREATE TABLE part_2 PARTITION OF list_parted FOR VALUES IN (2);
 CREATE TABLE part_null PARTITION OF list_parted FOR VALUES IN (null);
 CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN (int '1');
-ERROR:  syntax error at or near "int"
-LINE 1: ... fail_part PARTITION OF list_parted FOR VALUES IN (int '1');
-  ^
+ERROR:  partition 

Re: Boolean partitions syntax

[David Rowley]

On 11 April 2018 at 05:22, Tom Lane  wrote:
> David Rowley  writes:
>> On 11 April 2018 at 03:34, Tom Lane  wrote:
>>> Well, that just begs the question: why do these expressions need to
>>> be immutable?  What we really want, I think, is to evaluate them
>>> and reduce them to constants.  After that, it hardly matters whether
>>> the original expression was volatile.  I see absolutely no moral
>>> difference between "for values in (random())" and cases like
>>> this, which works today:
>
>> I'd personally be pretty surprised if this worked.
>
> Well, my point is that we're *already* behaving that way in some cases,
> simply because of the existence of macro-like inputs for some of these
> datatypes.  I'm not sure that users are going to perceive a big difference
> between 'now'::timestamptz and now(), for example.  If we take one but
> not the other, I don't think anybody will see that as a feature.

To me, it seems a bit inconsistent to treat 'now'::timestamp and now()
the same way.

I found this in the 7.4 release notes [1]:

"String literals specifying time-varying date/time values, such as
'now' or 'today' will no longer work as expected in column default
expressions; they now cause the time of the table creation to be the
default, not the time of the insertion. Functions such as now(),
current_timestamp, or current_dateshould be used instead.

In previous releases, there was special code so that strings such as
'now' were interpreted at INSERT time and not at table creation time,
but this work around didn't cover all cases. Release 7.4 now requires
that defaults be defined properly using functions such as now() or
current_timestamp. These will work in all situations."

So isn't 'now' being different from now() in DDL something users
should be quite used to by now?

I've got to admit, I'm somewhat less concerned about evaluating
volatile functions in this case because you don't seem that concerned,
but if you happen to be wrong, then it's going to be a much harder
thing to fix.  Really, is anyone going to complain if we don't
evaluate these and reject them with an error instead? It seems like a
safer option to me, also less work, and we're probably less likely to
regret it.

We also need to decide what of this we can backpatch to PG10 to fix
[2].  Ideally what goes into PG10 and PG11 would be the same, so
perhaps that's another reason to keep it more simple.

[1] https://www.postgresql.org/docs/current/static/release-7-4.html
[2] 
https://www.postgresql.org/message-id/CAKJS1f-BL%2Br5FXSejDu%3D%2BMAvzRARaawRnQ_ZFtbv_o6tha9NJw%40mail.gmail.com

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

At Wed, 11 Apr 2018 11:27:17 +0900, Amit Langote 
 wrote in 
<1810b14f-3cd7-aff5-8358-c225c0231...@lab.ntt.co.jp>
> On 2018/04/11 10:44, Tom Lane wrote:
> > Kyotaro HORIGUCHI  writes:
> >> At least partition bound *must* be a constant. Any expression
> >> that can be reduced to a constant at parse time ought to be
> >> accepted but must not be accepted if not.
> > 
> > My point is that *any* expression can be reduced to a constant,
> > we just have to do so.

Agreed in that sense. What was in my mind was something like
column reference, random() falls into reducible category of
course.

# I regard the change in gram.y is regarded as acceptable as a
# direction, so I'll continue to working on this.

> Currently transformPartitionBoundValue() applies eval_const_expressions()
> by way of calling expression_planner().  However passing to it, say, an
> expression representing random() is unable to reduce it to a Const because
> simplify_function/evaluate_function won't compute a mutable function like
> random().  So, that currently results in an error like this (note that
> this is after applying Horiguchi-san's latest patch that enables
> specifying random() as a partition bound in the first place):
> 
> create table foo_part partition of foo for values in ((random())::int);
> ERROR:  specified value cannot be cast to type integer for column "a"
> LINE 1: ...table foo_random partition of foo for values in ((random()):...
>  ^
> DETAIL:  The cast requires a non-immutable conversion.
> HINT:  Try putting the literal value in single quotes.
> 
> The error is output after the following if check in
> transformPartitionBoundValue fails:
> 
> /* Fail if we don't have a constant (i.e., non-immutable coercion) */
> if (!IsA(value, Const))
> 
> I think what Tom is proposing here, instead of bailing out upon
> eval_const_expressions() failing to simplify the input expression to a
> Const, is to *invoke the executor* to evaluate the expression, like the
> optimizer does in evaluate_expr, and cook up a Const with whatever comes
> out to store it into the catalog (that is, in relpartbound).

Yes. In the attached I used evaluate_expr by making it non-static
function. a_expr used instead of partbound_datum is changed to
u_expr, which is the same with range bounds.

> =# create table c1 partition of p for values in (random() * 100);
> CREATE TABLE
> =# \d c1
...
> Partition of: p FOR VALUES IN (97)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index ed6b680ed8..cfb0984100 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -152,8 +152,6 @@ static Node *substitute_actual_parameters(Node *expr, int nargs, List *args,
 static Node *substitute_actual_parameters_mutator(Node *node,
 	 substitute_actual_parameters_context *context);
 static void sql_inline_error_callback(void *arg);
-static Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
-			  Oid result_collation);
 static Query *substitute_actual_srf_parameters(Query *expr,
  int nargs, List *args);
 static Node *substitute_actual_srf_parameters_mutator(Node *node,
@@ -4842,7 +4840,7 @@ sql_inline_error_callback(void *arg)
  * We use the executor's routine ExecEvalExpr() to avoid duplication of
  * code and ensure we get the same result as the executor would get.
  */
-static Expr *
+Expr *
 evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
 			  Oid result_collation)
 {
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index dd0c26c11b..02979a3533 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -180,6 +180,8 @@ static Node *makeXmlExpr(XmlExprOp op, char *name, List *named_args,
 static List *mergeTableFuncParameters(List *func_args, List *columns);
 static TypeName *TableFuncTypeName(List *columns);
 static RangeVar *makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner);
+static Node *makePartRangeDatum(PartitionRangeDatumKind kind, Node *value,
+int location);
 static void SplitColQualList(List *qualList,
 			 List **constraintList, CollateClause **collClause,
 			 core_yyscan_t yyscanner);
@@ -472,7 +474,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type 	columnDef columnOptions
 %type 	def_elem reloption_elem old_aggr_elem operator_def_elem
 %type 	def_arg columnElem where_clause where_or_current_clause
-a_expr b_expr c_expr AexprConst indirection_el opt_slice_bound
+a_expr u_expr b_expr b0_expr c_expr c0_expr AexprConst indirection_el opt_slice_bound
 columnref in_expr having_clause func_table xmltable array_expr
 ExclusionWhereClause operator_def_arg
 %type 	rowsfrom_item rowsfrom_list opt_col_def_list
@@ 

Re: Boolean partitions syntax

[Amit Langote]

On 2018/04/10 23:37, David Rowley wrote:
> On 10 April 2018 at 23:13, Kyotaro HORIGUCHI
>  wrote:
>> Note: This is not intended to be committed this time but just for
>> information.
>>
>> At Tue, 10 Apr 2018 10:34:27 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
>> wrote:
>>> Just adding negation would work as a_expr is doing.
>>>
 | '-' a_expr%prec UMINUS
 { $$ = doNegate($2, @1); }
>>
>> a_expr fits partbound_datum_list as is but it cannot sit
>> side-by-side with MAX/MINVALUE at all. However c_expr can if
>> columnref is not excluded. The attached patch does that and
>> partition bound accepts the following syntax. (I didn't see the
>> transform side at all)
>>
>> create table p2c1 partition of p2 for values from (log(1000),0+1,0/1) to 
>> (10, 10, ('1'||'0')::int);
> 
> I imagined this would have had a check for volatile functions and some
> user-friendly error message to say partition bounds must be immutable,
> but instead, it does:
> 
> postgres=# create table d_p1 partition of d for values in (Random());
> ERROR:  specified value cannot be cast to type double precision for column "d"
> LINE 1: create table d_p1 partition of d for values in (Random());
> ^
> DETAIL:  The cast requires a non-immutable conversion.
> HINT:  Try putting the literal value in single quotes.
> 
> For inspiration, maybe you could follow the lead of CREATE INDEX:
> 
> postgres=# create index on d ((random()));
> ERROR:  functions in index expression must be marked IMMUTABLE

Hmm, I think we do already check that the *partition key* expression is
IMMUTABLE.

create table foo (a int) partition by list (random());
ERROR:  functions in partition key expression must be marked IMMUTABLE

This sounds to not apply to what we'd want to do with a partition bound
expression.

Thanks,
Amit

Re: Boolean partitions syntax

[Amit Langote]

On 2018/04/11 10:44, Tom Lane wrote:
> Kyotaro HORIGUCHI  writes:
>> At least partition bound *must* be a constant. Any expression
>> that can be reduced to a constant at parse time ought to be
>> accepted but must not be accepted if not.
> 
> My point is that *any* expression can be reduced to a constant,
> we just have to do so.

Currently transformPartitionBoundValue() applies eval_const_expressions()
by way of calling expression_planner().  However passing to it, say, an
expression representing random() is unable to reduce it to a Const because
simplify_function/evaluate_function won't compute a mutable function like
random().  So, that currently results in an error like this (note that
this is after applying Horiguchi-san's latest patch that enables
specifying random() as a partition bound in the first place):

create table foo_part partition of foo for values in ((random())::int);
ERROR:  specified value cannot be cast to type integer for column "a"
LINE 1: ...table foo_random partition of foo for values in ((random()):...
 ^
DETAIL:  The cast requires a non-immutable conversion.
HINT:  Try putting the literal value in single quotes.

The error is output after the following if check in
transformPartitionBoundValue fails:

/* Fail if we don't have a constant (i.e., non-immutable coercion) */
if (!IsA(value, Const))

I think what Tom is proposing here, instead of bailing out upon
eval_const_expressions() failing to simplify the input expression to a
Const, is to *invoke the executor* to evaluate the expression, like the
optimizer does in evaluate_expr, and cook up a Const with whatever comes
out to store it into the catalog (that is, in relpartbound).

Thanks,
Amit

Re: Boolean partitions syntax

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> At least partition bound *must* be a constant. Any expression
> that can be reduced to a constant at parse time ought to be
> accepted but must not be accepted if not.

My point is that *any* expression can be reduced to a constant,
we just have to do so.

> Maybe we could explicitly control that by having pseudo functions
> like eval().
> ... where sold_date = eval_on_parse('today');
> ... where sold_date = eval_on_exec('today');

This strikes me as far outside the immediate requirements.  In fact,
if we had such functions, they'd be irrelevant to this requirement.

regards, tom lane

Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

At Wed, 11 Apr 2018 02:33:58 +1200, David Rowley  
wrote in 
> On 3 February 2018 at 12:04, Tom Lane  wrote:
> > Perhaps more useful to discuss: would that truly be the semantics we want,
> > or should we just evaluate the expression and have done?  It's certainly
> > arguable that "IN (random())" ought to draw an error, not compute some
> > random value and use that.  But if you are insistent on partition bounds
> > being immutable in any strong sense, you've already got problems, because
> > e.g. a timestamptz literal's interpretation isn't necessarily fixed.
> > It's only after we've reduced the original input to Datum form that we
> > can make any real promises about the value not moving.  So I'm not seeing
> > where is the bright line between "IN ('today')" and "IN (random())".
> 
> I see there's been some progress on this thread that's probably gone a
> bit beyond here without the discussion about the desired semantics.
> 
> To kick that off, I'm wondering, in regards to the comment about
> 'today' vs random(); how does this differ from something like:
> 
> CREATE VIEW ... AS SELECT ... FROM ... WHERE datecol = 'today'; ?
> 
> In this case 'today' is going to be evaluated during the parse
> analysis that's done during CREATE VIEW. Why would partitioning need
> to be treated differently?

At least partition bound *must* be a constant. Any expression
that can be reduced to a constant at parse time ought to be
accepted but must not be accepted if not. random() is immutable
but can be reduced to a constant at parse time so it can take a
part of partbound expression freely. I don't think there's a
serious problem this side but docuementaion.

On the other hand view can take either but it is not explicitly
specifiable for its creator. The following two work in different
way for reasons of PostgreSQL internal and we cannot see the
difference until dumping definition.

create view vconstdate as select * from sales where sold_date = 'today';
create view vvardate   as select * from sales where sold_date = now()::date;

Maybe we could explicitly control that by having pseudo functions
like eval().

... where sold_date = eval_on_parse('today');
... where sold_date = eval_on_exec('today');


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Boolean partitions syntax

[Jonathan S. Katz]

> On Apr 10, 2018, at 1:22 PM, Tom Lane  wrote:
> 
> David Rowley  writes:
>> On 11 April 2018 at 03:34, Tom Lane  wrote:
>>> Well, that just begs the question: why do these expressions need to
>>> be immutable?  What we really want, I think, is to evaluate them
>>> and reduce them to constants.  After that, it hardly matters whether
>>> the original expression was volatile.  I see absolutely no moral
>>> difference between "for values in (random())" and cases like
>>> this, which works today:
> 
>> I'd personally be pretty surprised if this worked.
> 
> Well, my point is that we're *already* behaving that way in some cases,
> simply because of the existence of macro-like inputs for some of these
> datatypes.  I'm not sure that users are going to perceive a big difference
> between 'now'::timestamptz and now(), for example.  If we take one but
> not the other, I don't think anybody will see that as a feature.

+1.  Also, one hopes that a user tests their code prior to rolling it out
into a production environment, so a case like the “random()” example
has already been vetted as either not something for their partition, or
they want a one-time randomly generated number to determine how
things are partitioned.

>> What other DDL will execute a volatile function?
> 
> This might be a valid concern, not sure.  It's certainly true that
> most other DDL doesn't result in acquiring a transaction snapshot;
> but it's not *universally* true.  Certainly there's DDL that can
> execute nigh-arbitrary user code, such as CREATE INDEX.
> 
>> What if the volatile function has side
>> effects?
> 
> Can't say that that bothers me.  If the user has thought about what
> they're doing, the results won't surprise them; if they haven't,
> they're likely to be surprised in any case.

+1.  I’m all for protecting users from themselves, but there’s only so
much you can do.  This is where we can make up any knowledge
gap with documentation.

> We might be well advised to do a CCI after evaluating the expressions,
> but that could still be before anything interesting happens.
> 
>> What if the user didn't want the function evaluated and
>> somehow thought they wanted the evaluation to take place on INSERT?
> 
> You could object to awfully large chunks of SQL on the grounds that
> it might confuse somebody.

That's truer than you may think.  

At the end of the day, a user wants to be able to create a partition
with the syntax that they expect from working with other parts of the
database.  If we have clear documentation, e.g. “If you use a volatile
function for a partition, it will only be executed once” etc. should be enough
to educate, or at least say we provided notice about the behavior.

Jonathan

Re: Boolean partitions syntax

[Tom Lane]

David Rowley  writes:
> On 11 April 2018 at 03:34, Tom Lane  wrote:
>> Well, that just begs the question: why do these expressions need to
>> be immutable?  What we really want, I think, is to evaluate them
>> and reduce them to constants.  After that, it hardly matters whether
>> the original expression was volatile.  I see absolutely no moral
>> difference between "for values in (random())" and cases like
>> this, which works today:

> I'd personally be pretty surprised if this worked.

Well, my point is that we're *already* behaving that way in some cases,
simply because of the existence of macro-like inputs for some of these
datatypes.  I'm not sure that users are going to perceive a big difference
between 'now'::timestamptz and now(), for example.  If we take one but
not the other, I don't think anybody will see that as a feature.

> What other DDL will execute a volatile function?

This might be a valid concern, not sure.  It's certainly true that
most other DDL doesn't result in acquiring a transaction snapshot;
but it's not *universally* true.  Certainly there's DDL that can
execute nigh-arbitrary user code, such as CREATE INDEX.

> What if the volatile function has side
> effects?

Can't say that that bothers me.  If the user has thought about what
they're doing, the results won't surprise them; if they haven't,
they're likely to be surprised in any case.

We might be well advised to do a CCI after evaluating the expressions,
but that could still be before anything interesting happens.

> What if the user didn't want the function evaluated and
> somehow thought they wanted the evaluation to take place on INSERT?

You could object to awfully large chunks of SQL on the grounds that
it might confuse somebody.

regards, tom lane

Re: Boolean partitions syntax

[David Rowley]

On 11 April 2018 at 03:34, Tom Lane  wrote:
> David Rowley  writes:
>> I imagined this would have had a check for volatile functions and some
>> user-friendly error message to say partition bounds must be immutable,
>> but instead, it does:
>
>> postgres=# create table d_p1 partition of d for values in (Random());
>> ERROR:  specified value cannot be cast to type double precision for column 
>> "d"
>> LINE 1: create table d_p1 partition of d for values in (Random());
>> ^
>> DETAIL:  The cast requires a non-immutable conversion.
>> HINT:  Try putting the literal value in single quotes.
>
>> For inspiration, maybe you could follow the lead of CREATE INDEX:
>
>> postgres=# create index on d ((random()));
>> ERROR:  functions in index expression must be marked IMMUTABLE
>
> Well, that just begs the question: why do these expressions need to
> be immutable?  What we really want, I think, is to evaluate them
> and reduce them to constants.  After that, it hardly matters whether
> the original expression was volatile.  I see absolutely no moral
> difference between "for values in (random())" and cases like
> this, which works today:

I'd personally be pretty surprised if this worked. What other DDL will
execute a volatile function? What if the volatile function has side
effects? What if the user didn't want the function evaluated and
somehow thought they wanted the evaluation to take place on INSERT?

I imagine if someone does this then they're probably doing something
wrong, and we should tell them, rather than perhaps silently doing
something they don't want. Perhaps someone might think they can
randomly distribute records into a set of partitions with something
like: for values in(((random() * 1000)::int between 0 and 100)), they
might be surprised when all their records end up in the same (random)
partition.

If we did this, then it seems like we're more likely to live to regret
doing it, rather than regret not doing it.  If someone comes along and
gives us some valid use case in the future, then maybe it can be
considered then. I just can't imagine what that use case would be...

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: Boolean partitions syntax

[Tom Lane]

David Rowley  writes:
> I imagined this would have had a check for volatile functions and some
> user-friendly error message to say partition bounds must be immutable,
> but instead, it does:

> postgres=# create table d_p1 partition of d for values in (Random());
> ERROR:  specified value cannot be cast to type double precision for column "d"
> LINE 1: create table d_p1 partition of d for values in (Random());
> ^
> DETAIL:  The cast requires a non-immutable conversion.
> HINT:  Try putting the literal value in single quotes.

> For inspiration, maybe you could follow the lead of CREATE INDEX:

> postgres=# create index on d ((random()));
> ERROR:  functions in index expression must be marked IMMUTABLE

Well, that just begs the question: why do these expressions need to
be immutable?  What we really want, I think, is to evaluate them
and reduce them to constants.  After that, it hardly matters whether
the original expression was volatile.  I see absolutely no moral
difference between "for values in (random())" and cases like
this, which works today:

regression=# create table pp(d1 date) partition by range(d1);
CREATE TABLE
regression=# create table cc partition of pp for values from ('today') to 
('tomorrow');
CREATE TABLE
regression=# \d+ cc
   Table "public.cc"
 Column | Type | Collation | Nullable | Default | Storage | Stats target | Descr
iption 
+--+---+--+-+-+--+--
---
 d1 | date |   |  | | plain   |  | 
Partition of: pp FOR VALUES FROM ('2018-04-10') TO ('2018-04-11')
Partition constraint: ((d1 IS NOT NULL) AND (d1 >= '2018-04-10'::date) AND (d1 <
 '2018-04-11'::date))

If we're willing to reduce 'today'::date to a fixed constant,
why not random()?

regards, tom lane

Re: Boolean partitions syntax

[David Rowley]

On 10 April 2018 at 23:13, Kyotaro HORIGUCHI
 wrote:
> Note: This is not intended to be committed this time but just for
> information.
>
> At Tue, 10 Apr 2018 10:34:27 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
>  wrote in 
> <20180410.103427.244142052.horiguchi.kyot...@lab.ntt.co.jp>
>> Just adding negation would work as a_expr is doing.
>>
>> > | '-' a_expr%prec UMINUS
>> > { $$ = doNegate($2, @1); }
>
> a_expr fits partbound_datum_list as is but it cannot sit
> side-by-side with MAX/MINVALUE at all. However c_expr can if
> columnref is not excluded. The attached patch does that and
> partition bound accepts the following syntax. (I didn't see the
> transform side at all)
>
> create table p2c1 partition of p2 for values from (log(1000),0+1,0/1) to (10, 
> 10, ('1'||'0')::int);

I imagined this would have had a check for volatile functions and some
user-friendly error message to say partition bounds must be immutable,
but instead, it does:

postgres=# create table d_p1 partition of d for values in (Random());
ERROR:  specified value cannot be cast to type double precision for column "d"
LINE 1: create table d_p1 partition of d for values in (Random());
^
DETAIL:  The cast requires a non-immutable conversion.
HINT:  Try putting the literal value in single quotes.

For inspiration, maybe you could follow the lead of CREATE INDEX:

postgres=# create index on d ((random()));
ERROR:  functions in index expression must be marked IMMUTABLE

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: Boolean partitions syntax

[David Rowley]

On 3 February 2018 at 12:04, Tom Lane  wrote:
> Perhaps more useful to discuss: would that truly be the semantics we want,
> or should we just evaluate the expression and have done?  It's certainly
> arguable that "IN (random())" ought to draw an error, not compute some
> random value and use that.  But if you are insistent on partition bounds
> being immutable in any strong sense, you've already got problems, because
> e.g. a timestamptz literal's interpretation isn't necessarily fixed.
> It's only after we've reduced the original input to Datum form that we
> can make any real promises about the value not moving.  So I'm not seeing
> where is the bright line between "IN ('today')" and "IN (random())".

I see there's been some progress on this thread that's probably gone a
bit beyond here without the discussion about the desired semantics.

To kick that off, I'm wondering, in regards to the comment about
'today' vs random(); how does this differ from something like:

CREATE VIEW ... AS SELECT ... FROM ... WHERE datecol = 'today'; ?

In this case 'today' is going to be evaluated during the parse
analysis that's done during CREATE VIEW. Why would partitioning need
to be treated differently?

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

Hello.

Note: This is not intended to be committed this time but just for
information.

At Tue, 10 Apr 2018 10:34:27 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20180410.103427.244142052.horiguchi.kyot...@lab.ntt.co.jp>
> Just adding negation would work as a_expr is doing.
> 
> > | '-' a_expr%prec UMINUS
> > { $$ = doNegate($2, @1); }

a_expr fits partbound_datum_list as is but it cannot sit
side-by-side with MAX/MINVALUE at all. However c_expr can if
columnref is not excluded. The attached patch does that and
partition bound accepts the following syntax. (I didn't see the
transform side at all)

create table p2c1 partition of p2 for values from (log(1000),0+1,0/1) to (10, 
10, ('1'||'0')::int);

=# \d p2c1
...
Partition of: p2 FOR VALUES FROM (3, 1, 0) TO (10, 10, 10)


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index dd0c26c11b..2a3d6a3bb8 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -180,6 +180,8 @@ static Node *makeXmlExpr(XmlExprOp op, char *name, List *named_args,
 static List *mergeTableFuncParameters(List *func_args, List *columns);
 static TypeName *TableFuncTypeName(List *columns);
 static RangeVar *makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner);
+static Node *makePartRangeDatum(PartitionRangeDatumKind kind, Node *value,
+int location);
 static void SplitColQualList(List *qualList,
 			 List **constraintList, CollateClause **collClause,
 			 core_yyscan_t yyscanner);
@@ -472,7 +474,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type 	columnDef columnOptions
 %type 	def_elem reloption_elem old_aggr_elem operator_def_elem
 %type 	def_arg columnElem where_clause where_or_current_clause
-a_expr b_expr c_expr AexprConst indirection_el opt_slice_bound
+a_expr l_expr b_expr b0_expr c_expr c0_expr AexprConst indirection_el opt_slice_bound
 columnref in_expr having_clause func_table xmltable array_expr
 ExclusionWhereClause operator_def_arg
 %type 	rowsfrom_item rowsfrom_list opt_col_def_list
@@ -585,7 +587,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type 	part_elem
 %type 		part_params
 %type  PartitionBoundSpec
-%type 		partbound_datum PartitionRangeDatum
+%type 		PartitionRangeDatum
 %type 		hash_partbound partbound_datum_list range_datum_list
 %type 		hash_partbound_elem
 
@@ -2804,15 +2806,9 @@ hash_partbound:
 			}
 		;
 
-partbound_datum:
-			Sconst			{ $$ = makeStringConst($1, @1); }
-			| NumericOnly	{ $$ = makeAConst($1, @1); }
-			| NULL_P		{ $$ = makeNullAConst(@1); }
-		;
-
 partbound_datum_list:
-			partbound_datum		{ $$ = list_make1($1); }
-			| partbound_datum_list ',' partbound_datum
+			a_expr		{ $$ = list_make1($1); }
+			| partbound_datum_list ',' a_expr
 { $$ = lappend($1, $3); }
 		;
 
@@ -2825,33 +2821,18 @@ range_datum_list:
 PartitionRangeDatum:
 			MINVALUE
 {
-	PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
-
-	n->kind = PARTITION_RANGE_DATUM_MINVALUE;
-	n->value = NULL;
-	n->location = @1;
-
-	$$ = (Node *) n;
+	$$ = makePartRangeDatum(PARTITION_RANGE_DATUM_MINVALUE,
+			NULL, @1);
 }
 			| MAXVALUE
 {
-	PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
-
-	n->kind = PARTITION_RANGE_DATUM_MAXVALUE;
-	n->value = NULL;
-	n->location = @1;
-
-	$$ = (Node *) n;
+	$$ = makePartRangeDatum(PARTITION_RANGE_DATUM_MAXVALUE,
+			NULL, @1);
 }
-			| partbound_datum
+			| l_expr
 {
-	PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
-
-	n->kind = PARTITION_RANGE_DATUM_VALUE;
-	n->value = $1;
-	n->location = @1;
-
-	$$ = (Node *) n;
+	$$ = makePartRangeDatum(PARTITION_RANGE_DATUM_VALUE,
+			$1, @1);
 }
 		;
 
@@ -13478,9 +13459,20 @@ a_expr:		c_expr	{ $$ = $1; }
  * cause trouble in the places where b_expr is used.  For simplicity, we
  * just eliminate all the boolean-keyword-operator productions from b_expr.
  */
-b_expr:		c_expr
-{ $$ = $1; }
-			| b_expr TYPECAST Typename
+b_expr:		c_expr { $$ = $1; }
+			| b0_expr { $$ = $1; }
+		;
+
+/*
+ * l_expr is a subset of b_expr so as to fit in comma-separated list based on
+ * b_expr.
+ */
+l_expr:		c0_expr { $$ = $1; }
+			| b0_expr { $$ = $1; }
+		;
+
+/* common part of b_expr and l_expr */
+b0_expr: b_expr TYPECAST Typename
 { $$ = makeTypeCast($1, $3, @2); }
 			| '+' b_expr	%prec UMINUS
 { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "+", NULL, $2, @1); }
@@ -13554,7 +13546,11 @@ b_expr:		c_expr
  * ambiguity to the b_expr syntax.
  */
 c_expr:		columnref{ $$ = $1; }
-			| AexprConst			{ $$ = $1; }
+			| c0_expr{ $$ = $1; }
+		;
+
+/* common part of c_expr and l_expr */
+c0_expr:		 

Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

Hello, I returned to this.

I'd like to insisnt on prposing to use existing parser element.

At Mon, 9 Apr 2018 10:11:08 -0400, "Jonathan S. Katz" 
 wrote in 
<27021281-2ed7-4cde-9d82-366af10b3...@excoventures.com>
> > On Apr 9, 2018, at 10:06 AM, Tom Lane  wrote:
> > It's premature to discuss whether this could be back-patched when
> > we haven't got an acceptable patch yet.
> 
> Understood.

At Fri, 2 Mar 2018 16:49:29 +0900, Amit Langote  
wrote in <2d49cd86-acb9-fc5b-8eb7-e467b01ec...@lab.ntt.co.jp>
> I had tried the approach your patch takes and had noticed that the syntax
> had stopped accepting negative values (a regression test fails due to that).
...
> I had tried fixing that as well, but it didn't readily work.

Just adding negation would work as a_expr is doing.

> | '-' a_expr%prec UMINUS
> { $$ = doNegate($2, @1); }

Boolean and negative values are accepted as partition bound
values with the attached patch.

> =# create table p (a bool, b int) partition by list(a);
> CREATE TABLE
> =# create table ct partition of p for values in (true) partition by list (b);
> CREATE TABLE
> =# create table ct1 partition of ct for values in (-1, -2);
> CREATE TABLE

And illegal negative is rejected.

> =# create table ct3 partition of ct for values in (-true);
> ERROR:  operator does not exist: - boolean
> LINE 1: create table ct3 partition of ct for values in (-true);

Interval has a conversion to text so this behaves someshat oddly
but it seems to me that we don't need reject that explicitly.

> create table c2 partition of p for values in (interval '1 year');
> CREATE TABLE
> =# \d+ c2
...
> Partition of: p FOR VALUES IN ('1 year')

or

> =# create table c1 partition of p for values in (interval '1 day');
> ERROR:  specified value cannot be cast to type integer for column "a"
> LINE 1: create table c1 partition of p for values in (interval '1 da...

The attached patch is adding lines for error checking in some
functions like transformWindowFuncCall. They are basically
useless as they are to be rejected by parser but it seems to be
needed for consistency.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index dd0c26c11b..710f7a9dc1 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2805,9 +2805,9 @@ hash_partbound:
 		;
 
 partbound_datum:
-			Sconst			{ $$ = makeStringConst($1, @1); }
-			| NumericOnly	{ $$ = makeAConst($1, @1); }
-			| NULL_P		{ $$ = makeNullAConst(@1); }
+		AexprConst			{ $$ = $1; }
+		| '-' AexprConst			%prec UMINUS
+			{ $$ = doNegate($2, @1); }
 		;
 
 partbound_datum_list:
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 0307738946..61f59f7527 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -506,6 +506,12 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
 			else
 err = _("grouping operations are not allowed in EXECUTE parameters");
 
+		case EXPR_KIND_PARTITION_BOUNDS:
+			if (isAgg)
+err = _("aggregate functions are not allowed in partition bounds");
+			else
+err = _("grouping operations are not allowed in partition bounds");
+
 			break;
 		case EXPR_KIND_TRIGGER_WHEN:
 			if (isAgg)
@@ -908,6 +914,8 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
 			break;
 		case EXPR_KIND_PARTITION_EXPRESSION:
 			err = _("window functions are not allowed in partition key expressions");
+		case EXPR_KIND_PARTITION_BOUNDS:
+			err = _("window functions are not allowed in partition bounds");
 			break;
 		case EXPR_KIND_CALL_ARGUMENT:
 			err = _("window functions are not allowed in CALL arguments");
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 38fbe3366f..a7f3d86f75 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -1850,6 +1850,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
 		case EXPR_KIND_CALL_ARGUMENT:
 			err = _("cannot use subquery in CALL argument");
 			break;
+		case EXPR_KIND_PARTITION_BOUNDS:
+			err = _("cannot use subquery in partition bounds");
+			break;
 
 			/*
 			 * There is intentionally no default: case here, so that the
@@ -3474,6 +3477,8 @@ ParseExprKindName(ParseExprKind exprKind)
 			return "WHEN";
 		case EXPR_KIND_PARTITION_EXPRESSION:
 			return "PARTITION BY";
+		case EXPR_KIND_PARTITION_BOUNDS:
+			return "partition bounds";
 		case EXPR_KIND_CALL_ARGUMENT:
 			return "CALL";
 		case EXPR_KIND_MERGE_WHEN_AND:
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 615aee6d15..a39e26dd76 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2306,6 +2306,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
 		case 

Re: Boolean partitions syntax

[Jonathan S. Katz]

> On Apr 9, 2018, at 10:06 AM, Tom Lane  wrote:
> 
> "Jonathan S. Katz"  writes:
>> +1 based on running the above scenario on my 10.3 instance and
>> receiving the same error.  Is there a chance the fix could make it into
>> 10.4 then?
> 
> It's premature to discuss whether this could be back-patched when
> we haven't got an acceptable patch yet.

Understood.

Jonathan

Re: Boolean partitions syntax

[Tom Lane]

"Jonathan S. Katz"  writes:
> +1 based on running the above scenario on my 10.3 instance and
> receiving the same error.  Is there a chance the fix could make it into
> 10.4 then?

It's premature to discuss whether this could be back-patched when
we haven't got an acceptable patch yet.

regards, tom lane

Re: Boolean partitions syntax

[Jonathan S. Katz]

> On Apr 9, 2018, at 8:28 AM, Peter Eisentraut 
>  wrote:
> 
> On 4/7/18 11:16, Jonathan S. Katz wrote:
>> The last line yielding:
>> 
>> ERROR:  syntax error at or near "TRUE"
>> LINE 3: FOR VALUES IN (TRUE);
>> 
>> [Omitted from example: the “records_active” partition]
>> 
>> I’m glad to see this was added to the open items. I would strongly
>> suggest fixing
>> this prior to the 11 release as it is unintuitive from a user standpoint
>> to use ‘TRUE’
> 
> I think this is actually more accurately classified as an existing bug
> in PostgreSQL 10.

+1 based on running the above scenario on my 10.3 instance and
receiving the same error.  Is there a chance the fix could make it into
10.4 then?

Thanks,

Jonathan

Re: Boolean partitions syntax

[Peter Eisentraut]

On 4/7/18 11:16, Jonathan S. Katz wrote:
> The last line yielding:
> 
>     ERROR:  syntax error at or near "TRUE"
>     LINE 3: FOR VALUES IN (TRUE);
> 
> [Omitted from example: the “records_active” partition]
> 
> I’m glad to see this was added to the open items. I would strongly
> suggest fixing
> this prior to the 11 release as it is unintuitive from a user standpoint
> to use ‘TRUE’

I think this is actually more accurately classified as an existing bug
in PostgreSQL 10.

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

Re: Boolean partitions syntax

[Jonathan S. Katz]

> On Mar 21, 2018, at 10:59 PM, Amit Langote  
> wrote:
> 
> Hi David.
> 
> On 2018/03/21 23:31, David Steele wrote:
>> Hi Amit,
>> 
>> On 3/6/18 9:44 AM, David Steele wrote:
>>> On 3/2/18 2:27 AM, Amit Langote wrote:
 On 2018/03/02 15:58, Andres Freund wrote:
> On 2018-02-02 17:00:24 -0500, Tom Lane wrote:
>> Peter Eisentraut  writes:
>>> There might be other options, but one way to solve this would be to
>>> treat partition bounds as a general expression in the grammar and then
>>> check in post-parse analysis that it's a constant.
>> 
>> That's pretty much what I said upthread.  What I basically don't like
>> about the current setup is that it's assuming that the bound item is
>> a bare literal.  Even disregarding future-extension issues, that's bad
>> because it can't result in an error message smarter than "syntax error"
>> when someone tries the rather natural thing of writing a more complicated
>> expression.
> 
> Given the current state of this patch, with a number of senior
> developers disagreeing with the design, and the last CF being in
> progress, I think we should mark this as returned with feedback.
 
 I see no problem with pursuing this in the next CF if the consensus is
 that we should fix how partition bounds are parsed, instead of adopting
 one of the patches to allow the Boolean literals to be accepted as
 partition bounds.
>>> 
>>> I'm inclined to mark this patch Returned with Feedback unless I hear
>>> opinions to the contrary.
>> 
>> Hearing no opinions to the contrary I have marked this entry Returned
>> with Feedback.  Please resubmit when you have an updated patch.
> 
> OK.
> 
> Btw, there is an 11dev open item recently added to the wiki that's related
> to this, but I think we might be able to deal with it independently of
> this proposal.
> 
> * Partitions with bool partition keys *
> https://wiki.postgresql.org/wiki/PostgreSQL_11_Open_Items#Open_Issues 
> 

While testing the new partitioning features yesterday I got bit by this issue
when creating a common use-case: a table split up by active/archived records:

CREATE TABLE records (
id int GENERATED BY DEFAULT AS IDENTITY NOT NULL,
record_date date NOT NULL,
record_text text,
archived bool NOT NULL DEFAULT FALSE
) PARTITION BY LIST(archived);

CREATE TABLE records_archive
PARTITION OF records
FOR VALUES IN (TRUE);

The last line yielding:

ERROR:  syntax error at or near "TRUE"
LINE 3: FOR VALUES IN (TRUE);

[Omitted from example: the “records_active” partition]

I’m glad to see this was added to the open items. I would strongly suggest 
fixing
this prior to the 11 release as it is unintuitive from a user standpoint to use 
‘TRUE’

Thanks,

Jonathan

Re: Boolean partitions syntax

[David Steele]

On 3/21/18 10:59 PM, Amit Langote wrote:
> On 2018/03/21 23:31, David Steele wrote:
>> Hi Amit,
>>
>> On 3/6/18 9:44 AM, David Steele wrote:
>>> On 3/2/18 2:27 AM, Amit Langote wrote:
 On 2018/03/02 15:58, Andres Freund wrote:
> On 2018-02-02 17:00:24 -0500, Tom Lane wrote:
>> Peter Eisentraut  writes:
>>> There might be other options, but one way to solve this would be to
>>> treat partition bounds as a general expression in the grammar and then
>>> check in post-parse analysis that it's a constant.
>>
>> That's pretty much what I said upthread.  What I basically don't like
>> about the current setup is that it's assuming that the bound item is
>> a bare literal.  Even disregarding future-extension issues, that's bad
>> because it can't result in an error message smarter than "syntax error"
>> when someone tries the rather natural thing of writing a more complicated
>> expression.
>
> Given the current state of this patch, with a number of senior
> developers disagreeing with the design, and the last CF being in
> progress, I think we should mark this as returned with feedback.

 I see no problem with pursuing this in the next CF if the consensus is
 that we should fix how partition bounds are parsed, instead of adopting
 one of the patches to allow the Boolean literals to be accepted as
 partition bounds.
>>>
>>> I'm inclined to mark this patch Returned with Feedback unless I hear
>>> opinions to the contrary.
>>
>> Hearing no opinions to the contrary I have marked this entry Returned
>> with Feedback.  Please resubmit when you have an updated patch.
> 
> OK.
> 
> Btw, there is an 11dev open item recently added to the wiki that's related
> to this, but I think we might be able to deal with it independently of
> this proposal.
> 
> * Partitions with bool partition keys *
> https://wiki.postgresql.org/wiki/PostgreSQL_11_Open_Items#Open_Issues

If you want to bring this patch up to date and recast it as a bug fix
for the open issue I'll be happy to add it to the CF as a bug fix.

However, it seems to me the best plan might be to start with David's
patch [1] and make it play nice with old pg_dumps.

Thanks,
-- 
-David
da...@pgmasters.net

[1]
https://www.postgresql.org/message-id/flat/CAKJS1f-BL%2Br5FXSejDu%3D%2BMAvzRARaawRnQ_ZFtbv_o6tha9NJw%40mail.gmail.com#CAKJS1f-BL+r5FXSejDu=+mavzraraawrnq_zftbv_o6tha9...@mail.gmail.com

Re: Boolean partitions syntax

[Amit Langote]

Hi David.

On 2018/03/21 23:31, David Steele wrote:
> Hi Amit,
> 
> On 3/6/18 9:44 AM, David Steele wrote:
>> On 3/2/18 2:27 AM, Amit Langote wrote:
>>> On 2018/03/02 15:58, Andres Freund wrote:
 On 2018-02-02 17:00:24 -0500, Tom Lane wrote:
> Peter Eisentraut  writes:
>> There might be other options, but one way to solve this would be to
>> treat partition bounds as a general expression in the grammar and then
>> check in post-parse analysis that it's a constant.
>
> That's pretty much what I said upthread.  What I basically don't like
> about the current setup is that it's assuming that the bound item is
> a bare literal.  Even disregarding future-extension issues, that's bad
> because it can't result in an error message smarter than "syntax error"
> when someone tries the rather natural thing of writing a more complicated
> expression.

 Given the current state of this patch, with a number of senior
 developers disagreeing with the design, and the last CF being in
 progress, I think we should mark this as returned with feedback.
>>>
>>> I see no problem with pursuing this in the next CF if the consensus is
>>> that we should fix how partition bounds are parsed, instead of adopting
>>> one of the patches to allow the Boolean literals to be accepted as
>>> partition bounds.
>>
>> I'm inclined to mark this patch Returned with Feedback unless I hear
>> opinions to the contrary.
> 
> Hearing no opinions to the contrary I have marked this entry Returned
> with Feedback.  Please resubmit when you have an updated patch.

OK.

Btw, there is an 11dev open item recently added to the wiki that's related
to this, but I think we might be able to deal with it independently of
this proposal.

* Partitions with bool partition keys *
https://wiki.postgresql.org/wiki/PostgreSQL_11_Open_Items#Open_Issues

Thanks,
Amit

Re: Re: Re: Boolean partitions syntax

[David Steele]

Hi Amit,

On 3/6/18 9:44 AM, David Steele wrote:
> On 3/2/18 2:27 AM, Amit Langote wrote:
>> On 2018/03/02 15:58, Andres Freund wrote:
>>> On 2018-02-02 17:00:24 -0500, Tom Lane wrote:
 Peter Eisentraut  writes:
> There might be other options, but one way to solve this would be to
> treat partition bounds as a general expression in the grammar and then
> check in post-parse analysis that it's a constant.

 That's pretty much what I said upthread.  What I basically don't like
 about the current setup is that it's assuming that the bound item is
 a bare literal.  Even disregarding future-extension issues, that's bad
 because it can't result in an error message smarter than "syntax error"
 when someone tries the rather natural thing of writing a more complicated
 expression.
>>>
>>> Given the current state of this patch, with a number of senior
>>> developers disagreeing with the design, and the last CF being in
>>> progress, I think we should mark this as returned with feedback.
>>
>> I see no problem with pursuing this in the next CF if the consensus is
>> that we should fix how partition bounds are parsed, instead of adopting
>> one of the patches to allow the Boolean literals to be accepted as
>> partition bounds.
> 
> I'm inclined to mark this patch Returned with Feedback unless I hear
> opinions to the contrary.

Hearing no opinions to the contrary I have marked this entry Returned
with Feedback.  Please resubmit when you have an updated patch.

Regards,
-- 
-David
da...@pgmasters.net

Re: Re: Boolean partitions syntax

[David Steele]

Hi Amit,

On 3/2/18 2:27 AM, Amit Langote wrote:
> On 2018/03/02 15:58, Andres Freund wrote:
>> On 2018-02-02 17:00:24 -0500, Tom Lane wrote:
>>> Peter Eisentraut  writes:
 There might be other options, but one way to solve this would be to
 treat partition bounds as a general expression in the grammar and then
 check in post-parse analysis that it's a constant.
>>>
>>> That's pretty much what I said upthread.  What I basically don't like
>>> about the current setup is that it's assuming that the bound item is
>>> a bare literal.  Even disregarding future-extension issues, that's bad
>>> because it can't result in an error message smarter than "syntax error"
>>> when someone tries the rather natural thing of writing a more complicated
>>> expression.
>>
>> Given the current state of this patch, with a number of senior
>> developers disagreeing with the design, and the last CF being in
>> progress, I think we should mark this as returned with feedback.
> 
> I see no problem with pursuing this in the next CF if the consensus is
> that we should fix how partition bounds are parsed, instead of adopting
> one of the patches to allow the Boolean literals to be accepted as
> partition bounds.

I'm inclined to mark this patch Returned with Feedback unless I hear
opinions to the contrary.

> That said, after seeing David Rowley's post earlier today [2], it seems
> that we may need to consider this issue a bug rather than a new feature.

Perhaps that should be handled as a bug fix.  Does this patch answer the
need or should a new one be developed?

Thanks,
-- 
-David
da...@pgmasters.net

Re: Boolean partitions syntax

[Amit Langote]

Horiguchi-san,

On 2018/02/05 18:17, Kyotaro HORIGUCHI wrote:
> At Mon, 29 Jan 2018 13:21:54 +0900, Amit Langote wrote:
>> 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.
> 
> eval_const_expressions is already running under
> transformPartitionBoundValue to resolve remaining coercion.  I
> suppose we can use AexprConst to restrict the syntax within
> appropriate variations.  Please find the attached patch.
> It allows the following syntax as a by-prodcut.
> 
> | create table c4 partition of t for values in (numeric(1,0) '5');
> 
> Parser accepts arbitrary defined types but it does no harm.
> 
> | create table c2 partition of t for values from (line '{0,1,0}') to (1);
> | ERROR:  specified value cannot be cast to type double precision for column 
> "a"
> 
> It rejects unacceptable functions but the message may look
> somewhat unfriendly.
> 
> | =# create table c1 partition of t for values in (random());
> | ERROR:  syntax error at or near ")"
> | LINE 1: create table c1 partition of t for values in (random());
> |  ^
> (marker is placed under the closing parenthesis of "random()")
> 
> | =# create table c1 partition of t for values in (random(0) 'x');
> | ERROR:  type "random" does not exist
> | LINE 1: create table c1 partition of t for values in (random(0) 'x')...
> (marker is placed under the first letter of the "random".)

I had tried the approach your patch takes and had noticed that the syntax
had stopped accepting negative values (a regression test fails due to that).

create table foo (a int) partition by list (a);
create table foo1 partition of foo for values in (-1);
ERROR:  syntax error at or near "-"
LINE 1: create table foo1 partition of foo for values in (-1);

I guess the following in gram.y leaves out negative numbers:

/*
 * Constants
 */
AexprConst: Iconst
{
$$ = makeIntConst($1, @1);
}
| FCONST
{
$$ = makeFloatConst($1, @1);
}

I had tried fixing that as well, but it didn't readily work.

Thanks,
Amit

Re: Boolean partitions syntax

[Amit Langote]

On 2018/03/02 15:58, Andres Freund wrote:
> On 2018-02-02 17:00:24 -0500, Tom Lane wrote:
>> Peter Eisentraut  writes:
>>> There might be other options, but one way to solve this would be to
>>> treat partition bounds as a general expression in the grammar and then
>>> check in post-parse analysis that it's a constant.
>>
>> That's pretty much what I said upthread.  What I basically don't like
>> about the current setup is that it's assuming that the bound item is
>> a bare literal.  Even disregarding future-extension issues, that's bad
>> because it can't result in an error message smarter than "syntax error"
>> when someone tries the rather natural thing of writing a more complicated
>> expression.
> 
> Given the current state of this patch, with a number of senior
> developers disagreeing with the design, and the last CF being in
> progress, I think we should mark this as returned with feedback.

I see no problem with pursuing this in the next CF if the consensus is
that we should fix how partition bounds are parsed, instead of adopting
one of the patches to allow the Boolean literals to be accepted as
partition bounds.

For the latter, my patch [1] would do the job, although after posting that
patch, the discussion turned into the one about the state of the current
partition bound parsing code.  I agree with most of the points raised and
Horiguchi-san even came up with a not-so-invasive patch to do that,
although, neither I, nor anyone else has been able to review or test it so
far.  I had written a patch like that one myself that I haven't shared on
the list, but had seen some problems with it.  I guess I should report
those in reply to Horiguchi-san's post.

That said, after seeing David Rowley's post earlier today [2], it seems
that we may need to consider this issue a bug rather than a new feature.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/6844d7f9-8219-a9ff-88f9-82c05fc90d70%40lab.ntt.co.jp

[2]
https://www.postgresql.org/message-id/CAKJS1f-BL%2Br5FXSejDu%3D%2BMAvzRARaawRnQ_ZFtbv_o6tha9NJw%40mail.gmail.com

Re: Boolean partitions syntax

[Andres Freund]

On 2018-02-02 17:00:24 -0500, Tom Lane wrote:
> Peter Eisentraut  writes:
> > There might be other options, but one way to solve this would be to
> > treat partition bounds as a general expression in the grammar and then
> > check in post-parse analysis that it's a constant.
> 
> That's pretty much what I said upthread.  What I basically don't like
> about the current setup is that it's assuming that the bound item is
> a bare literal.  Even disregarding future-extension issues, that's bad
> because it can't result in an error message smarter than "syntax error"
> when someone tries the rather natural thing of writing a more complicated
> expression.

Given the current state of this patch, with a number of senior
developers disagreeing with the design, and the last CF being in
progress, I think we should mark this as returned with feedback.


Greetings,

Andres Freund

Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

Hello,

At Fri, 02 Feb 2018 18:04:44 -0500, Tom Lane  wrote in 
<14732.1517612...@sss.pgh.pa.us>
> Robert Haas  writes:
> > On Fri, Feb 2, 2018 at 4:40 PM, Peter Eisentraut
> >  wrote:
> >> There might be other options, but one way to solve this would be to
> >> treat partition bounds as a general expression in the grammar and then
> >> check in post-parse analysis that it's a constant.
> 
> > Yeah -- isn't the usual way of handling this to run the user's input
> > through eval_const_expressions and see if the result is constant?

At Mon, 29 Jan 2018 13:21:54 +0900, Amit Langote 
 wrote in 
<6844d7f9-8219-a9ff-88f9-82c05fc90...@lab.ntt.co.jp>
> 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.

eval_const_expressions is already running under
transformPartitionBoundValue to resolve remaining coercion.  I
suppose we can use AexprConst to restrict the syntax within
appropriate variations.  Please find the attached patch.


It allows the following syntax as a by-prodcut.

| create table c4 partition of t for values in (numeric(1,0) '5');

Parser accepts arbitrary defined types but it does no harm.

| create table c2 partition of t for values from (line '{0,1,0}') to (1);
| ERROR:  specified value cannot be cast to type double precision for column "a"

It rejects unacceptable functions but the message may look
somewhat unfriendly.

| =# create table c1 partition of t for values in (random());
| ERROR:  syntax error at or near ")"
| LINE 1: create table c1 partition of t for values in (random());
|  ^
(marker is placed under the closing parenthesis of "random()")

| =# create table c1 partition of t for values in (random(0) 'x');
| ERROR:  type "random" does not exist
| LINE 1: create table c1 partition of t for values in (random(0) 'x')...
(marker is placed under the first letter of the "random".)


> Not sure we want to go quite that far: at minimum it would imply invoking
> arbitrary stuff during a utility statement, which we generally try to
> avoid.  Still, copy-from-query does that, so we can certainly make it
> work if we wish.
> 
> Perhaps more useful to discuss: would that truly be the semantics we want,
> or should we just evaluate the expression and have done?  It's certainly
> arguable that "IN (random())" ought to draw an error, not compute some
> random value and use that.  But if you are insistent on partition bounds
> being immutable in any strong sense, you've already got problems, because
> e.g. a timestamptz literal's interpretation isn't necessarily fixed.
> It's only after we've reduced the original input to Datum form that we
> can make any real promises about the value not moving.  So I'm not seeing
> where is the bright line between "IN ('today')" and "IN (random())".
> 
>   regards, tom lane

The patch leaves the ambiguity of values like 'today' but doesn't
accept arbitrary functions. Howerver, it needs additional message
for errors that never happen since the patch adds a new item in
ParseExprKind...

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 5329432..c5d8526 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2790,9 +2790,7 @@ hash_partbound:
 		;
 
 partbound_datum:
-			Sconst			{ $$ = makeStringConst($1, @1); }
-			| NumericOnly	{ $$ = makeAConst($1, @1); }
-			| NULL_P		{ $$ = makeNullAConst(@1); }
+		AexprConst			{ $$ = $1; }
 		;
 
 partbound_datum_list:
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 6a9f1b0..9bbe9b1 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -506,6 +506,12 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
 			else
 err = _("grouping operations are not allowed in partition key expression");
 
+		case EXPR_KIND_PARTITION_BOUNDS:
+			if (isAgg)
+err = _("aggregate functions are not allowed in partition bounds value");
+			else
+err = _("grouping operations are not allowed in partition bounds value");
+
 			break;
 
 		case EXPR_KIND_CALL:
@@ -891,6 +897,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
 		case EXPR_KIND_PARTITION_EXPRESSION:
 			err = _("window functions are not allowed in partition key expression");
 			break;
+		case EXPR_KIND_PARTITION_BOUNDS:
+			err = _("window functions are not allowed in partition bounds value");
+			break;
 		case EXPR_KIND_CALL:
 			err = _("window functions are not allowed in CALL arguments");
 			break;
diff --git 

Re: Boolean partitions syntax

[Tom Lane]

Robert Haas  writes:
> On Fri, Feb 2, 2018 at 4:40 PM, Peter Eisentraut
>  wrote:
>> There might be other options, but one way to solve this would be to
>> treat partition bounds as a general expression in the grammar and then
>> check in post-parse analysis that it's a constant.

> Yeah -- isn't the usual way of handling this to run the user's input
> through eval_const_expressions and see if the result is constant?

Not sure we want to go quite that far: at minimum it would imply invoking
arbitrary stuff during a utility statement, which we generally try to
avoid.  Still, copy-from-query does that, so we can certainly make it
work if we wish.

Perhaps more useful to discuss: would that truly be the semantics we want,
or should we just evaluate the expression and have done?  It's certainly
arguable that "IN (random())" ought to draw an error, not compute some
random value and use that.  But if you are insistent on partition bounds
being immutable in any strong sense, you've already got problems, because
e.g. a timestamptz literal's interpretation isn't necessarily fixed.
It's only after we've reduced the original input to Datum form that we
can make any real promises about the value not moving.  So I'm not seeing
where is the bright line between "IN ('today')" and "IN (random())".

regards, tom lane

Re: Boolean partitions syntax

[Robert Haas]

On Fri, Feb 2, 2018 at 4:40 PM, Peter Eisentraut
 wrote:
> There might be other options, but one way to solve this would be to
> treat partition bounds as a general expression in the grammar and then
> check in post-parse analysis that it's a constant.

Yeah -- isn't the usual way of handling this to run the user's input
through eval_const_expressions and see if the result is constant?

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

Re: Boolean partitions syntax

[Tom Lane]

Peter Eisentraut  writes:
> There might be other options, but one way to solve this would be to
> treat partition bounds as a general expression in the grammar and then
> check in post-parse analysis that it's a constant.

That's pretty much what I said upthread.  What I basically don't like
about the current setup is that it's assuming that the bound item is
a bare literal.  Even disregarding future-extension issues, that's bad
because it can't result in an error message smarter than "syntax error"
when someone tries the rather natural thing of writing a more complicated
expression.

regards, tom lane

Re: Boolean partitions syntax

[Amit Langote]

On 2018/01/29 14:57, Tom Lane wrote:
> Amit Langote  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.

Hmm, I thought the following does convert an integer 1 to text value '1',
but maybe I'm missing your point.

create table foo (a text default 1);

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

I do remember the partitioning patch used to use a_expr for what today is:

partbound_datum:
Sconst  { $$ = makeStringConst($1, @1); }
| NumericOnly   { $$ = makeAConst($1, @1); }
| NULL_P{ $$ = makeNullAConst(@1); }
;

but thought at the time that that allowed way too much stuff into the
partition bound syntax.

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

Most of the code does treat partition bound values as Node values doing
coercing before calling the input value good and failing upon not being
able to convert to the desired type for whatever reason.

create table b (a bool) partition by list (a);
create table bt partition of b for values in (1);
ERROR:  specified value cannot be cast to type boolean for column "a"
LINE 1: create table bt partition of b for values in (1);

Can you say a bit more about the compatibility issues if we extend the syntax?

Thanks,
Amit

Re: Boolean partitions syntax

[Tom Lane]

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

Re: Boolean partitions syntax

[Amit Langote]

On 2018/01/27 1:31, Tom Lane wrote:
> Robert Haas  writes:
>> On Fri, Jan 26, 2018 at 11:07 AM, Stephen Frost  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 "".

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
From 392abca09192218a73066fd41131819963daf1a4 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 12 Dec 2017 10:33:11 +0900
Subject: [PATCH v4] Allow Boolean values in partition FOR VALUES clause

---
 doc/src/sgml/ref/create_table.sgml | 14 +++---
 src/backend/parser/gram.y  |  2 ++
 src/test/regress/expected/create_table.out | 14 ++
 src/test/regress/sql/create_table.sql  |  7 +++
 4 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index a0c9a6d257..61371195ac 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -86,9 +86,9 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] 
TABLE [ IF NOT EXI
 
 and partition_bound_spec 
is:
 
-IN ( { numeric_literal | 
string_literal | NULL } [, ...] ) |
-FROM ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] )
-  TO ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] ) |
+IN ( { literal_constant } [, ...] 
) |
+FROM ( { literal_constant | 
MINVALUE | MAXVALUE } [, ...] )
+  TO ( { literal_constant | 
MINVALUE | MAXVALUE } [, ...] ) |
 WITH ( MODULUS numeric_literal, 
REMAINDER numeric_literal )
 
 index_parameters in 
UNIQUE, PRIMARY KEY, and 
EXCLUDE constraints are:
@@ -274,10 +274,10 @@ WITH ( MODULUS numeric_literal, REM
  
   Each of the values specified in
   the partition_bound_spec is
-  a literal, NULL, MINVALUE, or
-  MAXVALUE.  Each literal value must be either a
-  numeric constant that is coercible to the corresponding partition key
-  column's type, or a string literal that is valid input for that type.
+  a literal constant, MINVALUE, or
+  MAXVALUE.  Each literal constant value must be either
+  a number, a Boolean, a null value, or a string that is valid input for
+  the partition key's type.
  
 
  
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 5329432f25..0c3bc67620 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2793,6 +2793,8 @@ partbound_datum:
Sconst  { $$ = makeStringConst($1, @1); 
}
| NumericOnly   { $$ = makeAConst($1, @1); }
| NULL_P{ $$ = 

Re: Boolean partitions syntax

[Amit Langote]

On 2018/01/27 0:30, Robert Haas wrote:
> On Thu, Jan 25, 2018 at 8:44 PM, Amit Langote
>  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

Re: Boolean partitions syntax

[Tom Lane]

Robert Haas  writes:
> On Fri, Jan 26, 2018 at 11:07 AM, Stephen Frost  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 "".

Re: Boolean partitions syntax

[Robert Haas]

On Fri, Jan 26, 2018 at 11:07 AM, Stephen Frost  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

Re: Boolean partitions syntax

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Jan 25, 2018 at 8:44 PM, Amit Langote
>  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
Description: PGP signature

Re: Boolean partitions syntax

[Robert Haas]

On Thu, Jan 25, 2018 at 8:44 PM, Amit Langote
 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

Re: Boolean partitions syntax

[Amit Langote]

Hi Stephen.

On 2018/01/26 10:16, Stephen Frost wrote:
> * Amit Langote (langote_amit...@lab.ntt.co.jp) 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
>From 3ebd9387c4515cc5272e12e0138cd8035fedaaea Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 12 Dec 2017 10:33:11 +0900
Subject: [PATCH v3] Allow Boolean values in partition FOR VALUES clause

---
 doc/src/sgml/ref/create_table.sgml |  6 +++---
 src/backend/parser/gram.y  | 24 +++-
 src/test/regress/expected/create_table.out | 14 ++
 src/test/regress/sql/create_table.sql  |  7 +++
 4 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index a0c9a6d257..eaa79ae333 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -86,9 +86,9 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] 
TABLE [ IF NOT EXI
 
 and partition_bound_spec 
is:
 
-IN ( { numeric_literal | 
string_literal | NULL } [, ...] ) |
-FROM ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] )
-  TO ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] ) |
+IN ( { numeric_literal | 
string_literal | TRUE | FALSE | 
NULL } [, ...] ) |
+FROM ( { numeric_literal | 
string_literal | TRUE | FALSE | 
MINVALUE | MAXVALUE } [, ...] )
+  TO ( { numeric_literal | 
string_literal | TRUE | FALSE | 
MINVALUE | MAXVALUE } [, ...] ) |
 WITH ( MODULUS numeric_literal, 
REMAINDER numeric_literal )
 
 index_parameters in 
UNIQUE, PRIMARY KEY, and 
EXCLUDE constraints are:
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 459a227e57..15b5c85a6e 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -152,6 +152,7 @@ static Node *makeFloatConst(char *str, int location);
 static Node *makeBitStringConst(char *str, int location);
 static Node *makeNullAConst(int location);
 static Node *makeAConst(Value *v, int location);
+static Node *makeBoolAConstCast(bool state, int location);
 static Node *makeBoolAConst(bool state, int location);
 static RoleSpec *makeRoleSpec(RoleSpecType type, int location);
 static void check_qualified_name(List *names, core_yyscan_t yyscanner);
@@ -2793,6 +2794,8 @@ partbound_datum:
Sconst

Re: Boolean partitions syntax

[Stephen Frost]

Greetings Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> On 2017/12/20 6:46, Mark Dilger wrote:
> >> On Dec 12, 2017, at 10:32 PM, Amit Langote  
> >> 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
Description: PGP signature

Re: Boolean partitions syntax

[Amit Langote]

Hi Mark,

On 2017/12/20 6:46, Mark Dilger wrote:
>> On Dec 12, 2017, at 10:32 PM, Amit Langote  
>> 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

Re: Boolean partitions syntax

[Mark Dilger]

> On Dec 12, 2017, at 10:32 PM, Amit Langote  
> 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),

Re: Boolean partitions syntax

[Peter Eisentraut]

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

Re: Boolean partitions syntax

[Amit Langote]

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

Re: Boolean partitions syntax

[Ashutosh Bapat]

On Tue, Dec 12, 2017 at 7:19 AM, Amit Langote
 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

Re: Boolean partitions syntax

[Amit Langote]

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
From 318c815264b27fcbda5b83e542c6a2970f714399 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 12 Dec 2017 10:33:11 +0900
Subject: [PATCH] Allow Boolean values in partition FOR VALUES clause

---
 doc/src/sgml/ref/create_table.sgml |  6 +++---
 src/backend/parser/gram.y  | 16 +++-
 src/test/regress/expected/create_table.out | 14 ++
 src/test/regress/sql/create_table.sql  |  7 +++
 4 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index a0c9a6d257..eaa79ae333 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -86,9 +86,9 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] 
TABLE [ IF NOT EXI
 
 and partition_bound_spec 
is:
 
-IN ( { numeric_literal | 
string_literal | NULL } [, ...] ) |
-FROM ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] )
-  TO ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] ) |
+IN ( { numeric_literal | 
string_literal | TRUE | FALSE | 
NULL } [, ...] ) |
+FROM ( { numeric_literal | 
string_literal | TRUE | FALSE | 
MINVALUE | MAXVALUE } [, ...] )
+  TO ( { numeric_literal | 
string_literal | TRUE | FALSE | 
MINVALUE | MAXVALUE } [, ...] ) |
 WITH ( MODULUS numeric_literal, 
REMAINDER numeric_literal )
 
 index_parameters in 
UNIQUE, PRIMARY KEY, and 
EXCLUDE constraints are:
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index ebfc94f896..7dfa2759a4 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -153,6 +153,7 @@ static Node *makeBitStringConst(char *str, int location);
 static Node *makeNullAConst(int location);
 static Node *makeAConst(Value *v, int location);
 static Node *makeBoolAConst(bool state, int location);
+static Node *makeBoolAConstNoCast(bool state, int location);
 static RoleSpec *makeRoleSpec(RoleSpecType type, int location);
 static void check_qualified_name(List *names, core_yyscan_t yyscanner);
 static List *check_func_name(List *names, core_yyscan_t yyscanner);
@@ -2768,6 +2769,8 @@ partbound_datum:
Sconst  { $$ = makeStringConst($1, @1); 
}
| NumericOnly   { $$ = makeAConst($1, @1); }
| NULL_P{ $$ = makeNullAConst(@1); }
+   | TRUE_P{ $$ = 
makeBoolAConstNoCast(true, @1); }
+   | FALSE_P   { $$ = 
makeBoolAConstNoCast(false, @1); }
;
 
 partbound_datum_list:
@@ -15581,13 +15584,24 @@ makeAConst(Value *v, int location)
 static Node *
 makeBoolAConst(bool state, int location)
 {
+   A_Const *n = (A_Const *) makeBoolAConstNoCast(state, location);
+
+   return makeTypeCast((Node *)n, SystemTypeName("bool"), -1);
+}
+
+/* makeBoolAConstNoCast()
+ * Create an A_Const string node containing valid bool type values.
+ */
+static Node *
+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 makeTypeCast((Node *)n, SystemTypeName("bool"), -1);
+   return (Node *) n;
 }
 
 /* makeRoleSpec
diff --git a/src/test/regress/expected/create_table.out 
b/src/test/regress/expected/create_table.out
index 8e745402ae..c541a652c4 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -863,3 +863,17 @@ Partition key: LIST (a)
 Number of partitions: 0
 
 DROP TABLE parted_col_comment;
+-- boolean partitions
+create table boolspart (a bool) 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   |  | 
+Partition key: LIST (a)
+Partitions: boolspart_f FOR VALUES IN (false),
+boolspart_t FOR VALUES IN (true)
+
+drop table boolspart;
diff --git a/src/test/regress/sql/create_table.sql 
b/src/test/regress/sql/create_table.sql
index 8f9991ef18..c71e9f938e 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -707,3 +707,10 @@ COMMENT ON COLUMN parted_col_comment.a IS 'Partition key';
 SELECT obj_description('parted_col_comment'::regclass);
 \d+ parted_col_comment
 DROP TABLE parted_col_comment;
+
+-- boolean 

Re: Boolean partitions syntax

[Amit Langote]

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
From 85d51048ee147c819a9ed0648557c7f05f3802c5 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 12 Dec 2017 10:33:11 +0900
Subject: [PATCH] Allow Boolean values in partition FOR VALUES clause

---
 doc/src/sgml/ref/create_table.sgml |  6 +++---
 src/backend/parser/gram.y  | 18 ++
 src/test/regress/expected/create_table.out | 14 ++
 src/test/regress/sql/create_table.sql  |  7 +++
 4 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index a0c9a6d257..eaa79ae333 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -86,9 +86,9 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] 
TABLE [ IF NOT EXI
 
 and partition_bound_spec 
is:
 
-IN ( { numeric_literal | 
string_literal | NULL } [, ...] ) |
-FROM ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] )
-  TO ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] ) |
+IN ( { numeric_literal | 
string_literal | TRUE | FALSE | 
NULL } [, ...] ) |
+FROM ( { numeric_literal | 
string_literal | TRUE | FALSE | 
MINVALUE | MAXVALUE } [, ...] )
+  TO ( { numeric_literal | 
string_literal | TRUE | FALSE | 
MINVALUE | MAXVALUE } [, ...] ) |
 WITH ( MODULUS numeric_literal, 
REMAINDER numeric_literal )
 
 index_parameters in 
UNIQUE, PRIMARY KEY, and 
EXCLUDE constraints are:
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index ebfc94f896..c6cbf9b844 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -153,6 +153,7 @@ static Node *makeBitStringConst(char *str, int location);
 static Node *makeNullAConst(int location);
 static Node *makeAConst(Value *v, int location);
 static Node *makeBoolAConst(bool state, int location);
+static Node *makeBoolAConstNoCast(bool state, int location);
 static RoleSpec *makeRoleSpec(RoleSpecType type, int location);
 static void check_qualified_name(List *names, core_yyscan_t yyscanner);
 static List *check_func_name(List *names, core_yyscan_t yyscanner);
@@ -2768,6 +2769,8 @@ partbound_datum:
Sconst  { $$ = makeStringConst($1, @1); 
}
| NumericOnly   { $$ = makeAConst($1, @1); }
| NULL_P{ $$ = makeNullAConst(@1); }
+   | TRUE_P{ $$ = 
makeBoolAConstNoCast(true, @1); }
+   | FALSE_P   { $$ = 
makeBoolAConstNoCast(false, @1); }
;
 
 partbound_datum_list:
@@ -15590,6 +15593,21 @@ makeBoolAConst(bool state, int location)
return makeTypeCast((Node *)n, SystemTypeName("bool"), -1);
 }
 
+/* makeBoolAConstNoCast()
+ * Create an A_Const string node containing valid bool type values.
+ */
+static Node *
+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;
+}
+
 /* makeRoleSpec
  * Create a RoleSpec with the given type
  */
diff --git a/src/test/regress/expected/create_table.out 
b/src/test/regress/expected/create_table.out
index 8e745402ae..c541a652c4 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -863,3 +863,17 @@ 

Boolean partitions syntax

[Amit Langote]

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
From 85d51048ee147c819a9ed0648557c7f05f3802c5 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 12 Dec 2017 10:33:11 +0900
Subject: [PATCH] Allow Boolean values in partition FOR VALUES clause

---
 doc/src/sgml/ref/create_table.sgml |  6 +++---
 src/backend/parser/gram.y  | 18 ++
 src/test/regress/expected/create_table.out | 14 ++
 src/test/regress/sql/create_table.sql  |  7 +++
 4 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index a0c9a6d257..eaa79ae333 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -86,9 +86,9 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] 
TABLE [ IF NOT EXI
 
 and partition_bound_spec 
is:
 
-IN ( { numeric_literal | 
string_literal | NULL } [, ...] ) |
-FROM ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] )
-  TO ( { numeric_literal | 
string_literal | MINVALUE | 
MAXVALUE } [, ...] ) |
+IN ( { numeric_literal | 
string_literal | TRUE | FALSE | 
NULL } [, ...] ) |
+FROM ( { numeric_literal | 
string_literal | TRUE | FALSE | 
MINVALUE | MAXVALUE } [, ...] )
+  TO ( { numeric_literal | 
string_literal | TRUE | FALSE | 
MINVALUE | MAXVALUE } [, ...] ) |
 WITH ( MODULUS numeric_literal, 
REMAINDER numeric_literal )
 
 index_parameters in 
UNIQUE, PRIMARY KEY, and 
EXCLUDE constraints are:
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index ebfc94f896..c6cbf9b844 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -153,6 +153,7 @@ static Node *makeBitStringConst(char *str, int location);
 static Node *makeNullAConst(int location);
 static Node *makeAConst(Value *v, int location);
 static Node *makeBoolAConst(bool state, int location);
+static Node *makeBoolAConstNoCast(bool state, int location);
 static RoleSpec *makeRoleSpec(RoleSpecType type, int location);
 static void check_qualified_name(List *names, core_yyscan_t yyscanner);
 static List *check_func_name(List *names, core_yyscan_t yyscanner);
@@ -2768,6 +2769,8 @@ partbound_datum:
Sconst  { $$ = makeStringConst($1, @1); 
}
| NumericOnly   { $$ = makeAConst($1, @1); }
| NULL_P{ $$ = makeNullAConst(@1); }
+   | TRUE_P{ $$ = 
makeBoolAConstNoCast(true, @1); }
+   | FALSE_P   { $$ = 
makeBoolAConstNoCast(false, @1); }
;
 
 partbound_datum_list:
@@ -15590,6 +15593,21 @@ makeBoolAConst(bool state, int location)
return makeTypeCast((Node *)n, SystemTypeName("bool"), -1);
 }
 
+/* makeBoolAConstNoCast()
+ * Create an A_Const string node containing valid bool type values.
+ */
+static Node *
+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;
+}
+
 /* makeRoleSpec
  * Create a RoleSpec with the given type
  */
diff --git a/src/test/regress/expected/create_table.out 
b/src/test/regress/expected/create_table.out
index 8e745402ae..c541a652c4 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -863,3 +863,17 @@ Partition key: LIST (a)
 Number of partitions: 0
 
 DROP TABLE parted_col_comment;
+-- boolean partitions
+create table boolspart (a bool) 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   |