Hi,
On 2018/11/07 0:10, Alvaro Herrera wrote:
> Looking over the stuff in gram.y (to make sure there's nothing that
> could be lost), I noticed that we're losing the COLLATE clause when they
> are added to columns in partitions. I would expect part1 to end up with
> collation es_CL, or alternatively that the command throws an error:
>
> 55462 10.6 138851=# create table part (a text collate "en_US") partition by
> range (a);
> CREATE TABLE
> Duración: 23,511 ms
> 55462 10.6 138851=# create table part1 partition of part (a collate "es_CL")
> for values from ('ca') to ('cu');
> CREATE TABLE
> Duración: 111,551 ms
> 55462 10.6 138851=# \d part
> Tabla «public.part»
> Columna │ Tipo │ Collation │ Nullable │ Default
> ─────────┼──────┼───────────┼──────────┼─────────
> a │ text │ en_US │ │
> Partition key: RANGE (a)
> Number of partitions: 1 (Use \d+ to list them.)
>
> 55462 10.6 138851=# \d part1
> Tabla «public.part1»
> Columna │ Tipo │ Collation │ Nullable │ Default
> ─────────┼──────┼───────────┼──────────┼─────────
> a │ text │ en_US │ │
> Partition of: part FOR VALUES FROM ('ca') TO ('cu')
>
>
> (This case is particularly bothersome because the column is the
> partition key, so the partition range bounds would differ depending on
> which collation is used to compare. I assume we'd always want to use
> the parent table's collation; so there's even a stronger case for
> raising an error if it doesn't match the parent's. However, this case
> could arise for other columns too, where it's not *so* bad, but still
> not completely correct I think.)
Thank you for investigating.
I think the result in this case should be an error, just as it would in
the regular inheritance case.
create table parent (a text);
create table child (a text collate "en_US") inherits (parent);
NOTICE: merging column "a" with inherited definition
ERROR: column "a" has a collation conflict
DETAIL: "default" versus "en_US"
In fact, I see that ATTACH PARTITION rejects a partition if collations
don't match.
create table part (a text collate "en_US") partition by range (a);
create table part1 (a text collate "es_CL");
alter table part attach partition part1 for values from ('ca') to ('cu');
ERROR: child table "part1" has different collation for column "a"
> This happens on unpatched code, and doesn't seem covered by any tests.
> However, this seems an independent bug that isn't affected by this
> patch.
>
> The only other things there are deferrability markers, which seem to be
> propagated in a relatively sane fashion (but no luck if you want to add
> foreign keys with mismatching deferrability than the parent's -- you
> just get a dupe, and there's no way in the grammar to change the flags
> for the existing constraint). But you can add a UNIQUE DEFERRED
> constraint in a partition that wasn't there in the parent, for example.
Looking again at MergeAttributes, it seems that the fix for disallowing
mismatching collations is not that invasive. PFA a patch that applies on
top of your 0001-Revise-attribute-handling-code-on-partition-creation.patch.
I haven't looked closely at the cases involving deferrability markers though.
Thanks,
Amit
From d2d2489e4978366b4fb489c60d85540264ba1064 Mon Sep 17 00:00:00 2001
From: amit <[email protected]>
Date: Wed, 7 Nov 2018 10:32:14 +0900
Subject: [PATCH 2/2] Disallow creating partitions with mismatching collations
for columns
---
src/backend/commands/tablecmds.c | 21 +++++++++++++++++++++
src/test/regress/expected/create_table.out | 6 ++++++
src/test/regress/sql/create_table.sql | 5 +++++
3 files changed, 32 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a0279ae383..fbf902143b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2446,6 +2446,10 @@ MergeAttributes(List *schema, List *supers, char
relpersistence,
if (strcmp(coldef->colname, restdef->colname)
== 0)
{
+ Oid defTypeId;
+ int32 deftypmod;
+ Oid newCollId;
+
found = true;
coldef->is_not_null |=
restdef->is_not_null;
@@ -2465,6 +2469,23 @@ MergeAttributes(List *schema, List *supers, char
relpersistence,
coldef->raw_default =
restdef->raw_default;
coldef->cooked_default = NULL;
}
+
+ /*
+ * Collation must be same, so error out
if a different one
+ * specified for the partition.
+ */
+ typenameTypeIdAndMod(NULL,
coldef->typeName,
+
&defTypeId, &deftypmod);
+ newCollId = GetColumnDefCollation(NULL,
restdef,
+
defTypeId);
+ if (newCollId != coldef->collOid)
+ ereport(ERROR,
+
(errcode(ERRCODE_COLLATION_MISMATCH),
+ errmsg("column
\"%s\" has a collation conflict",
+
coldef->colname),
+
errdetail("\"%s\" versus \"%s\"",
+
get_collation_name(newCollId),
+
get_collation_name(coldef->collOid))));
}
}
diff --git a/src/test/regress/expected/create_table.out
b/src/test/regress/expected/create_table.out
index bfc0dc7f6d..c921c8dc36 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -744,6 +744,12 @@ DETAIL: Failing row contains (1, null).
Partition of: parted_notnull_inh_test FOR VALUES IN (1)
drop table parted_notnull_inh_test;
+-- check that conflicting COLLATE clause for partition disallowed
+create table parted_collate_must_match (a text collate "C") partition by range
(a);
+create table parted_collate_must_match1 partition of parted_collate_must_match
(a collate "POSIX") for values from ('a') to ('z');
+ERROR: column "a" has a collation conflict
+DETAIL: "POSIX" versus "C"
+drop table parted_collate_must_match;
-- Partition bound in describe output
\d+ part_b
Table "public.part_b"
diff --git a/src/test/regress/sql/create_table.sql
b/src/test/regress/sql/create_table.sql
index ee49e82a62..3875b38fbd 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -662,6 +662,11 @@ insert into parted_notnull_inh_test (b) values (null);
\d parted_notnull_inh_test1
drop table parted_notnull_inh_test;
+-- check that conflicting COLLATE clause for partition disallowed
+create table parted_collate_must_match (a text collate "C") partition by range
(a);
+create table parted_collate_must_match1 partition of parted_collate_must_match
(a collate "POSIX") for values from ('a') to ('z');
+drop table parted_collate_must_match;
+
-- Partition bound in describe output
\d+ part_b
--
2.11.0
