On 2018/01/27 1:31, Tom Lane wrote:
> Robert Haas <[email protected]> writes:
>> On Fri, Jan 26, 2018 at 11:07 AM, Stephen Frost <[email protected]> wrote:
>>> I've already had two people mention that it'd be neat to have PG support
>>> it, so I don't believe it'd go unused. As for if we should force people
>>> to use quotes, my vote would be no because we don't require that for
>>> other usage of true/false in the parser and I don't see any reason why
>>> this should be different.
>
>> OK. Let's wait a bit and see if anyone else wants to weigh in.
>
> I dunno, this patch seems quite bizarre to me. IIUC, it results in
> TRUE/FALSE behaving differently in a partbound_datum than they do
> anywhere else in the grammar, to wit that they're effectively just
> another spelling for the undecorated literals 't' and 'f', without
> anything indicating that they're boolean. That seems wrong from a
> data typing standpoint. And even if it's really true that we'd
> rather lose type information for partbound literals, a naive observer
> would probably think that these should decay to the strings "true"
> and "false" not "t" and "f" (cf. opt_boolean_or_string).
Partition bound literals as captured gram.y don't have any type
information attached. They're carried over in a A_Const to
transformPartitionBoundValue() and coerced to the target partition key
type there. Note that each of the the partition bound literal datums
received from gram.y is castNode(A_Const)'d in parse_utilcmds.c.
I agree that it's better to simply makeStringConst("true"/"false") for
TRUE_P and FALSE_P, instead of makingBoolAConst(true/false).
> I've not paid any attention to this thread up to now, so maybe there's
> a rational explanation for this choice that I missed. But mucking
> with makeBoolAConst like that doesn't seem to me to pass the smell
> test. I'm on board with the stated goal of allowing TRUE/FALSE here,
> but having to contort the grammar output like this suggests that
> there's something wrong in parse analysis of partbound_datum.
Attached updated patch doesn't change anything about makeBoolAConst and
now is just a 2-line change to gram.y.
> PS: the proposed documentation wording is too verbose by half.
> I'd just cut it down to "<literal constant>".
Yeah, I was getting nervous about the lines in syntax synopsis getting
unwieldily long after this change. I changed all of them to use
literal_constant for anything other than special keywords MINVALUE and
MAXVALUE and a paragraph in the description to clarify.
Attached updated patch. Thanks for the comments.
Regards,
Amit
From 392abca09192218a73066fd41131819963daf1a4 Mon Sep 17 00:00:00 2001
From: amit <[email protected]>
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
<phrase>and <replaceable class="parameter">partition_bound_spec</replaceable>
is:</phrase>
-IN ( { <replaceable class="parameter">numeric_literal</replaceable> |
<replaceable class="parameter">string_literal</replaceable> | NULL } [, ...] ) |
-FROM ( { <replaceable class="parameter">numeric_literal</replaceable> |
<replaceable class="parameter">string_literal</replaceable> | MINVALUE |
MAXVALUE } [, ...] )
- TO ( { <replaceable class="parameter">numeric_literal</replaceable> |
<replaceable class="parameter">string_literal</replaceable> | MINVALUE |
MAXVALUE } [, ...] ) |
+IN ( { <replaceable class="parameter">literal_constant</replaceable> } [, ...]
) |
+FROM ( { <replaceable class="parameter">literal_constant</replaceable> |
MINVALUE | MAXVALUE } [, ...] )
+ TO ( { <replaceable class="parameter">literal_constant</replaceable> |
MINVALUE | MAXVALUE } [, ...] ) |
WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>,
REMAINDER <replaceable class="parameter">numeric_literal</replaceable> )
<phrase><replaceable class="parameter">index_parameters</replaceable> in
<literal>UNIQUE</literal>, <literal>PRIMARY KEY</literal>, and
<literal>EXCLUDE</literal> constraints are:</phrase>
@@ -274,10 +274,10 @@ WITH ( MODULUS <replaceable
class="parameter">numeric_literal</replaceable>, REM
<para>
Each of the values specified in
the <replaceable class="parameter">partition_bound_spec</replaceable> is
- a literal, <literal>NULL</literal>, <literal>MINVALUE</literal>, or
- <literal>MAXVALUE</literal>. 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, <literal>MINVALUE</literal>, or
+ <literal>MAXVALUE</literal>. 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.
</para>
<para>
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 { $$ = makeNullAConst(@1); }
+ | TRUE_P { $$ = makeStringConst("true",
@1); }
+ | FALSE_P { $$ = makeStringConst("false",
@1); }
;
partbound_datum_list:
diff --git a/src/test/regress/expected/create_table.out
b/src/test/regress/expected/create_table.out
index ef0906776e..ec8525f471 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -868,3 +868,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 10e5d49e8e..1e5768d178 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -711,3 +711,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 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
+drop table boolspart;
--
2.11.0
