Обсуждение: Boolean partitions syntax
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
Вложения
On Tue, Dec 12, 2017 at 7:19 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Hi.
Horiguchi-san pointed out [1] on a nearby thread that the partitioning
syntax (the FOR VALUES clause) doesn't accept true and false as valid
partition bound datums, which seems to me like an oversight. Attached a
patch to fix that.
create table bools (a bool) partition by list (a);
Before patch:
create table bools_t partition of bools for values in (true);
ERROR: syntax error at or near "true"
LINE 1: ...reate table bools_t partition of bools for values in (true);
After:
create table bools_t partition of bools for values in (true);
CREATE TABLE
\d bools_t
Table "public.bools_t"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | boolean | | |
Partition of: bools FOR VALUES IN (true)
Thanks,
Amit
[1]
https://www.postgresql.org/message-id/20171128.203915. 26713586.horiguchi.kyotaro% 40lab.ntt.co.jp
+makeBoolAConstNoCast(bool state, int location)
+{
+ A_Const *n = makeNode(A_Const);
+
+ n->val.type = T_String;
+ n->val.val.str = (state ? "t" : "f");
+ n->location = location;
+
+ return (Node *) n;
+}
+
I think we can change makeBoolAConst as below so that we can avoid duplicate code.
static Node *
makeBoolAConst(bool state, int location)
{
Node *n = makeBoolAConstNoCast(state, location);
return makeTypeCast(n, SystemTypeName("bool"), -1);
}
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
Вложения
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
Вложения
On Tue, Dec 12, 2017 at 7:19 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
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
On Tue, Dec 12, 2017 at 3:13 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > On 2017/12/12 18:12, Ashutosh Bapat wrote: >> On Tue, Dec 12, 2017 at 7:19 AM, Amit Langote wrote: >>> Horiguchi-san pointed out [1] on a nearby thread that the partitioning >>> syntax (the FOR VALUES clause) doesn't accept true and false as valid >>> partition bound datums, which seems to me like an oversight. Attached a >>> patch to fix that. >> >> May be you should use opt_boolean_or_string instead of TRUE_P and >> FALSE_P. It also supports ON and OFF, which will be bonus. > > Thanks for the suggestion. I tried that but NonReservedWord_or_Sconst > conflicts with Sconst that partbound_datum itself has a rule for, > resulting in the following error: > > gram.y: conflicts: 6 reduce/reduce > gram.y: expected 0 reduce/reduce conflicts > gram.y:2769.25-81: warning: rule useless in parser due to conflicts: > partbound_datum: Sconst Probably that would get fixed if you remove Sconst from the partition_datum and leave NonReservedWord_or_Sconst. > > Moreover, it seems like on/off are not being accepted as valid Boolean > values like true/false are. > > insert into rp values (true); > INSERT 0 1 > insert into rp values (on); > ERROR: syntax error at or near "on" > LINE 1: insert into rp values (on); > ^ > What's going on with that? Maybe on/off values work only with SET > statements? But this is more important observation. Looks like on/off work with SET and EXPLAIN only, not in normal SQL. So probably my suggestion was wrongheaded. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
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
On 2017/12/12 15:39, Amit Langote wrote: > On 2017/12/12 15:35, Amit Langote wrote: >> Works for me, updated patch attached. > > Oops, attached the old one with the last email. > > Updated one really attached this time. Added to CF: https://commitfest.postgresql.org/16/1410/ Thanks, Amit
> On Dec 12, 2017, at 10:32 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > On 2017/12/12 15:39, Amit Langote wrote: >> On 2017/12/12 15:35, Amit Langote wrote: >>> Works for me, updated patch attached. >> >> Oops, attached the old one with the last email. >> >> Updated one really attached this time. > > Added to CF: https://commitfest.postgresql.org/16/1410/ This compiles and passes the regression tests for me. I extended your test a bit to check whether partitions over booleans are useful. Note specifically the 'explain' output, which does not seem to restrict the scan to just the relevant partitions. You could easily argue that this is beyond the scope of your patch (and therefore not your problem), but I doubt it makes much sense to have boolean partitions without planner support for skipping partitions like is done for tables partitioned over other data types. mark -- boolean partitions create table boolspart (a bool, b text) partition by list (a); create table boolspart_t partition of boolspart for values in (true); create table boolspart_f partition of boolspart for values in (false); \d+ boolspart Table "public.boolspart" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description --------+---------+-----------+----------+---------+----------+--------------+------------- a | boolean | | | | plain | | b | text | | | | extended | | Partition key: LIST (a) Partitions: boolspart_f FOR VALUES IN (false), boolspart_t FOR VALUES IN (true) insert into boolspart (a, b) values (false, 'false'); insert into boolspart (a, b) values (true, 'true'); explain select * from boolspart where a is true; QUERY PLAN --------------------------------------------------------------------- Append (cost=0.00..46.60 rows=1330 width=33) -> Seq Scan on boolspart_f (cost=0.00..23.30 rows=665 width=33) Filter: (a IS TRUE) -> Seq Scan on boolspart_t (cost=0.00..23.30 rows=665 width=33) Filter: (a IS TRUE) (5 rows) explain select * from boolspart where a is false; QUERY PLAN --------------------------------------------------------------------- Append (cost=0.00..46.60 rows=1330 width=33) -> Seq Scan on boolspart_f (cost=0.00..23.30 rows=665 width=33) Filter: (a IS FALSE) -> Seq Scan on boolspart_t (cost=0.00..23.30 rows=665 width=33) Filter: (a IS FALSE) (5 rows) drop table boolspart; create table multiboolspart (a bool, b bool, c bool, d float, e text) partition by range (a, b, c); create table multiboolspart_fff partition of multiboolspart for values from (minvalue, minvalue, minvalue) to (false, false,false); create table multiboolspart_fft partition of multiboolspart for values from (false, false, false) to (false, false, true); create table multiboolspart_ftf partition of multiboolspart for values from (false, false, true) to (false, true, false); create table multiboolspart_ftt partition of multiboolspart for values from (false, true, false) to (false, true, true); create table multiboolspart_tff partition of multiboolspart for values from (false, true, true) to (true, false, false); create table multiboolspart_tft partition of multiboolspart for values from (true, false, false) to (true, false, true); create table multiboolspart_ttf partition of multiboolspart for values from (true, false, true) to (true, true, false); create table multiboolspart_ttt partition of multiboolspart for values from (true, true, false) to (true, true, true); create table multiboolspart_max partition of multiboolspart for values from (true, true, true) to (maxvalue, maxvalue, maxvalue); \d+ multiboolspart; Table "public.multiboolspart" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description --------+------------------+-----------+----------+---------+----------+--------------+------------- a | boolean | | | | plain | | b | boolean | | | | plain | | c | boolean | | | | plain | | d | double precision | | | | plain | | e | text | | | | extended | | Partition key: RANGE (a, b, c) Partitions: multiboolspart_fff FOR VALUES FROM (MINVALUE, MINVALUE, MINVALUE) TO (false, false, false), multiboolspart_fft FOR VALUES FROM (false, false, false) TO (false, false, true), multiboolspart_ftf FOR VALUES FROM (false, false, true) TO (false, true, false), multiboolspart_ftt FOR VALUES FROM (false, true, false) TO (false, true, true), multiboolspart_max FOR VALUES FROM (true, true, true) TO (MAXVALUE, MAXVALUE, MAXVALUE), multiboolspart_tff FOR VALUES FROM (false, true, true) TO (true, false, false), multiboolspart_tft FOR VALUES FROM (true, false, false) TO (true, false, true), multiboolspart_ttf FOR VALUES FROM (true, false, true) TO (true, true, false), multiboolspart_ttt FOR VALUES FROM (true, true, false) TO (true, true, true) insert into multiboolspart (a, b, c, d, e) values (true, false, true, 1.7, 'hello'); explain select * from multiboolspart where a is true and b is false and c is true; QUERY PLAN ---------------------------------------------------------------------------- Append (cost=0.00..150.50 rows=1008 width=43) -> Seq Scan on multiboolspart_fff (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_fft (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_ftf (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_tff (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_tft (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_ttf (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_max (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS FALSE) AND (c IS TRUE)) (15 rows) explain select * from multiboolspart where a is true and b is true and c is true; QUERY PLAN ---------------------------------------------------------------------------- Append (cost=0.00..193.50 rows=1296 width=43) -> Seq Scan on multiboolspart_fff (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_fft (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_ftf (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_ftt (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_tff (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_tft (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_ttf (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_ttt (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE)) -> Seq Scan on multiboolspart_max (cost=0.00..21.50 rows=144 width=43) Filter: ((a IS TRUE) AND (b IS TRUE) AND (c IS TRUE)) (19 rows) select * from multiboolspart; a | b | c | d | e ---+---+---+-----+------- t | f | t | 1.7 | hello (1 row) drop table multiboolspart;
Hi Mark, On 2017/12/20 6:46, Mark Dilger wrote: >> On Dec 12, 2017, at 10:32 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
Greetings Amit, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > On 2017/12/20 6:46, Mark Dilger wrote: > >> On Dec 12, 2017, at 10:32 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
Вложения
Hi Stephen. On 2018/01/26 10:16, Stephen Frost wrote: > * Amit Langote (Langote_Amit_f8@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
Вложения
On Thu, Jan 25, 2018 at 8:44 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Thu, Jan 25, 2018 at 8:44 PM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> 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
Вложения
On Fri, Jan 26, 2018 at 11:07 AM, Stephen Frost <sfrost@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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jan 26, 2018 at 11:07 AM, Stephen Frost <sfrost@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).
I've not paid any attention to this thread up to now, so maybe there's
a rational explanation for this choice that I missed. But mucking
with makeBoolAConst like that doesn't seem to me to pass the smell
test. I'm on board with the stated goal of allowing TRUE/FALSE here,
but having to contort the grammar output like this suggests that
there's something wrong in parse analysis of partbound_datum.
regards, tom lane
PS: the proposed documentation wording is too verbose by half.
I'd just cut it down to "<literal constant>".
On 2018/01/27 0:30, Robert Haas wrote: > On Thu, Jan 25, 2018 at 8:44 PM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> 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
On 2018/01/27 1:31, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Jan 26, 2018 at 11:07 AM, Stephen Frost <sfrost@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
Вложения
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
On 2018/01/29 14:57, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
On 2/2/18 04:39, Amit Langote wrote: > 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? Without getting into the technicial details too much, this thread surprised me. Why do we need to add support for each kind of constant literal separately? That seems wrong. 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. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
On Fri, Feb 2, 2018 at 4:40 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Feb 2, 2018 at 4:40 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> 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
Hello,
At Fri, 02 Feb 2018 18:04:44 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in <14732.1517612684@sss.pgh.pa.us>
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Fri, Feb 2, 2018 at 4:40 PM, Peter Eisentraut
> > <peter.eisentraut@2ndquadrant.com> 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 <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<6844d7f9-8219-a9ff-88f9-82c05fc90d70@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 a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index b2f5e46..d1f9b02 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -1846,6 +1846,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_PARTITION_EXPRESSION:
err = _("cannot use subquery in partition key expression");
break;
+ case EXPR_KIND_PARTITION_BOUNDS:
+ err = _("cannot use subquery in partition bounds value");
+ break;
/*
* There is intentionally no default: case here, so that the
@@ -3468,6 +3471,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:
return "CALL";
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index ffae0f3..9e83ffe 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2289,6 +2289,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
case EXPR_KIND_PARTITION_EXPRESSION:
err = _("set-returning functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUNDS:
+ err = _("set-returning functions are not allowed in partition bounds value");
+ break;
case EXPR_KIND_CALL:
err = _("set-returning functions are not allowed in CALL arguments");
break;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index d415d71..0b05dda 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -134,7 +134,7 @@ static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
static void setSchemaName(char *context_schema, char **stmt_schema_name);
static void transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd);
static void validateInfiniteBounds(ParseState *pstate, List *blist);
-static Const *transformPartitionBoundValue(ParseState *pstate, A_Const *con,
+static Const *transformPartitionBoundValue(ParseState *pstate, Node *con,
const char *colName, Oid colType, int32 colTypmod);
@@ -3420,12 +3420,12 @@ transformPartitionBound(ParseState *pstate, Relation parent,
result_spec->listdatums = NIL;
foreach(cell, spec->listdatums)
{
- A_Const *con = castNode(A_Const, lfirst(cell));
+ Node *expr = (Node *)lfirst (cell);
Const *value;
ListCell *cell2;
bool duplicate;
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate, expr,
colname, coltype, coltypmod);
/* Don't add to the result if the value is a duplicate */
@@ -3486,7 +3486,6 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
- A_Const *con;
Const *value;
/* Get the column's name in case we need to output an error */
@@ -3507,8 +3506,8 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (ldatum->value)
{
- con = castNode(A_Const, ldatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ ldatum->value,
colname,
coltype, coltypmod);
if (value->constisnull)
@@ -3521,8 +3520,8 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (rdatum->value)
{
- con = castNode(A_Const, rdatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ rdatum->value,
colname,
coltype, coltypmod);
if (value->constisnull)
@@ -3591,13 +3590,13 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
* Transform one constant in a partition bound spec
*/
static Const *
-transformPartitionBoundValue(ParseState *pstate, A_Const *con,
+transformPartitionBoundValue(ParseState *pstate, Node *val,
const char *colName, Oid colType, int32 colTypmod)
{
Node *value;
/* Make it into a Const */
- value = (Node *) make_const(pstate, &con->val, con->location);
+ value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUNDS);
/* Coerce to correct type */
value = coerce_to_target_type(pstate,
@@ -3613,7 +3612,7 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("specified value cannot be cast to type %s for column \"%s\"",
format_type_be(colType), colName),
- parser_errposition(pstate, con->location)));
+ parser_errposition(pstate, exprLocation(val))));
/* Simplify the expression, in case we had a coercion */
if (!IsA(value, Const))
@@ -3627,7 +3626,7 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con,
format_type_be(colType), colName),
errdetail("The cast requires a non-immutable conversion."),
errhint("Try putting the literal value in single quotes."),
- parser_errposition(pstate, con->location)));
+ parser_errposition(pstate, exprLocation(val))));
return (Const *) value;
}
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index 4e96fa7..23451cb 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -68,6 +68,7 @@ typedef enum ParseExprKind
EXPR_KIND_TRIGGER_WHEN, /* WHEN condition in CREATE TRIGGER */
EXPR_KIND_POLICY, /* USING or WITH CHECK expr in policy */
EXPR_KIND_PARTITION_EXPRESSION, /* PARTITION BY expression */
+ EXPR_KIND_PARTITION_BOUNDS, /* partition bounds value */
EXPR_KIND_CALL /* CALL argument */
} ParseExprKind;
On 2018-02-02 17:00:24 -0500, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
On 2018/03/02 15:58, Andres Freund wrote: > On 2018-02-02 17:00:24 -0500, Tom Lane wrote: >> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
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
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 <peter.eisentraut@2ndquadrant.com> 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 david@pgmasters.net
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 <peter.eisentraut@2ndquadrant.com> 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 david@pgmasters.net
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 <peter.eisentraut@2ndquadrant.com> 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
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 <peter.eisentraut@2ndquadrant.com> 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 david@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_o6tha9NJw@mail.gmail.com
On Mar 21, 2018, at 10:59 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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 <peter.eisentraut@2ndquadrant.com> 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
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
> On Apr 9, 2018, at 8:28 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
"Jonathan S. Katz" <jonathan.katz@excoventures.com> 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
> On Apr 9, 2018, at 10:06 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > "Jonathan S. Katz" <jonathan.katz@excoventures.com> 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
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" <jonathan.katz@excoventures.com> wrote in
<27021281-2ED7-4CDE-9D82-366AF10B3B57@excoventures.com>
> > On Apr 9, 2018, at 10:06 AM, Tom Lane <tgl@sss.pgh.pa.us> 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 <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<2d49cd86-acb9-fc5b-8eb7-e467b01ec25a@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 EXPR_KIND_PARTITION_EXPRESSION:
err = _("set-returning functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUNDS:
+ err = _("set-returning functions are not allowed in partition bounds");
+ break;
case EXPR_KIND_CALL_ARGUMENT:
err = _("set-returning functions are not allowed in CALL arguments");
break;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index f9f9904bad..c14f1265a9 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -138,7 +138,7 @@ static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
static void setSchemaName(char *context_schema, char **stmt_schema_name);
static void transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd);
static void validateInfiniteBounds(ParseState *pstate, List *blist);
-static Const *transformPartitionBoundValue(ParseState *pstate, A_Const *con,
+static Const *transformPartitionBoundValue(ParseState *pstate, Node *con,
const char *colName, Oid colType, int32 colTypmod);
@@ -3674,12 +3674,12 @@ transformPartitionBound(ParseState *pstate, Relation parent,
result_spec->listdatums = NIL;
foreach(cell, spec->listdatums)
{
- A_Const *con = castNode(A_Const, lfirst(cell));
+ Node *expr = (Node *)lfirst (cell);
Const *value;
ListCell *cell2;
bool duplicate;
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate, expr,
colname, coltype, coltypmod);
/* Don't add to the result if the value is a duplicate */
@@ -3740,7 +3740,6 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
- A_Const *con;
Const *value;
/* Get the column's name in case we need to output an error */
@@ -3761,8 +3760,8 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (ldatum->value)
{
- con = castNode(A_Const, ldatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ ldatum->value,
colname,
coltype, coltypmod);
if (value->constisnull)
@@ -3775,8 +3774,8 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (rdatum->value)
{
- con = castNode(A_Const, rdatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ rdatum->value,
colname,
coltype, coltypmod);
if (value->constisnull)
@@ -3845,13 +3844,13 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
* Transform one constant in a partition bound spec
*/
static Const *
-transformPartitionBoundValue(ParseState *pstate, A_Const *con,
+transformPartitionBoundValue(ParseState *pstate, Node *val,
const char *colName, Oid colType, int32 colTypmod)
{
Node *value;
/* Make it into a Const */
- value = (Node *) make_const(pstate, &con->val, con->location);
+ value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUNDS);
/* Coerce to correct type */
value = coerce_to_target_type(pstate,
@@ -3867,7 +3866,7 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("specified value cannot be cast to type %s for column \"%s\"",
format_type_be(colType), colName),
- parser_errposition(pstate, con->location)));
+ parser_errposition(pstate, exprLocation(val))));
/* Simplify the expression, in case we had a coercion */
if (!IsA(value, Const))
@@ -3881,7 +3880,7 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con,
format_type_be(colType), colName),
errdetail("The cast requires a non-immutable conversion."),
errhint("Try putting the literal value in single quotes."),
- parser_errposition(pstate, con->location)));
+ parser_errposition(pstate, exprLocation(val))));
return (Const *) value;
}
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index 3fd2151ccb..5eccf5078a 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -70,6 +70,7 @@ typedef enum ParseExprKind
EXPR_KIND_TRIGGER_WHEN, /* WHEN condition in CREATE TRIGGER */
EXPR_KIND_POLICY, /* USING or WITH CHECK expr in policy */
EXPR_KIND_PARTITION_EXPRESSION, /* PARTITION BY expression */
+ EXPR_KIND_PARTITION_BOUNDS, /* partition bounds value */
EXPR_KIND_CALL_ARGUMENT /* procedure argument in CALL */
} ParseExprKind;
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 <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20180410.103427.244142052.horiguchi.kyotaro@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 <node> columnDef columnOptions
%type <defelt> def_elem reloption_elem old_aggr_elem operator_def_elem
%type <node> 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 <list> rowsfrom_item rowsfrom_list opt_col_def_list
@@ -585,7 +587,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <partelem> part_elem
%type <list> part_params
%type <partboundspec> PartitionBoundSpec
-%type <node> partbound_datum PartitionRangeDatum
+%type <node> PartitionRangeDatum
%type <list> hash_partbound partbound_datum_list range_datum_list
%type <defelt> 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: AexprConst { $$ = $1; }
| PARAM opt_indirection
{
ParamRef *p = makeNode(ParamRef);
@@ -16275,6 +16271,18 @@ makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner)
return r;
}
+static Node *
+makePartRangeDatum(PartitionRangeDatumKind kind, Node *value, int location)
+{
+ PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
+
+ n->kind = kind;
+ n->value = value;
+ n->location = location;
+
+ return (Node *) n;
+}
+
/* Separate Constraint nodes from COLLATE clauses in a ColQualList */
static void
SplitColQualList(List *qualList,
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 EXPR_KIND_PARTITION_EXPRESSION:
err = _("set-returning functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUNDS:
+ err = _("set-returning functions are not allowed in partition bounds");
+ break;
case EXPR_KIND_CALL_ARGUMENT:
err = _("set-returning functions are not allowed in CALL arguments");
break;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index f9f9904bad..c14f1265a9 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -138,7 +138,7 @@ static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
static void setSchemaName(char *context_schema, char **stmt_schema_name);
static void transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd);
static void validateInfiniteBounds(ParseState *pstate, List *blist);
-static Const *transformPartitionBoundValue(ParseState *pstate, A_Const *con,
+static Const *transformPartitionBoundValue(ParseState *pstate, Node *con,
const char *colName, Oid colType, int32 colTypmod);
@@ -3674,12 +3674,12 @@ transformPartitionBound(ParseState *pstate, Relation parent,
result_spec->listdatums = NIL;
foreach(cell, spec->listdatums)
{
- A_Const *con = castNode(A_Const, lfirst(cell));
+ Node *expr = (Node *)lfirst (cell);
Const *value;
ListCell *cell2;
bool duplicate;
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate, expr,
colname, coltype, coltypmod);
/* Don't add to the result if the value is a duplicate */
@@ -3740,7 +3740,6 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
- A_Const *con;
Const *value;
/* Get the column's name in case we need to output an error */
@@ -3761,8 +3760,8 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (ldatum->value)
{
- con = castNode(A_Const, ldatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ ldatum->value,
colname,
coltype, coltypmod);
if (value->constisnull)
@@ -3775,8 +3774,8 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (rdatum->value)
{
- con = castNode(A_Const, rdatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ rdatum->value,
colname,
coltype, coltypmod);
if (value->constisnull)
@@ -3845,13 +3844,13 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
* Transform one constant in a partition bound spec
*/
static Const *
-transformPartitionBoundValue(ParseState *pstate, A_Const *con,
+transformPartitionBoundValue(ParseState *pstate, Node *val,
const char *colName, Oid colType, int32 colTypmod)
{
Node *value;
/* Make it into a Const */
- value = (Node *) make_const(pstate, &con->val, con->location);
+ value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUNDS);
/* Coerce to correct type */
value = coerce_to_target_type(pstate,
@@ -3867,7 +3866,7 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("specified value cannot be cast to type %s for column \"%s\"",
format_type_be(colType), colName),
- parser_errposition(pstate, con->location)));
+ parser_errposition(pstate, exprLocation(val))));
/* Simplify the expression, in case we had a coercion */
if (!IsA(value, Const))
@@ -3881,7 +3880,7 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con,
format_type_be(colType), colName),
errdetail("The cast requires a non-immutable conversion."),
errhint("Try putting the literal value in single quotes."),
- parser_errposition(pstate, con->location)));
+ parser_errposition(pstate, exprLocation(val))));
return (Const *) value;
}
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index 3fd2151ccb..5eccf5078a 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -70,6 +70,7 @@ typedef enum ParseExprKind
EXPR_KIND_TRIGGER_WHEN, /* WHEN condition in CREATE TRIGGER */
EXPR_KIND_POLICY, /* USING or WITH CHECK expr in policy */
EXPR_KIND_PARTITION_EXPRESSION, /* PARTITION BY expression */
+ EXPR_KIND_PARTITION_BOUNDS, /* partition bounds value */
EXPR_KIND_CALL_ARGUMENT /* procedure argument in CALL */
} ParseExprKind;
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index e724439037..ac71d17d2c 100644
--- 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
CREATE TABLE partitioned (
a int
) PARTITION BY LIST ((a LIKE (SELECT 1)));
@@ -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 overlap partition "part_1"
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
-ERROR: syntax error at or near "::"
-LINE 1: ...fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
- ^
+ERROR: partition "fail_part" would overlap partition "part_1"
-- syntax does not allow empty list of values for list partitions
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ();
ERROR: syntax error at or near ")"
On 3 February 2018 at 12:04, Tom Lane <tgl@sss.pgh.pa.us> 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
On 10 April 2018 at 23:13, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> 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 <horiguchi.kyotaro@lab.ntt.co.jp> wrote
in<20180410.103427.244142052.horiguchi.kyotaro@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
David Rowley <david.rowley@2ndquadrant.com> 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
On 11 April 2018 at 03:34, Tom Lane <tgl@sss.pgh.pa.us> wrote: > David Rowley <david.rowley@2ndquadrant.com> 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
David Rowley <david.rowley@2ndquadrant.com> writes:
> On 11 April 2018 at 03:34, Tom Lane <tgl@sss.pgh.pa.us> 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
> On Apr 10, 2018, at 1:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > David Rowley <david.rowley@2ndquadrant.com> writes: >> On 11 April 2018 at 03:34, Tom Lane <tgl@sss.pgh.pa.us> 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
At Wed, 11 Apr 2018 02:33:58 +1200, David Rowley <david.rowley@2ndquadrant.com> wrote in
<CAKJS1f8QAF8bT7ixF21ScE8M3CN0c37xE5PT4XEvnthxete5Ng@mail.gmail.com>
> On 3 February 2018 at 12:04, Tom Lane <tgl@sss.pgh.pa.us> 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
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> 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
On 2018/04/11 10:44, Tom Lane wrote:
> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> 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
On 2018/04/10 23:37, David Rowley wrote:
> On 10 April 2018 at 23:13, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> 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
At Wed, 11 Apr 2018 11:27:17 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<1810b14f-3cd7-aff5-8358-c225c0231ee9@lab.ntt.co.jp>
> On 2018/04/11 10:44, Tom Lane wrote:
> > Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> 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 <node> columnDef columnOptions
%type <defelt> def_elem reloption_elem old_aggr_elem operator_def_elem
%type <node> 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 <list> rowsfrom_item rowsfrom_list opt_col_def_list
@@ -585,7 +587,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <partelem> part_elem
%type <list> part_params
%type <partboundspec> PartitionBoundSpec
-%type <node> partbound_datum PartitionRangeDatum
+%type <node> PartitionRangeDatum
%type <list> hash_partbound partbound_datum_list range_datum_list
%type <defelt> 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
+ u_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
+ | u_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; }
+ ;
+
+/*
+ * u_expr is a subset of b_expr so as to be usable along with
+ * unreserved keywords.
+ */
+u_expr: c0_expr { $$ = $1; }
+ | b0_expr { $$ = $1; }
+ ;
+
+/* common part of b_expr and u_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 u_expr */
+c0_expr: AexprConst { $$ = $1; }
| PARAM opt_indirection
{
ParamRef *p = makeNode(ParamRef);
@@ -16275,6 +16271,18 @@ makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner)
return r;
}
+static Node *
+makePartRangeDatum(PartitionRangeDatumKind kind, Node *value, int location)
+{
+ PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
+
+ n->kind = kind;
+ n->value = value;
+ n->location = location;
+
+ return (Node *) n;
+}
+
/* Separate Constraint nodes from COLLATE clauses in a ColQualList */
static void
SplitColQualList(List *qualList,
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 EXPR_KIND_PARTITION_EXPRESSION:
err = _("set-returning functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUNDS:
+ err = _("set-returning functions are not allowed in partition bounds");
+ break;
case EXPR_KIND_CALL_ARGUMENT:
err = _("set-returning functions are not allowed in CALL arguments");
break;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index f9f9904bad..05b24e7f69 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -48,6 +48,7 @@
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
+#include "optimizer/clauses.h"
#include "optimizer/planner.h"
#include "parser/analyze.h"
#include "parser/parse_clause.h"
@@ -138,7 +139,7 @@ static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
static void setSchemaName(char *context_schema, char **stmt_schema_name);
static void transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd);
static void validateInfiniteBounds(ParseState *pstate, List *blist);
-static Const *transformPartitionBoundValue(ParseState *pstate, A_Const *con,
+static Const *transformPartitionBoundValue(ParseState *pstate, Node *con,
const char *colName, Oid colType, int32 colTypmod);
@@ -3674,12 +3675,12 @@ transformPartitionBound(ParseState *pstate, Relation parent,
result_spec->listdatums = NIL;
foreach(cell, spec->listdatums)
{
- A_Const *con = castNode(A_Const, lfirst(cell));
+ Node *expr = (Node *)lfirst (cell);
Const *value;
ListCell *cell2;
bool duplicate;
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate, expr,
colname, coltype, coltypmod);
/* Don't add to the result if the value is a duplicate */
@@ -3740,7 +3741,6 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
- A_Const *con;
Const *value;
/* Get the column's name in case we need to output an error */
@@ -3761,8 +3761,8 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (ldatum->value)
{
- con = castNode(A_Const, ldatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ ldatum->value,
colname,
coltype, coltypmod);
if (value->constisnull)
@@ -3775,8 +3775,8 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (rdatum->value)
{
- con = castNode(A_Const, rdatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ rdatum->value,
colname,
coltype, coltypmod);
if (value->constisnull)
@@ -3845,13 +3845,13 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
* Transform one constant in a partition bound spec
*/
static Const *
-transformPartitionBoundValue(ParseState *pstate, A_Const *con,
+transformPartitionBoundValue(ParseState *pstate, Node *val,
const char *colName, Oid colType, int32 colTypmod)
{
Node *value;
/* Make it into a Const */
- value = (Node *) make_const(pstate, &con->val, con->location);
+ value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUNDS);
/* Coerce to correct type */
value = coerce_to_target_type(pstate,
@@ -3867,21 +3867,28 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("specified value cannot be cast to type %s for column \"%s\"",
format_type_be(colType), colName),
- parser_errposition(pstate, con->location)));
+ parser_errposition(pstate, exprLocation(val))));
/* Simplify the expression, in case we had a coercion */
if (!IsA(value, Const))
value = (Node *) expression_planner((Expr *) value);
- /* Fail if we don't have a constant (i.e., non-immutable coercion) */
+ /* Eval if we still don't have a constant (i.e., non-immutable coercion) */
if (!IsA(value, Const))
- ereport(ERROR,
- (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("specified value cannot be cast to type %s for column \"%s\"",
- format_type_be(colType), colName),
- errdetail("The cast requires a non-immutable conversion."),
- errhint("Try putting the literal value in single quotes."),
- parser_errposition(pstate, con->location)));
+ {
+ Oid result_type;
+ int32 result_typmod;
+ Oid result_col;
+
+ /* Collect result characteristics */
+ result_type = exprType(value);
+ result_typmod = exprTypmod(value);
+ result_col = exprCollation(value);
+
+ value = (Node *)evaluate_expr((Expr *) value, result_type,
+ result_typmod, result_col);
+ Assert(IsA(value, Const));
+ }
return (Const *) value;
}
diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h
index ba4fa4b68b..4b1a5b96f8 100644
--- a/src/include/optimizer/clauses.h
+++ b/src/include/optimizer/clauses.h
@@ -85,4 +85,6 @@ extern Node *estimate_expression_value(PlannerInfo *root, Node *node);
extern Query *inline_set_returning_function(PlannerInfo *root,
RangeTblEntry *rte);
+extern Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
+ Oid result_collation);
#endif /* CLAUSES_H */
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index 3fd2151ccb..5eccf5078a 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -70,6 +70,7 @@ typedef enum ParseExprKind
EXPR_KIND_TRIGGER_WHEN, /* WHEN condition in CREATE TRIGGER */
EXPR_KIND_POLICY, /* USING or WITH CHECK expr in policy */
EXPR_KIND_PARTITION_EXPRESSION, /* PARTITION BY expression */
+ EXPR_KIND_PARTITION_BOUNDS, /* partition bounds value */
EXPR_KIND_CALL_ARGUMENT /* procedure argument in CALL */
} ParseExprKind;
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index e724439037..c1ead46a7d 100644
--- 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
CREATE TABLE partitioned (
a int
) PARTITION BY LIST ((a LIKE (SELECT 1)));
@@ -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 overlap partition "part_1"
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
-ERROR: syntax error at or near "::"
-LINE 1: ...fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
- ^
+ERROR: partition "fail_part" would overlap partition "part_1"
-- syntax does not allow empty list of values for list partitions
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ();
ERROR: syntax error at or near ")"
@@ -490,12 +486,10 @@ CREATE TABLE moneyp (
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-ERROR: specified value cannot be cast to type money for column "a"
-LINE 1: ...EATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
- ^
-DETAIL: The cast requires a non-immutable conversion.
-HINT: Try putting the literal value in single quotes.
+CREATE TABLE moneyp_11 PARTITION OF moneyp FOR VALUES IN ('11');
+CREATE TABLE moneyp_12 PARTITION OF moneyp FOR VALUES IN (to_char(12, '99')::int);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
+ERROR: relation "moneyp_10" already exists
DROP TABLE moneyp;
-- immutable cast should work, though
CREATE TABLE bigintp (
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 235bef13dc..a626bfe659 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -458,6 +458,8 @@ CREATE TABLE moneyp (
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
+CREATE TABLE moneyp_11 PARTITION OF moneyp FOR VALUES IN ('11');
+CREATE TABLE moneyp_12 PARTITION OF moneyp FOR VALUES IN (to_char(12, '99')::int);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
DROP TABLE moneyp;
On 11 April 2018 at 05:22, Tom Lane <tgl@sss.pgh.pa.us> wrote: > David Rowley <david.rowley@2ndquadrant.com> writes: >> On 11 April 2018 at 03:34, Tom Lane <tgl@sss.pgh.pa.us> 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
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 <horiguchi.kyotaro@lab.ntt.co.jp> 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 "fail_part" would overlap partition "part_1"
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
-ERROR: syntax error at or near "::"
-LINE 1: ...fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
- ^
+ERROR: partition "fail_part" would overlap partition "part_1"
How about just remove the two tests that now get the overlap error.
Also,
@@ -490,12 +486,10 @@ CREATE TABLE moneyp (
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-ERROR: specified value cannot be cast to type money for column "a"
-LINE 1: ...EATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
- ^
-DETAIL: The cast requires a non-immutable conversion.
-HINT: Try putting the literal value in single quotes.
+CREATE TABLE moneyp_11 PARTITION OF moneyp FOR VALUES IN ('11');
+CREATE TABLE moneyp_12 PARTITION OF moneyp FOR VALUES IN (to_char(12,
'99')::int);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
+ERROR: relation "moneyp_10" already exists
Remove the command that causes overlap error, or simply just remove the
whole moneyp test, as its purpose was to exercise the code that's now removed.
Thanks,
Amit
On 2018/04/11 13:39, David Rowley wrote: > On 11 April 2018 at 05:22, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> David Rowley <david.rowley@2ndquadrant.com> writes: >>> On 11 April 2018 at 03:34, Tom Lane <tgl@sss.pgh.pa.us> 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
At Wed, 11 Apr 2018 14:22:29 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <6e929961-4160-7338-3d26-ccf84f41672a@lab.ntt.co.jp> > On 2018/04/11 13:39, David Rowley wrote: > > On 11 April 2018 at 05:22, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> David Rowley <david.rowley@2ndquadrant.com> writes: > >>> On 11 April 2018 at 03:34, Tom Lane <tgl@sss.pgh.pa.us> 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
Thank you for the comments.
At Wed, 11 Apr 2018 13:51:55 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<3d0fda29-986c-d970-a22c-b4bd44f56931@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 <horiguchi.kyotaro@lab.ntt.co.jp> 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 overlap partition "part_1"
> CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
> -ERROR: syntax error at or near "::"
> -LINE 1: ...fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
> - ^
> +ERROR: partition "fail_part" would overlap partition "part_1"
>
> How about just remove the two tests that now get the overlap error.
Removed.
> Also,
>
> @@ -490,12 +486,10 @@ CREATE TABLE moneyp (
> a money
> ) PARTITION BY LIST (a);
> CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
> -ERROR: specified value cannot be cast to type money for column "a"
> -LINE 1: ...EATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
> - ^
> -DETAIL: The cast requires a non-immutable conversion.
> -HINT: Try putting the literal value in single quotes.
> +CREATE TABLE moneyp_11 PARTITION OF moneyp FOR VALUES IN ('11');
> +CREATE TABLE moneyp_12 PARTITION OF moneyp FOR VALUES IN (to_char(12,
> '99')::int);
> CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
> +ERROR: relation "moneyp_10" already exists
>
> Remove the command that causes overlap error, or simply just remove the
> whole moneyp test, as its purpose was to exercise the code that's now removed.
Removed. And added tests for collation handling. (and
partbound_datum_list is fixed.)
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 <node> columnDef columnOptions
%type <defelt> def_elem reloption_elem old_aggr_elem operator_def_elem
%type <node> 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 <list> rowsfrom_item rowsfrom_list opt_col_def_list
@@ -585,7 +588,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <partelem> part_elem
%type <list> part_params
%type <partboundspec> PartitionBoundSpec
-%type <node> partbound_datum PartitionRangeDatum
+%type <node> PartitionRangeDatum
%type <list> hash_partbound partbound_datum_list range_datum_list
%type <defelt> 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); }
+ | partbound_datum_list ',' u_expr
{ $$ = lappend($1, $3); }
;
@@ -2825,33 +2822,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
+ | u_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 +13460,17 @@ 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; }
+ ;
+
+/* u_expr is a subset of b_expr usable along with unreserved keywords */
+u_expr: c0_expr { $$ = $1; }
+ | b0_expr { $$ = $1; }
+ ;
+
+/* common part of b_expr and u_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 +13544,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 u_expr */
+c0_expr: AexprConst { $$ = $1; }
| PARAM opt_indirection
{
ParamRef *p = makeNode(ParamRef);
@@ -16275,6 +16269,18 @@ makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner)
return r;
}
+static Node *
+makePartRangeDatum(PartitionRangeDatumKind kind, Node *value, int location)
+{
+ PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
+
+ n->kind = kind;
+ n->value = value;
+ n->location = location;
+
+ return (Node *) n;
+}
+
/* Separate Constraint nodes from COLLATE clauses in a ColQualList */
static void
SplitColQualList(List *qualList,
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 0307738946..4e426f2b28 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -506,6 +506,13 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
else
err = _("grouping operations are not allowed in EXECUTE parameters");
+ break;
+ case EXPR_KIND_PARTITION_BOUND:
+ if (isAgg)
+ err = _("aggregate functions are not allowed in partition bound");
+ else
+ err = _("grouping operations are not allowed in partition bound");
+
break;
case EXPR_KIND_TRIGGER_WHEN:
if (isAgg)
@@ -909,6 +916,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_PARTITION_EXPRESSION:
err = _("window functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("window functions are not allowed in partition bound");
+ break;
case EXPR_KIND_CALL_ARGUMENT:
err = _("window functions are not allowed in CALL arguments");
break;
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..fef05388b6 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 EXPR_KIND_PARTITION_EXPRESSION:
err = _("set-returning functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("set-returning functions are not allowed in partition bounds");
+ break;
case EXPR_KIND_CALL_ARGUMENT:
err = _("set-returning functions are not allowed in CALL arguments");
break;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index f9f9904bad..0112d22d23 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -48,6 +48,7 @@
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
+#include "optimizer/clauses.h"
#include "optimizer/planner.h"
#include "parser/analyze.h"
#include "parser/parse_clause.h"
@@ -138,8 +139,9 @@ static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
static void setSchemaName(char *context_schema, char **stmt_schema_name);
static void transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd);
static void validateInfiniteBounds(ParseState *pstate, List *blist);
-static Const *transformPartitionBoundValue(ParseState *pstate, A_Const *con,
- const char *colName, Oid colType, int32 colTypmod);
+static Const *transformPartitionBoundValue(ParseState *pstate, Node *con,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid colCollation);
/*
@@ -3651,6 +3653,7 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
+ Oid colcollation;
if (spec->strategy != PARTITION_STRATEGY_LIST)
ereport(ERROR,
@@ -3670,17 +3673,19 @@ transformPartitionBound(ParseState *pstate, Relation parent,
/* Need its type data too */
coltype = get_partition_col_typid(key, 0);
coltypmod = get_partition_col_typmod(key, 0);
+ colcollation = get_partition_col_collation(key, 0);
result_spec->listdatums = NIL;
foreach(cell, spec->listdatums)
{
- A_Const *con = castNode(A_Const, lfirst(cell));
+ Node *expr = (Node *)lfirst (cell);
Const *value;
ListCell *cell2;
bool duplicate;
- value = transformPartitionBoundValue(pstate, con,
- colname, coltype, coltypmod);
+ value = transformPartitionBoundValue(pstate, expr,
+ colname, coltype, coltypmod,
+ colcollation);
/* Don't add to the result if the value is a duplicate */
duplicate = false;
@@ -3740,7 +3745,7 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
- A_Const *con;
+ Oid colcollation;
Const *value;
/* Get the column's name in case we need to output an error */
@@ -3758,13 +3763,15 @@ transformPartitionBound(ParseState *pstate, Relation parent,
/* Need its type data too */
coltype = get_partition_col_typid(key, i);
coltypmod = get_partition_col_typmod(key, i);
+ colcollation = get_partition_col_collation(key, i);
if (ldatum->value)
{
- con = castNode(A_Const, ldatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ ldatum->value,
colname,
- coltype, coltypmod);
+ coltype, coltypmod,
+ colcollation);
if (value->constisnull)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
@@ -3775,10 +3782,11 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (rdatum->value)
{
- con = castNode(A_Const, rdatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ rdatum->value,
colname,
- coltype, coltypmod);
+ coltype, coltypmod,
+ colcollation);
if (value->constisnull)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
@@ -3845,13 +3853,14 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
* Transform one constant in a partition bound spec
*/
static Const *
-transformPartitionBoundValue(ParseState *pstate, A_Const *con,
- const char *colName, Oid colType, int32 colTypmod)
+transformPartitionBoundValue(ParseState *pstate, Node *val,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid colCollation)
{
Node *value;
- /* Make it into a Const */
- value = (Node *) make_const(pstate, &con->val, con->location);
+ /* Transform raw parsetree */
+ value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUND);
/* Coerce to correct type */
value = coerce_to_target_type(pstate,
@@ -3867,21 +3876,32 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("specified value cannot be cast to type %s for column \"%s\"",
format_type_be(colType), colName),
- parser_errposition(pstate, con->location)));
+ parser_errposition(pstate, exprLocation(val))));
+
+ /* Fix collations after all else */
+ assign_expr_collations(pstate, value);
+
+ /*
+ * Check for conflict between explict collations. Partition key expression
+ * has precedence over partition bound value.
+ */
+ if (exprCollation(value) != DEFAULT_COLLATION_OID &&
+ colCollation != exprCollation(value))
+ ereport(ERROR,
+ (errcode(ERRCODE_COLLATION_MISMATCH),
+ errmsg("collation mismatch between partition key expression (%d) and partition bound value (%d)",
colCollation,exprCollation(value)),
+ parser_errposition(pstate, exprLocation(val))));
+
/* Simplify the expression, in case we had a coercion */
if (!IsA(value, Const))
value = (Node *) expression_planner((Expr *) value);
- /* Fail if we don't have a constant (i.e., non-immutable coercion) */
+ /* Eval if we still don't have a constant (i.e., non-immutable coercion) */
if (!IsA(value, Const))
- ereport(ERROR,
- (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("specified value cannot be cast to type %s for column \"%s\"",
- format_type_be(colType), colName),
- errdetail("The cast requires a non-immutable conversion."),
- errhint("Try putting the literal value in single quotes."),
- parser_errposition(pstate, con->location)));
-
+ value = (Node *)evaluate_expr((Expr *) value, colType, colTypmod,
+ colCollation);
+
+ Assert(IsA(value, Const));
return (Const *) value;
}
diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h
index ba4fa4b68b..4b1a5b96f8 100644
--- a/src/include/optimizer/clauses.h
+++ b/src/include/optimizer/clauses.h
@@ -85,4 +85,6 @@ extern Node *estimate_expression_value(PlannerInfo *root, Node *node);
extern Query *inline_set_returning_function(PlannerInfo *root,
RangeTblEntry *rte);
+extern Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
+ Oid result_collation);
#endif /* CLAUSES_H */
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index 3fd2151ccb..9175a32c42 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -70,6 +70,7 @@ typedef enum ParseExprKind
EXPR_KIND_TRIGGER_WHEN, /* WHEN condition in CREATE TRIGGER */
EXPR_KIND_POLICY, /* USING or WITH CHECK expr in policy */
EXPR_KIND_PARTITION_EXPRESSION, /* PARTITION BY expression */
+ EXPR_KIND_PARTITION_BOUND, /* partition bounds value */
EXPR_KIND_CALL_ARGUMENT /* procedure argument in CALL */
} ParseExprKind;
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index ffffde01da..215f5fa06e 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -660,6 +660,12 @@ get_partition_col_typmod(PartitionKey key, int col)
return key->parttypmod[col];
}
+static inline Oid
+get_partition_col_collation(PartitionKey key, int col)
+{
+ return key->partcollation[col];
+}
+
/*
* RelationGetPartitionDesc
* Returns partition descriptor for a relation.
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index e724439037..2080a656e4 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -449,14 +449,6 @@ CREATE TABLE list_parted (
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');
- ^
-CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
-ERROR: syntax error at or near "::"
-LINE 1: ...fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
- ^
-- syntax does not allow empty list of values for list partitions
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ();
ERROR: syntax error at or near ")"
@@ -490,12 +482,8 @@ CREATE TABLE moneyp (
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-ERROR: specified value cannot be cast to type money for column "a"
-LINE 1: ...EATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
- ^
-DETAIL: The cast requires a non-immutable conversion.
-HINT: Try putting the literal value in single quotes.
-CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
+CREATE TABLE moneyp_11 PARTITION OF moneyp FOR VALUES IN ('11');
+CREATE TABLE moneyp_12 PARTITION OF moneyp FOR VALUES IN (to_char(12, '99')::int);
DROP TABLE moneyp;
-- immutable cast should work, though
CREATE TABLE bigintp (
@@ -683,6 +671,26 @@ ERROR: modulus for hash partition must be a positive integer
-- remainder must be greater than or equal to zero and less than modulus
CREATE TABLE fail_part PARTITION OF hash_parted2 FOR VALUES WITH (MODULUS 8, REMAINDER 8);
ERROR: remainder for hash partition must be less than modulus
+-- check for collation handling
+CREATE TABLE col_parted (
+ a varchar
+) PARTITION BY LIST (a);
+CREATE TABLE fail_part PARTITION OF col_parted FOR VALUES IN (('a' collate "en_US"));
+ERROR: collation mismatch between partition key expression (100) and partition bound value (12638)
+LINE 1: ...fail_part PARTITION OF col_parted FOR VALUES IN (('a' collat...
+ ^
+CREATE TABLE success_part PARTITION OF col_parted FOR VALUES IN ('a');
+DROP TABLE col_parted;
+CREATE TABLE col_parted (
+ a varchar collate "en_US"
+) PARTITION BY LIST (a);
+CREATE TABLE fail_part PARTITION OF col_parted FOR VALUES IN (('a' collate "en_GB"));
+ERROR: collation mismatch between partition key expression (12638) and partition bound value (12631)
+LINE 1: ...fail_part PARTITION OF col_parted FOR VALUES IN (('a' collat...
+ ^
+CREATE TABLE success_part PARTITION OF col_parted FOR VALUES IN ('a');
+CREATE TABLE success_part2 PARTITION OF col_parted FOR VALUES IN (('b' collate "en_US"));
+DROP TABLE col_parted;
-- check schema propagation from parent
CREATE TABLE parted (
a text,
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 235bef13dc..f739d89a75 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -432,8 +432,6 @@ CREATE TABLE list_parted (
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');
-CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
-- syntax does not allow empty list of values for list partitions
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ();
@@ -458,7 +456,8 @@ CREATE TABLE moneyp (
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
+CREATE TABLE moneyp_11 PARTITION OF moneyp FOR VALUES IN ('11');
+CREATE TABLE moneyp_12 PARTITION OF moneyp FOR VALUES IN (to_char(12, '99')::int);
DROP TABLE moneyp;
-- immutable cast should work, though
@@ -620,6 +619,22 @@ CREATE TABLE fail_part PARTITION OF hash_parted2 FOR VALUES WITH (MODULUS 0, REM
-- remainder must be greater than or equal to zero and less than modulus
CREATE TABLE fail_part PARTITION OF hash_parted2 FOR VALUES WITH (MODULUS 8, REMAINDER 8);
+-- check for collation handling
+CREATE TABLE col_parted (
+ a varchar
+) PARTITION BY LIST (a);
+CREATE TABLE fail_part PARTITION OF col_parted FOR VALUES IN (('a' collate "en_US"));
+CREATE TABLE success_part PARTITION OF col_parted FOR VALUES IN ('a');
+DROP TABLE col_parted;
+
+CREATE TABLE col_parted (
+ a varchar collate "en_US"
+) PARTITION BY LIST (a);
+CREATE TABLE fail_part PARTITION OF col_parted FOR VALUES IN (('a' collate "en_GB"));
+CREATE TABLE success_part PARTITION OF col_parted FOR VALUES IN ('a');
+CREATE TABLE success_part2 PARTITION OF col_parted FOR VALUES IN (('b' collate "en_US"));
+DROP TABLE col_parted;
+
-- check schema propagation from parent
CREATE TABLE parted (
> On Apr 11, 2018, at 4:33 AM, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>
> Thank you for the comments.
>
> At Wed, 11 Apr 2018 13:51:55 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<3d0fda29-986c-d970-a22c-b4bd44f56931@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 <horiguchi.kyotaro@lab.ntt.co.jp> 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 overlap partition "part_1"
>> CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
>> -ERROR: syntax error at or near "::"
>> -LINE 1: ...fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
>> - ^
>> +ERROR: partition "fail_part" would overlap partition "part_1"
>>
>> How about just remove the two tests that now get the overlap error.
>
> Removed.
>
>> Also,
>>
>> @@ -490,12 +486,10 @@ CREATE TABLE moneyp (
>> a money
>> ) PARTITION BY LIST (a);
>> CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
>> -ERROR: specified value cannot be cast to type money for column "a"
>> -LINE 1: ...EATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
>> - ^
>> -DETAIL: The cast requires a non-immutable conversion.
>> -HINT: Try putting the literal value in single quotes.
>> +CREATE TABLE moneyp_11 PARTITION OF moneyp FOR VALUES IN ('11');
>> +CREATE TABLE moneyp_12 PARTITION OF moneyp FOR VALUES IN (to_char(12,
>> '99')::int);
>> CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
>> +ERROR: relation "moneyp_10" already exists
>>
>> Remove the command that causes overlap error, or simply just remove the
>> whole moneyp test, as its purpose was to exercise the code that's now removed.
>
> Removed. And added tests for collation handling. (and
> partbound_datum_list is fixed.)
I experimented with this patch a bit. First, I could not build it:
parse_expr.c:1853:8: error: use of undeclared identifier 'EXPR_KIND_PARTITION_BOUNDS'; did you mean
'EXPR_KIND_PARTITION_BOUND'?
case EXPR_KIND_PARTITION_BOUNDS:
^~~~~~~~~~~~~~~~~~~~~~~~~~
EXPR_KIND_PARTITION_BOUND
../../../src/include/parser/parse_node.h:73:2: note: 'EXPR_KIND_PARTITION_BOUND' declared here
EXPR_KIND_PARTITION_BOUND, /* partition bounds value */
^
parse_expr.c:3480:8: error: use of undeclared identifier 'EXPR_KIND_PARTITION_BOUNDS'; did you mean
'EXPR_KIND_PARTITION_BOUND'?
case EXPR_KIND_PARTITION_BOUNDS:
^~~~~~~~~~~~~~~~~~~~~~~~~~
EXPR_KIND_PARTITION_BOUND
../../../src/include/parser/parse_node.h:73:2: note: 'EXPR_KIND_PARTITION_BOUND' declared here
EXPR_KIND_PARTITION_BOUND, /* partition bounds value */
^
2 errors generated.
The attached patch fixes the error.
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.
Вложения
Hello.
At Wed, 11 Apr 2018 09:43:37 -0400, "Jonathan S. Katz" <jonathan.katz@excoventures.com> wrote in
<EFEBA03D-EFFC-46E2-A0F7-41D00487066B@excoventures.com>
> 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 <node> columnDef columnOptions
%type <defelt> def_elem reloption_elem old_aggr_elem operator_def_elem
%type <node> 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 <list> rowsfrom_item rowsfrom_list opt_col_def_list
@@ -585,7 +588,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <partelem> part_elem
%type <list> part_params
%type <partboundspec> PartitionBoundSpec
-%type <node> partbound_datum PartitionRangeDatum
+%type <node> PartitionRangeDatum
%type <list> hash_partbound partbound_datum_list range_datum_list
%type <defelt> 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); }
+ | partbound_datum_list ',' u_expr
{ $$ = lappend($1, $3); }
;
@@ -2825,33 +2822,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
+ | u_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 +13460,17 @@ 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; }
+ ;
+
+/* u_expr is a subset of b_expr usable along with unreserved keywords */
+u_expr: c0_expr { $$ = $1; }
+ | b0_expr { $$ = $1; }
+ ;
+
+/* common part of b_expr and u_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 +13544,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 u_expr */
+c0_expr: AexprConst { $$ = $1; }
| PARAM opt_indirection
{
ParamRef *p = makeNode(ParamRef);
@@ -16275,6 +16269,18 @@ makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner)
return r;
}
+static Node *
+makePartRangeDatum(PartitionRangeDatumKind kind, Node *value, int location)
+{
+ PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
+
+ n->kind = kind;
+ n->value = value;
+ n->location = location;
+
+ return (Node *) n;
+}
+
/* Separate Constraint nodes from COLLATE clauses in a ColQualList */
static void
SplitColQualList(List *qualList,
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 0307738946..4e426f2b28 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -506,6 +506,13 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
else
err = _("grouping operations are not allowed in EXECUTE parameters");
+ break;
+ case EXPR_KIND_PARTITION_BOUND:
+ if (isAgg)
+ err = _("aggregate functions are not allowed in partition bound");
+ else
+ err = _("grouping operations are not allowed in partition bound");
+
break;
case EXPR_KIND_TRIGGER_WHEN:
if (isAgg)
@@ -909,6 +916,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_PARTITION_EXPRESSION:
err = _("window functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("window functions are not allowed in partition bound");
+ break;
case EXPR_KIND_CALL_ARGUMENT:
err = _("window functions are not allowed in CALL arguments");
break;
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 38fbe3366f..6c06296c00 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_BOUND:
+ err = _("cannot use subquery in partition bound");
+ 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_BOUND:
+ return "partition bound";
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..c63dfce866 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 EXPR_KIND_PARTITION_EXPRESSION:
err = _("set-returning functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("set-returning functions are not allowed in partition bound");
+ break;
case EXPR_KIND_CALL_ARGUMENT:
err = _("set-returning functions are not allowed in CALL arguments");
break;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index f9f9904bad..0112d22d23 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -48,6 +48,7 @@
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
+#include "optimizer/clauses.h"
#include "optimizer/planner.h"
#include "parser/analyze.h"
#include "parser/parse_clause.h"
@@ -138,8 +139,9 @@ static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
static void setSchemaName(char *context_schema, char **stmt_schema_name);
static void transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd);
static void validateInfiniteBounds(ParseState *pstate, List *blist);
-static Const *transformPartitionBoundValue(ParseState *pstate, A_Const *con,
- const char *colName, Oid colType, int32 colTypmod);
+static Const *transformPartitionBoundValue(ParseState *pstate, Node *con,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid colCollation);
/*
@@ -3651,6 +3653,7 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
+ Oid colcollation;
if (spec->strategy != PARTITION_STRATEGY_LIST)
ereport(ERROR,
@@ -3670,17 +3673,19 @@ transformPartitionBound(ParseState *pstate, Relation parent,
/* Need its type data too */
coltype = get_partition_col_typid(key, 0);
coltypmod = get_partition_col_typmod(key, 0);
+ colcollation = get_partition_col_collation(key, 0);
result_spec->listdatums = NIL;
foreach(cell, spec->listdatums)
{
- A_Const *con = castNode(A_Const, lfirst(cell));
+ Node *expr = (Node *)lfirst (cell);
Const *value;
ListCell *cell2;
bool duplicate;
- value = transformPartitionBoundValue(pstate, con,
- colname, coltype, coltypmod);
+ value = transformPartitionBoundValue(pstate, expr,
+ colname, coltype, coltypmod,
+ colcollation);
/* Don't add to the result if the value is a duplicate */
duplicate = false;
@@ -3740,7 +3745,7 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
- A_Const *con;
+ Oid colcollation;
Const *value;
/* Get the column's name in case we need to output an error */
@@ -3758,13 +3763,15 @@ transformPartitionBound(ParseState *pstate, Relation parent,
/* Need its type data too */
coltype = get_partition_col_typid(key, i);
coltypmod = get_partition_col_typmod(key, i);
+ colcollation = get_partition_col_collation(key, i);
if (ldatum->value)
{
- con = castNode(A_Const, ldatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ ldatum->value,
colname,
- coltype, coltypmod);
+ coltype, coltypmod,
+ colcollation);
if (value->constisnull)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
@@ -3775,10 +3782,11 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (rdatum->value)
{
- con = castNode(A_Const, rdatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ rdatum->value,
colname,
- coltype, coltypmod);
+ coltype, coltypmod,
+ colcollation);
if (value->constisnull)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
@@ -3845,13 +3853,14 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
* Transform one constant in a partition bound spec
*/
static Const *
-transformPartitionBoundValue(ParseState *pstate, A_Const *con,
- const char *colName, Oid colType, int32 colTypmod)
+transformPartitionBoundValue(ParseState *pstate, Node *val,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid colCollation)
{
Node *value;
- /* Make it into a Const */
- value = (Node *) make_const(pstate, &con->val, con->location);
+ /* Transform raw parsetree */
+ value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUND);
/* Coerce to correct type */
value = coerce_to_target_type(pstate,
@@ -3867,21 +3876,32 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("specified value cannot be cast to type %s for column \"%s\"",
format_type_be(colType), colName),
- parser_errposition(pstate, con->location)));
+ parser_errposition(pstate, exprLocation(val))));
+
+ /* Fix collations after all else */
+ assign_expr_collations(pstate, value);
+
+ /*
+ * Check for conflict between explict collations. Partition key expression
+ * has precedence over partition bound value.
+ */
+ if (exprCollation(value) != DEFAULT_COLLATION_OID &&
+ colCollation != exprCollation(value))
+ ereport(ERROR,
+ (errcode(ERRCODE_COLLATION_MISMATCH),
+ errmsg("collation mismatch between partition key expression (%d) and partition bound value (%d)",
colCollation,exprCollation(value)),
+ parser_errposition(pstate, exprLocation(val))));
+
/* Simplify the expression, in case we had a coercion */
if (!IsA(value, Const))
value = (Node *) expression_planner((Expr *) value);
- /* Fail if we don't have a constant (i.e., non-immutable coercion) */
+ /* Eval if we still don't have a constant (i.e., non-immutable coercion) */
if (!IsA(value, Const))
- ereport(ERROR,
- (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("specified value cannot be cast to type %s for column \"%s\"",
- format_type_be(colType), colName),
- errdetail("The cast requires a non-immutable conversion."),
- errhint("Try putting the literal value in single quotes."),
- parser_errposition(pstate, con->location)));
-
+ value = (Node *)evaluate_expr((Expr *) value, colType, colTypmod,
+ colCollation);
+
+ Assert(IsA(value, Const));
return (Const *) value;
}
diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h
index ba4fa4b68b..4b1a5b96f8 100644
--- a/src/include/optimizer/clauses.h
+++ b/src/include/optimizer/clauses.h
@@ -85,4 +85,6 @@ extern Node *estimate_expression_value(PlannerInfo *root, Node *node);
extern Query *inline_set_returning_function(PlannerInfo *root,
RangeTblEntry *rte);
+extern Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
+ Oid result_collation);
#endif /* CLAUSES_H */
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index 3fd2151ccb..9175a32c42 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -70,6 +70,7 @@ typedef enum ParseExprKind
EXPR_KIND_TRIGGER_WHEN, /* WHEN condition in CREATE TRIGGER */
EXPR_KIND_POLICY, /* USING or WITH CHECK expr in policy */
EXPR_KIND_PARTITION_EXPRESSION, /* PARTITION BY expression */
+ EXPR_KIND_PARTITION_BOUND, /* partition bounds value */
EXPR_KIND_CALL_ARGUMENT /* procedure argument in CALL */
} ParseExprKind;
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index ffffde01da..215f5fa06e 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -660,6 +660,12 @@ get_partition_col_typmod(PartitionKey key, int col)
return key->parttypmod[col];
}
+static inline Oid
+get_partition_col_collation(PartitionKey key, int col)
+{
+ return key->partcollation[col];
+}
+
/*
* RelationGetPartitionDesc
* Returns partition descriptor for a relation.
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index e724439037..2080a656e4 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -449,14 +449,6 @@ CREATE TABLE list_parted (
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');
- ^
-CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
-ERROR: syntax error at or near "::"
-LINE 1: ...fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
- ^
-- syntax does not allow empty list of values for list partitions
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ();
ERROR: syntax error at or near ")"
@@ -490,12 +482,8 @@ CREATE TABLE moneyp (
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-ERROR: specified value cannot be cast to type money for column "a"
-LINE 1: ...EATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
- ^
-DETAIL: The cast requires a non-immutable conversion.
-HINT: Try putting the literal value in single quotes.
-CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
+CREATE TABLE moneyp_11 PARTITION OF moneyp FOR VALUES IN ('11');
+CREATE TABLE moneyp_12 PARTITION OF moneyp FOR VALUES IN (to_char(12, '99')::int);
DROP TABLE moneyp;
-- immutable cast should work, though
CREATE TABLE bigintp (
@@ -683,6 +671,26 @@ ERROR: modulus for hash partition must be a positive integer
-- remainder must be greater than or equal to zero and less than modulus
CREATE TABLE fail_part PARTITION OF hash_parted2 FOR VALUES WITH (MODULUS 8, REMAINDER 8);
ERROR: remainder for hash partition must be less than modulus
+-- check for collation handling
+CREATE TABLE col_parted (
+ a varchar
+) PARTITION BY LIST (a);
+CREATE TABLE fail_part PARTITION OF col_parted FOR VALUES IN (('a' collate "en_US"));
+ERROR: collation mismatch between partition key expression (100) and partition bound value (12638)
+LINE 1: ...fail_part PARTITION OF col_parted FOR VALUES IN (('a' collat...
+ ^
+CREATE TABLE success_part PARTITION OF col_parted FOR VALUES IN ('a');
+DROP TABLE col_parted;
+CREATE TABLE col_parted (
+ a varchar collate "en_US"
+) PARTITION BY LIST (a);
+CREATE TABLE fail_part PARTITION OF col_parted FOR VALUES IN (('a' collate "en_GB"));
+ERROR: collation mismatch between partition key expression (12638) and partition bound value (12631)
+LINE 1: ...fail_part PARTITION OF col_parted FOR VALUES IN (('a' collat...
+ ^
+CREATE TABLE success_part PARTITION OF col_parted FOR VALUES IN ('a');
+CREATE TABLE success_part2 PARTITION OF col_parted FOR VALUES IN (('b' collate "en_US"));
+DROP TABLE col_parted;
-- check schema propagation from parent
CREATE TABLE parted (
a text,
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 235bef13dc..f739d89a75 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -432,8 +432,6 @@ CREATE TABLE list_parted (
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');
-CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
-- syntax does not allow empty list of values for list partitions
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ();
@@ -458,7 +456,8 @@ CREATE TABLE moneyp (
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
+CREATE TABLE moneyp_11 PARTITION OF moneyp FOR VALUES IN ('11');
+CREATE TABLE moneyp_12 PARTITION OF moneyp FOR VALUES IN (to_char(12, '99')::int);
DROP TABLE moneyp;
-- immutable cast should work, though
@@ -620,6 +619,22 @@ CREATE TABLE fail_part PARTITION OF hash_parted2 FOR VALUES WITH (MODULUS 0, REM
-- remainder must be greater than or equal to zero and less than modulus
CREATE TABLE fail_part PARTITION OF hash_parted2 FOR VALUES WITH (MODULUS 8, REMAINDER 8);
+-- check for collation handling
+CREATE TABLE col_parted (
+ a varchar
+) PARTITION BY LIST (a);
+CREATE TABLE fail_part PARTITION OF col_parted FOR VALUES IN (('a' collate "en_US"));
+CREATE TABLE success_part PARTITION OF col_parted FOR VALUES IN ('a');
+DROP TABLE col_parted;
+
+CREATE TABLE col_parted (
+ a varchar collate "en_US"
+) PARTITION BY LIST (a);
+CREATE TABLE fail_part PARTITION OF col_parted FOR VALUES IN (('a' collate "en_GB"));
+CREATE TABLE success_part PARTITION OF col_parted FOR VALUES IN ('a');
+CREATE TABLE success_part2 PARTITION OF col_parted FOR VALUES IN (('b' collate "en_US"));
+DROP TABLE col_parted;
+
-- check schema propagation from parent
CREATE TABLE parted (
Hi,
On Apr 12, 2018, at 12:12 AM, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:Hello.
At Wed, 11 Apr 2018 09:43:37 -0400, "Jonathan S. Katz" <jonathan.katz@excoventures.com> wrote in <EFEBA03D-EFFC-46E2-A0F7-41D00487066B@excoventures.com>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
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
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 <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<ca37d679-014e-1895-e4bc-89b7129ce58b@lab.ntt.co.jp>
> 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 <node> columnDef columnOptions
%type <defelt> def_elem reloption_elem old_aggr_elem operator_def_elem
%type <node> 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 <list> rowsfrom_item rowsfrom_list opt_col_def_list
@@ -581,7 +584,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <partelem> part_elem
%type <list> part_params
%type <partboundspec> PartitionBoundSpec
-%type <node> partbound_datum PartitionRangeDatum
+%type <node> PartitionRangeDatum
%type <list> hash_partbound partbound_datum_list range_datum_list
%type <defelt> 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;
+ * for PRESERVE STRIP_P to support xml_whitespace_option.
*
* To support CUBE and ROLLUP in GROUP BY without reserving them, we give them
* an explicit priority lower than '(', so that a rule with CUBE '(' will shift
@@ -752,7 +759,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
* blame any funny behavior of UNBOUNDED on the SQL standard, though.
*/
%nonassoc UNBOUNDED /* ideally should have same precedence as IDENT */
-%nonassoc IDENT GENERATED NULL_P PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
+%nonassoc IDENT GENERATED NULL_P PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP PASSING BY COLUMNS ROW
PRESERVESTRIP_P
%left Op OPERATOR /* multi-character ops and user-defined operators */
%left '+' '-'
%left '*' '/' '%'
@@ -773,9 +780,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
* left-associativity among the JOIN rules themselves.
*/
%left JOIN CROSS LEFT FULL RIGHT INNER_P NATURAL
-/* kluge to keep xml_whitespace_option from causing shift/reduce conflicts */
-%right PRESERVE STRIP_P
-
%%
/*
@@ -2795,15 +2799,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); }
+ | partbound_datum_list ',' u_expr
{ $$ = lappend($1, $3); }
;
@@ -2816,33 +2814,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
+ | u_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);
}
;
@@ -11605,12 +11588,8 @@ opt_select_fetch_first_value:
| /*EMPTY*/ { $$ = makeIntConst(1, -1); }
;
-/*
- * Again, the trailing ROW/ROWS in this case prevent the full expression
- * syntax. c_expr is the best we can do.
- */
select_offset_value2:
- c_expr { $$ = $1; }
+ a_expr { $$ = $1; }
;
/* noise words */
@@ -12264,9 +12243,16 @@ TableFuncElement: ColId Typename opt_collate_clause
/*
* XMLTABLE
+ *
+ * If we see PASSING after the a_expr, it cannot be a postfix operator but a
+ * part of xmlexists_arguent but it can be taken as a part of the
+ * a_expr. Similary xmlexists_argument can end with a_expr then the succeeding
+ * COLUMNS can be taken as an postfix operator. We fix this by making them
+ * have higher precedence than POSTFIXOP, so that the shift/reduce conflict is
+ * resolved in favor of shifting the rule.
*/
xmltable:
- XMLTABLE '(' c_expr xmlexists_argument COLUMNS xmltable_column_list ')'
+ XMLTABLE '(' a_expr xmlexists_argument COLUMNS xmltable_column_list ')'
{
RangeTableFunc *n = makeNode(RangeTableFunc);
n->rowexpr = $3;
@@ -12277,7 +12263,7 @@ xmltable:
$$ = (Node *)n;
}
| XMLTABLE '(' XMLNAMESPACES '(' xml_namespace_list ')' ','
- c_expr xmlexists_argument COLUMNS xmltable_column_list ')'
+ a_expr xmlexists_argument COLUMNS xmltable_column_list ')'
{
RangeTableFunc *n = makeNode(RangeTableFunc);
n->rowexpr = $8;
@@ -12891,16 +12877,19 @@ interval_second:
* General expressions
* This is the heart of the expression syntax.
*
- * We have two expression types: a_expr is the unrestricted kind, and
- * b_expr is a subset that must be used in some places to avoid shift/reduce
+ * We have three expression types: a_expr is the unrestricted kind, b_expr and
+ * u_expr are subsets that must be used in some places to avoid shift/reduce
* conflicts. For example, we can't do BETWEEN as "BETWEEN a_expr AND a_expr"
* because that use of AND conflicts with AND as a boolean operator. So,
* b_expr is used in BETWEEN and we remove boolean keywords from b_expr.
+ * Likewise we can't use MAXVALUE as an alternative rule of a_expr because
+ * columnref conflicts. So, u_expr is used in such places and we remove
+ * columnref from a_expr.
*
- * Note that '(' a_expr ')' is a b_expr, so an unrestricted expression can
- * always be used by surrounding it with parens.
+ * Note that '(' a_expr ')' is both a b_expr and a u_expr, so an unrestricted
+ * expression can always be used by surrounding it with parens.
*
- * c_expr is all the productions that are common to a_expr and b_expr;
+ * c_expr is all the productions that are common to a_expr, u_expr and b_expr;
* it's factored out just to eliminate redundant coding.
*
* Be careful of productions involving more than one terminal token.
@@ -12909,7 +12898,19 @@ interval_second:
* of the first terminal instead; otherwise you will not get the behavior
* you expect! So we use %prec annotations freely to set precedences.
*/
-a_expr: c_expr { $$ = $1; }
+a_expr: columnref { $$ = $1; }
+ | u_expr { $$ = $1; }
+ ;
+
+/*
+ * u_expr is a subset of a_expr usable as an altertive rule with unreserved
+ * keywords
+ *
+ * a_expr causes trouble since columnref conflicts with unreserved_keywords.
+ * This syntax is used for expressions appears with alternatives of
+ * unreserved_keywords.
+ */
+u_expr: c_expr { $$ = $1; }
| a_expr TYPECAST Typename
{ $$ = makeTypeCast($1, $3, @2); }
| a_expr COLLATE any_name
@@ -13331,8 +13332,8 @@ 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: columnref { $$ = $1; }
+ | c_expr { $$ = $1; }
| b_expr TYPECAST Typename
{ $$ = makeTypeCast($1, $3, @2); }
| '+' b_expr %prec UMINUS
@@ -13399,15 +13400,14 @@ b_expr: c_expr
;
/*
- * Productions that can be used in both a_expr and b_expr.
+ * Productions that can be used in both a_expr, u_expr and b_expr.
*
- * Note: productions that refer recursively to a_expr or b_expr mostly
- * cannot appear here. However, it's OK to refer to a_exprs that occur
- * inside parentheses, such as function arguments; that cannot introduce
- * ambiguity to the b_expr syntax.
+ * Note: productions that refer recursively to a_expr, u_expr or b_expr mostly
+ * cannot appear here. However, it's OK to refer to a_exprs that occur inside
+ * parentheses, such as function arguments; that cannot introduce ambiguity to
+ * the b_expr syntax.
*/
-c_expr: columnref { $$ = $1; }
- | AexprConst { $$ = $1; }
+c_expr: AexprConst { $$ = $1; }
| PARAM opt_indirection
{
ParamRef *p = makeNode(ParamRef);
@@ -13851,7 +13851,7 @@ func_expr_common_subexpr:
{
$$ = makeXmlExpr(IS_XMLELEMENT, $4, $6, $8, @1);
}
- | XMLEXISTS '(' c_expr xmlexists_argument ')'
+ | XMLEXISTS '(' a_expr xmlexists_argument ')'
{
/* xmlexists(A PASSING [BY REF] B [BY REF]) is
* converted to xmlexists(A, B)*/
@@ -13947,21 +13947,28 @@ xml_whitespace_option: PRESERVE WHITESPACE_P { $$ = true; }
| /*EMPTY*/ { $$ = false; }
;
-/* We allow several variants for SQL and other compatibility. */
+/*
+ * We allow several variants for SQL and other compatibility.
+ *
+ * If we see BY just aftert a_expr, it is not a part of preceding a_expr but
+ * it can be taken as a postfix operator. We fix this by making BY have
+ * higher precedence than POSTFIXOP, so that the shift/reduce confict is
+ * resolved in favor of reducing the rule.
+ */
xmlexists_argument:
- PASSING c_expr
+ PASSING a_expr
{
$$ = $2;
}
- | PASSING c_expr BY REF
+ | PASSING a_expr BY REF
{
$$ = $2;
}
- | PASSING BY REF c_expr
+ | PASSING BY REF a_expr
{
$$ = $4;
}
- | PASSING BY REF c_expr BY REF
+ | PASSING BY REF a_expr BY REF
{
$$ = $4;
}
@@ -16126,6 +16133,18 @@ makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner)
return r;
}
+static Node *
+makePartRangeDatum(PartitionRangeDatumKind kind, Node *value, int location)
+{
+ PartitionRangeDatum *n = makeNode(PartitionRangeDatum);
+
+ n->kind = kind;
+ n->value = value;
+ n->location = location;
+
+ return (Node *) n;
+}
+
/* Separate Constraint nodes from COLLATE clauses in a ColQualList */
static void
SplitColQualList(List *qualList,
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 61727e1d71..55a5304a38 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -499,6 +499,13 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
else
err = _("grouping operations are not allowed in EXECUTE parameters");
+ break;
+ case EXPR_KIND_PARTITION_BOUND:
+ if (isAgg)
+ err = _("aggregate functions are not allowed in partition bound");
+ else
+ err = _("grouping operations are not allowed in partition bound");
+
break;
case EXPR_KIND_TRIGGER_WHEN:
if (isAgg)
@@ -899,6 +906,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_PARTITION_EXPRESSION:
err = _("window functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("window functions are not allowed in partition bound");
+ break;
case EXPR_KIND_CALL_ARGUMENT:
err = _("window functions are not allowed in CALL arguments");
break;
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 385e54a9b6..f8759f185b 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -1849,6 +1849,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_CALL_ARGUMENT:
err = _("cannot use subquery in CALL argument");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("cannot use subquery in partition bound");
+ break;
/*
* There is intentionally no default: case here, so that the
@@ -3473,6 +3476,8 @@ ParseExprKindName(ParseExprKind exprKind)
return "WHEN";
case EXPR_KIND_PARTITION_EXPRESSION:
return "PARTITION BY";
+ case EXPR_KIND_PARTITION_BOUND:
+ return "partition bound";
case EXPR_KIND_CALL_ARGUMENT:
return "CALL";
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index ea5d5212b4..570ae860ae 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2303,6 +2303,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
case EXPR_KIND_PARTITION_EXPRESSION:
err = _("set-returning functions are not allowed in partition key expressions");
break;
+ case EXPR_KIND_PARTITION_BOUND:
+ err = _("set-returning functions are not allowed in partition bound");
+ break;
case EXPR_KIND_CALL_ARGUMENT:
err = _("set-returning functions are not allowed in CALL arguments");
break;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 9178139912..950421286c 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -48,6 +48,7 @@
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
+#include "optimizer/clauses.h"
#include "optimizer/planner.h"
#include "parser/analyze.h"
#include "parser/parse_clause.h"
@@ -138,8 +139,9 @@ static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
static void setSchemaName(char *context_schema, char **stmt_schema_name);
static void transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd);
static void validateInfiniteBounds(ParseState *pstate, List *blist);
-static Const *transformPartitionBoundValue(ParseState *pstate, A_Const *con,
- const char *colName, Oid colType, int32 colTypmod);
+static Const *transformPartitionBoundValue(ParseState *pstate, Node *con,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid colCollation);
/*
@@ -3648,6 +3650,7 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
+ Oid colcollation;
if (spec->strategy != PARTITION_STRATEGY_LIST)
ereport(ERROR,
@@ -3667,17 +3670,19 @@ transformPartitionBound(ParseState *pstate, Relation parent,
/* Need its type data too */
coltype = get_partition_col_typid(key, 0);
coltypmod = get_partition_col_typmod(key, 0);
+ colcollation = get_partition_col_collation(key, 0);
result_spec->listdatums = NIL;
foreach(cell, spec->listdatums)
{
- A_Const *con = castNode(A_Const, lfirst(cell));
+ Node *expr = (Node *)lfirst (cell);
Const *value;
ListCell *cell2;
bool duplicate;
- value = transformPartitionBoundValue(pstate, con,
- colname, coltype, coltypmod);
+ value = transformPartitionBoundValue(pstate, expr,
+ colname, coltype, coltypmod,
+ colcollation);
/* Don't add to the result if the value is a duplicate */
duplicate = false;
@@ -3737,7 +3742,7 @@ transformPartitionBound(ParseState *pstate, Relation parent,
char *colname;
Oid coltype;
int32 coltypmod;
- A_Const *con;
+ Oid colcollation;
Const *value;
/* Get the column's name in case we need to output an error */
@@ -3755,13 +3760,15 @@ transformPartitionBound(ParseState *pstate, Relation parent,
/* Need its type data too */
coltype = get_partition_col_typid(key, i);
coltypmod = get_partition_col_typmod(key, i);
+ colcollation = get_partition_col_collation(key, i);
if (ldatum->value)
{
- con = castNode(A_Const, ldatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ ldatum->value,
colname,
- coltype, coltypmod);
+ coltype, coltypmod,
+ colcollation);
if (value->constisnull)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
@@ -3772,10 +3779,11 @@ transformPartitionBound(ParseState *pstate, Relation parent,
if (rdatum->value)
{
- con = castNode(A_Const, rdatum->value);
- value = transformPartitionBoundValue(pstate, con,
+ value = transformPartitionBoundValue(pstate,
+ rdatum->value,
colname,
- coltype, coltypmod);
+ coltype, coltypmod,
+ colcollation);
if (value->constisnull)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
@@ -3842,13 +3850,14 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
* Transform one constant in a partition bound spec
*/
static Const *
-transformPartitionBoundValue(ParseState *pstate, A_Const *con,
- const char *colName, Oid colType, int32 colTypmod)
+transformPartitionBoundValue(ParseState *pstate, Node *val,
+ const char *colName, Oid colType, int32 colTypmod,
+ Oid colCollation)
{
Node *value;
- /* Make it into a Const */
- value = (Node *) make_const(pstate, &con->val, con->location);
+ /* Transform raw parsetree */
+ value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUND);
/* Coerce to correct type */
value = coerce_to_target_type(pstate,
@@ -3864,21 +3873,32 @@ transformPartitionBoundValue(ParseState *pstate, A_Const *con,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("specified value cannot be cast to type %s for column \"%s\"",
format_type_be(colType), colName),
- parser_errposition(pstate, con->location)));
+ parser_errposition(pstate, exprLocation(val))));
+
+ /* Fix collations after all else */
+ assign_expr_collations(pstate, value);
+
+ /*
+ * Check for conflict between explict collations. Partition key expression
+ * has precedence over partition bound value.
+ */
+ if (exprCollation(value) != DEFAULT_COLLATION_OID &&
+ colCollation != exprCollation(value))
+ ereport(ERROR,
+ (errcode(ERRCODE_COLLATION_MISMATCH),
+ errmsg("collation mismatch between partition key expression (%d) and partition bound value (%d)",
colCollation,exprCollation(value)),
+ parser_errposition(pstate, exprLocation(val))));
+
/* Simplify the expression, in case we had a coercion */
if (!IsA(value, Const))
value = (Node *) expression_planner((Expr *) value);
- /* Fail if we don't have a constant (i.e., non-immutable coercion) */
+ /* Eval if we still don't have a constant (i.e., non-immutable coercion) */
if (!IsA(value, Const))
- ereport(ERROR,
- (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("specified value cannot be cast to type %s for column \"%s\"",
- format_type_be(colType), colName),
- errdetail("The cast requires a non-immutable conversion."),
- errhint("Try putting the literal value in single quotes."),
- parser_errposition(pstate, con->location)));
-
+ value = (Node *)evaluate_expr((Expr *) value, colType, colTypmod,
+ colCollation);
+
+ Assert(IsA(value, Const));
return (Const *) value;
}
diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h
index ba4fa4b68b..4b1a5b96f8 100644
--- a/src/include/optimizer/clauses.h
+++ b/src/include/optimizer/clauses.h
@@ -85,4 +85,6 @@ extern Node *estimate_expression_value(PlannerInfo *root, Node *node);
extern Query *inline_set_returning_function(PlannerInfo *root,
RangeTblEntry *rte);
+extern Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
+ Oid result_collation);
#endif /* CLAUSES_H */
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index 0230543810..68bec4b932 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -69,6 +69,7 @@ typedef enum ParseExprKind
EXPR_KIND_TRIGGER_WHEN, /* WHEN condition in CREATE TRIGGER */
EXPR_KIND_POLICY, /* USING or WITH CHECK expr in policy */
EXPR_KIND_PARTITION_EXPRESSION, /* PARTITION BY expression */
+ EXPR_KIND_PARTITION_BOUND, /* partition bounds value */
EXPR_KIND_CALL_ARGUMENT /* procedure argument in CALL */
} ParseExprKind;
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index ffffde01da..215f5fa06e 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -660,6 +660,12 @@ get_partition_col_typmod(PartitionKey key, int col)
return key->parttypmod[col];
}
+static inline Oid
+get_partition_col_collation(PartitionKey key, int col)
+{
+ return key->partcollation[col];
+}
+
/*
* RelationGetPartitionDesc
* Returns partition descriptor for a relation.
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 654464c631..b4918802ed 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -449,14 +449,6 @@ CREATE TABLE list_parted (
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');
- ^
-CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
-ERROR: syntax error at or near "::"
-LINE 1: ...fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
- ^
-- syntax does not allow empty list of values for list partitions
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ();
ERROR: syntax error at or near ")"
@@ -490,12 +482,8 @@ CREATE TABLE moneyp (
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-ERROR: specified value cannot be cast to type money for column "a"
-LINE 1: ...EATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
- ^
-DETAIL: The cast requires a non-immutable conversion.
-HINT: Try putting the literal value in single quotes.
-CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
+CREATE TABLE moneyp_11 PARTITION OF moneyp FOR VALUES IN ('11');
+CREATE TABLE moneyp_12 PARTITION OF moneyp FOR VALUES IN (to_char(12, '99')::int);
DROP TABLE moneyp;
-- immutable cast should work, though
CREATE TABLE bigintp (
@@ -683,6 +671,26 @@ ERROR: modulus for hash partition must be a positive integer
-- remainder must be greater than or equal to zero and less than modulus
CREATE TABLE fail_part PARTITION OF hash_parted2 FOR VALUES WITH (MODULUS 8, REMAINDER 8);
ERROR: remainder for hash partition must be less than modulus
+-- check for collation handling
+CREATE TABLE col_parted (
+ a varchar
+) PARTITION BY LIST (a);
+CREATE TABLE fail_part PARTITION OF col_parted FOR VALUES IN (('a' collate "en_US"));
+ERROR: collation mismatch between partition key expression (100) and partition bound value (12638)
+LINE 1: ...fail_part PARTITION OF col_parted FOR VALUES IN (('a' collat...
+ ^
+CREATE TABLE success_part PARTITION OF col_parted FOR VALUES IN ('a');
+DROP TABLE col_parted;
+CREATE TABLE col_parted (
+ a varchar collate "en_US"
+) PARTITION BY LIST (a);
+CREATE TABLE fail_part PARTITION OF col_parted FOR VALUES IN (('a' collate "en_GB"));
+ERROR: collation mismatch between partition key expression (12638) and partition bound value (12631)
+LINE 1: ...fail_part PARTITION OF col_parted FOR VALUES IN (('a' collat...
+ ^
+CREATE TABLE success_part PARTITION OF col_parted FOR VALUES IN ('a');
+CREATE TABLE success_part2 PARTITION OF col_parted FOR VALUES IN (('b' collate "en_US"));
+DROP TABLE col_parted;
-- check schema propagation from parent
CREATE TABLE parted (
a text,
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 54694347ae..e979b6866e 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -432,8 +432,6 @@ CREATE TABLE list_parted (
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');
-CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ('1'::int);
-- syntax does not allow empty list of values for list partitions
CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN ();
@@ -458,7 +456,8 @@ CREATE TABLE moneyp (
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
+CREATE TABLE moneyp_11 PARTITION OF moneyp FOR VALUES IN ('11');
+CREATE TABLE moneyp_12 PARTITION OF moneyp FOR VALUES IN (to_char(12, '99')::int);
DROP TABLE moneyp;
-- immutable cast should work, though
@@ -620,6 +619,22 @@ CREATE TABLE fail_part PARTITION OF hash_parted2 FOR VALUES WITH (MODULUS 0, REM
-- remainder must be greater than or equal to zero and less than modulus
CREATE TABLE fail_part PARTITION OF hash_parted2 FOR VALUES WITH (MODULUS 8, REMAINDER 8);
+-- check for collation handling
+CREATE TABLE col_parted (
+ a varchar
+) PARTITION BY LIST (a);
+CREATE TABLE fail_part PARTITION OF col_parted FOR VALUES IN (('a' collate "en_US"));
+CREATE TABLE success_part PARTITION OF col_parted FOR VALUES IN ('a');
+DROP TABLE col_parted;
+
+CREATE TABLE col_parted (
+ a varchar collate "en_US"
+) PARTITION BY LIST (a);
+CREATE TABLE fail_part PARTITION OF col_parted FOR VALUES IN (('a' collate "en_GB"));
+CREATE TABLE success_part PARTITION OF col_parted FOR VALUES IN ('a');
+CREATE TABLE success_part2 PARTITION OF col_parted FOR VALUES IN (('b' collate "en_US"));
+DROP TABLE col_parted;
+
-- check schema propagation from parent
CREATE TABLE parted (
Sorry for a silly typo. At Mon, 16 Apr 2018 16:17:40 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20180416.161740.51264437.horiguchi.kyotaro@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 <Langote_Amit_f8@lab.ntt.co.jp> wrote in <ca37d679-014e-1895-e4bc-89b7129ce58b@lab.ntt.co.jp> > > 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
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
Вложения
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
Вложения
Thanks for reviewing.
At Wed, 18 Apr 2018 19:27:16 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<7ac6b44e-4638-3320-1512-f6c03a28d0f7@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 doesn't seem to be
> used anywhere.
DEFAULT value is not compared with anything, partition key
expression *is* compared with column value and the collation is
applied on the comparison.
In the following case, the comparison between 'zirkoniumet' and
'z', 'å' must be performed with "sv_SE" since the range is empty
if the collation were en_US and it is not the intention of the
user.
=# create table p (a text) partition by range (a collate "sv_SE");
=# create table c1 partition of p for values from ('z') to ('å');
=# insert into p values ('zirkoniumet');
> 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.
Of course documentation change is required. So, I'm going the
following direction.
For PG11
- Modify gram.y as v6. (or v8 and separate refactor patch for
PG12 if rejected:p)
- Add collation-check code as v6
- + documentaion
For PG10
- Modify gram.y as v8
- Add collation-check code as v6
- + documentation
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
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 ('A');
ERROR: no partition of relation "q" found for row
DETAIL: Partition key of the failing row contains (a) = (A).
So, user-specified collation is definitely considered for partitioning,
but you seem to be already aware of that per rest of your email.
> 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,
Right and it's because 'x' <= 'ö' per "sv_SE" which is the collation of
the partition key.
> =# 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 ('ö')...
I guess you meant to show this:
create table p (a text collate "en_US") partition by range (a);
create table c1 partition of p for values from ('x') to ('ö');
ERROR: empty range bound specified for partition "c1"
DETAIL: Specified lower bound ('x') is greater than or equal to upper
bound ('ö').
Because 'x' is not <= 'ö', per "en_US".
> This is because the collation of the key column is actually
> *used* to check the range bounds.
Yes.
> With this fix partition bound
> values can have collate specification and if it differs from key
> collation, it is just a mess.
I was trying to point out that there is no infrastructure to keep track of
the collations of the individual bound values. Neither in the system
catalog, nor in the PartitionBoundInfoData internal structure. So, even
if a user may specify collation for the individual bound value expressions
because the syntax allows, there is no place to put it or no occasion to
use it.
In the particular example that you provided, an informed user might be
able to deduce from the below DETAIL message that the problem might have
to do with collation.
DETAIL: Specified lower bound ('x') is greater than or equal to upper
bound ('ö').
Maybe, we should add a HINT to ask user to check the collation that's been
defined for the partition key. But checking the collation of the
partition bound expression and causing error when it doesn't match the
partition key's collation does not seem to improve the situation for users
in this case.
>> 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.
>
> DEFAULT value is not compared with anything, partition key
> expression *is* compared with column value and the collation is
> applied on the comparison.
>
> In the following case, the comparison between 'zirkoniumet' and
> 'z', 'å' must be performed with "sv_SE" since the range is empty
> if the collation were en_US and it is not the intention of the
> user.
>
> =# create table p (a text) partition by range (a collate "sv_SE");
> =# create table c1 partition of p for values from ('z') to ('å');
> =# insert into p values ('zirkoniumet');
It *will* be performed with "sv_SE", because that's the partition key's
collation and so it will be routed into c1. That doesn't have anything to
do with what collation was used for partition bounds.
Regarding why I brought up the DEFAULT into this discussion, see the
following example:
create table foo (a text collate "en_US" check (a >= 'x'));
alter table foo alter a set default 'ö' collate "sv_SE";
\d foo
Table "public.foo"
Column | Type | Collation | Nullable | Default
--------+------+-----------+----------+-----------------------------
a | text | en_US | | ('ö'::text COLLATE "sv_SE")
Check constraints:
"foo_a_check" CHECK (a >= 'x'::text)
insert into foo values (DEFAULT);
ERROR: new row for relation "foo" violates check constraint "foo_a_check"
DETAIL: Failing row contains (ö).
Here because the collation of the underlying column is "en_US" check
constraint expression a >= 'x' is evaluated using that collation and so
trying to insert the default value 'ö' fails. Specifying collation for
the default expression didn't change anything, but the user wasn't warned
about that.
Am I missing something?
Thanks,
Amit
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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 <partelem> part_elem
%type <list> part_params
%type <partboundspec> PartitionBoundSpec
! %type <node> partbound_datum PartitionRangeDatum
! %type <list> hash_partbound partbound_datum_list range_datum_list
%type <defelt> hash_partbound_elem
/*
--- 581,587 ----
%type <partelem> part_elem
%type <list> part_params
%type <partboundspec> PartitionBoundSpec
! %type <list> hash_partbound
%type <defelt> 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 ----
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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 ] <replaceable c
*** 87,95 ****
<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 } [, ...] ) |
WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REMAINDER <replaceable
class="parameter">numeric_literal</replaceable>)
<phrase>and <replaceable class="parameter">column_constraint</replaceable> is:</phrase>
--- 87,95 ----
<phrase>and <replaceable class="parameter">partition_bound_spec</replaceable> is:</phrase>
! 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>and <replaceable class="parameter">column_constraint</replaceable> is:</phrase>
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 ****
<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 } [, ...] ) |
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>
--- 86,94 ----
<phrase>and <replaceable class="parameter">partition_bound_spec</replaceable> is:</phrase>
! 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 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 | |
+ 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 5469434..140bf41 100644
*** a/src/test/regress/sql/create_table.sql
--- b/src/test/regress/sql/create_table.sql
*************** CREATE TABLE arrlp (a int[]) PARTITION B
*** 719,721 ****
--- 719,728 ----
CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
\d+ arrlp12
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
+ drop table boolspart;
On 2018/04/22 2:29, Tom Lane wrote: > Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
> On Apr 23, 2018, at 12:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
On 2018/04/24 4:33, Tom Lane wrote: > Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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. Thanks for making that change and committing. Regards, Amit
At Tue, 24 Apr 2018 09:23:03 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <bdb1d41a-a150-406c-cc66-c93c7625f388@lab.ntt.co.jp> > On 2018/04/24 4:33, Tom Lane wrote: > > Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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