On 2018/01/27 1:31, Tom Lane wrote: > Robert Haas <robertmh...@gmail.com> writes: >> On Fri, Jan 26, 2018 at 11:07 AM, Stephen Frost <sfr...@snowman.net> 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 <amitlangot...@gmail.com> 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