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 <amitlangot...@gmail.com>
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
<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">numeric_literal</replaceable> |
<replaceable class="parameter">string_literal</replaceable> | TRUE | FALSE |
NULL } [, ...] ) |
+FROM ( { <replaceable class="parameter">numeric_literal</replaceable> |
<replaceable class="parameter">string_literal</replaceable> | TRUE | FALSE |
MINVALUE | MAXVALUE } [, ...] )
+ TO ( { <replaceable class="parameter">numeric_literal</replaceable> |
<replaceable class="parameter">string_literal</replaceable> | TRUE | FALSE |
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>
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 { $$ = makeStringConst($1, @1);
}
| NumericOnly { $$ = makeAConst($1, @1); }
| NULL_P { $$ = makeNullAConst(@1); }
+ | TRUE_P { $$ = makeBoolAConst(true,
@1); }
+ | FALSE_P { $$ = makeBoolAConst(false,
@1); }
;
partbound_datum_list:
@@ -13826,7 +13829,7 @@ func_expr_common_subexpr:
{
XmlExpr *x = (XmlExpr *)
makeXmlExpr(IS_XMLPARSE, NULL,
NIL,
-
list_make2($4, makeBoolAConst($5, -1)),
+
list_make2($4, makeBoolAConstCast($5, -1)),
@1);
x->xmloption = $3;
$$ = (Node *)x;
@@ -14777,11 +14780,11 @@ AexprConst: Iconst
}
| TRUE_P
{
- $$ = makeBoolAConst(true, @1);
+ $$ = makeBoolAConstCast(true, @1);
}
| FALSE_P
{
- $$ = makeBoolAConst(false, @1);
+ $$ = makeBoolAConstCast(false, @1);
}
| NULL_P
{
@@ -15597,10 +15600,21 @@ makeAConst(Value *v, int location)
return n;
}
-/* makeBoolAConst()
+/* makeBoolAConstCast()
* Create an A_Const string node and put it inside a boolean cast.
*/
static Node *
+makeBoolAConstCast(bool state, int location)
+{
+ A_Const *n = (A_Const *) makeBoolAConst(state, location);
+
+ return makeTypeCast((Node *)n, SystemTypeName("bool"), -1);
+}
+
+/* makeBoolAConst()
+ * Create an A_Const string node containing valid bool type values.
+ */
+static Node *
makeBoolAConst(bool state, int location)
{
A_Const *n = makeNode(A_Const);
@@ -15609,7 +15623,7 @@ makeBoolAConst(bool state, int location)
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 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
