Thread: pg_dump losing index column collations for unique and primary keys
Hello, I found that pg_dump fails to note down the collations used primary and unique keys supporting indexes. To reproduce: create table f(a text); create unique index f_pkey on f(a collate "C.UTF-8"); alter table f add primary key using index f_pkey; \d f pg_dump -t f The only way to dump it correctly is to create indexes explicitly and then use them in constraint definitions. Please could someone have a look at the patch attached. Thank you. Best, Alex
Attachment
Alexey Bashtanov <bashtanov@imap.cc> writes:
> I found that pg_dump fails to note down the collations used primary and
> unique keys supporting indexes.
> To reproduce:
> create table f(a text);
> create unique index f_pkey on f(a collate "C.UTF-8");
> alter table f add primary key using index f_pkey;
Hm. I kinda think that we should reject the above. The point of
ADD PRIMARY KEY USING INDEX ought to be to allow you to build
the index concurrently; it should not be something you can use to
create a DDL state that is impossible to arrive at with plain
ADD PRIMARY KEY.
As an example of the sort of problem that I'm worried about,
consider a situation where the index's collation has a different
notion of equality than the column's default collation has.
(Which is entirely possible now that we have nondeterministic
collations.) That's going to lead to weird restrictions on
whether the index really satisfies query WHERE conditions, and
I'd bet considerable money that we'd have bugs due to failing
to account for that.
In short, I'd say the bug here is not pg_dump's fault at all,
but failure to insist on collation match in ADD PRIMARY KEY
USING INDEX.
regards, tom lane
I wrote:
> In short, I'd say the bug here is not pg_dump's fault at all,
> but failure to insist on collation match in ADD PRIMARY KEY
> USING INDEX.
Concretely, I think we should do the attached.
I'm not quite sure whether we should back-patch this, though.
It's been wrong since we added collations, but the main impact
of a back-patch might be to break cases that were working more
or less okay for people.
A compromise idea is to back-patch only into v12, where the issue
became quite a lot more important due to nondeterministic
collations.
Thoughts?
regards, tom lane
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index ee47547..b761fdf 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -2147,15 +2147,17 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
if (i < index_form->indnkeyatts)
{
/*
- * Insist on default opclass and sort options. While the
- * index would still work as a constraint with non-default
- * settings, it might not provide exactly the same uniqueness
- * semantics as you'd get from a normally-created constraint;
- * and there's also the dump/reload problem mentioned above.
+ * Insist on default opclass, collation, and sort options.
+ * While the index would still work as a constraint with
+ * non-default settings, it might not provide exactly the same
+ * uniqueness semantics as you'd get from a normally-created
+ * constraint; and there's also the dump/reload problem
+ * mentioned above.
*/
defopclass = GetDefaultOpClass(attform->atttypid,
index_rel->rd_rel->relam);
if (indclass->values[i] != defopclass ||
+ attform->attcollation != index_rel->rd_indcollation[i] ||
index_rel->rd_indoption[i] != 0)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 1cdb7a9..645ae2c 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1479,6 +1479,19 @@ primary key, btree, for table "public.cwi_test"
DROP INDEX cwi_replaced_pkey; -- Should fail; a constraint depends on it
ERROR: cannot drop index cwi_replaced_pkey because constraint cwi_replaced_pkey on table cwi_test requires it
HINT: You can drop constraint cwi_replaced_pkey on table cwi_test instead.
+-- Check that non-default index options are rejected
+CREATE UNIQUE INDEX cwi_uniq3_idx ON cwi_test(a desc);
+ALTER TABLE cwi_test ADD UNIQUE USING INDEX cwi_uniq3_idx; -- fail
+ERROR: index "cwi_uniq3_idx" column number 1 does not have default sorting behavior
+LINE 1: ALTER TABLE cwi_test ADD UNIQUE USING INDEX cwi_uniq3_idx;
+ ^
+DETAIL: Cannot create a primary key or unique constraint using such an index.
+CREATE UNIQUE INDEX cwi_uniq4_idx ON cwi_test(b collate "POSIX");
+ALTER TABLE cwi_test ADD UNIQUE USING INDEX cwi_uniq4_idx; -- fail
+ERROR: index "cwi_uniq4_idx" column number 1 does not have default sorting behavior
+LINE 1: ALTER TABLE cwi_test ADD UNIQUE USING INDEX cwi_uniq4_idx;
+ ^
+DETAIL: Cannot create a primary key or unique constraint using such an index.
DROP TABLE cwi_test;
-- ADD CONSTRAINT USING INDEX is forbidden on partitioned tables
CREATE TABLE cwi_test(a int) PARTITION BY hash (a);
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index 7659808..73a55ea 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -538,6 +538,12 @@ ALTER TABLE cwi_test DROP CONSTRAINT cwi_uniq_idx,
DROP INDEX cwi_replaced_pkey; -- Should fail; a constraint depends on it
+-- Check that non-default index options are rejected
+CREATE UNIQUE INDEX cwi_uniq3_idx ON cwi_test(a desc);
+ALTER TABLE cwi_test ADD UNIQUE USING INDEX cwi_uniq3_idx; -- fail
+CREATE UNIQUE INDEX cwi_uniq4_idx ON cwi_test(b collate "POSIX");
+ALTER TABLE cwi_test ADD UNIQUE USING INDEX cwi_uniq4_idx; -- fail
+
DROP TABLE cwi_test;
-- ADD CONSTRAINT USING INDEX is forbidden on partitioned tables
Hi Tom, >> In short, I'd say the bug here is not pg_dump's fault at all, >> but failure to insist on collation match in ADD PRIMARY KEY >> USING INDEX. While being entirely reasonable especially with the non-deterministic collations, this breaks down an important facility of changing a column collation without rewriting it even if it is a part of a unique constraint: first change the PK then the column itself. > Concretely, I think we should do the attached. While being entirely reasonable especially with the non-deterministic collations, this breaks down an important facility of changing a column collation without rewriting it even if it is a part of a unique constraint: first change the PK then the column itself. Disallowing changing the direction (ASC/DESC) also looks cruel to me. BTW with playing with this stuff I came across another issue, which seems unrelated to collations: 2019-12-03 19:08:26 alexey@[local]/alexey=# \d g Table "public.g" ┌────────┬──────┬────────────┬──────────┬─────────┐ │ Column │ Type │ Collation │ Nullable │ Default │ ├────────┼──────┼────────────┼──────────┼─────────┤ │ a │ text │ en_US.utf8 │ not null │ │ └────────┴──────┴────────────┴──────────┴─────────┘ Indexes: "g_pkey" PRIMARY KEY, btree (a) "g_a_idx" UNIQUE, btree (a) 2019-12-03 19:08:27 alexey@[local]/alexey=# begin; alter table g drop constraint g_pkey, add primary key using index g_a_idx, alter column a type text; BEGIN Time: 0.421 ms ERROR: could not open relation with OID 16417 Time: 9.990 ms Is it something known? From user's perspective I think an ideal solution would be to fix the above and to enforce the PK/UK and column collation to be the same only by the end of ALTER TABLE command (and document this, as it isn't very obvious). I haven't yet looked in the code though to see how comfortable would that be to implement it. Best, Alex
Alexey Bashtanov <bashtanov@imap.cc> writes: >> In short, I'd say the bug here is not pg_dump's fault at all, >> but failure to insist on collation match in ADD PRIMARY KEY >> USING INDEX. > While being entirely reasonable especially with the non-deterministic > collations, > this breaks down an important facility of changing a column collation > without > rewriting it even if it is a part of a unique constraint: first change > the PK then the > column itself. Well, I disagree that that's a high-priority use case, but it seems to me that you can still do it. You just can't call the index the pkey until the column collation agrees. So roughly it'd be * CREATE UNIQUE INDEX ... (col COLLATE new-collation) * Drop old pkey (the new index is still enforcing uniqueness) * ALTER TABLE ... ALTER COLUMN ... COLLATE new-collation * ALTER TABLE ADD PRIMARY KEY USING INDEX These seem to me to be pretty much exactly the same steps you'd need today, although maybe the current code is more forgiving about their ordering. > Disallowing changing the direction (ASC/DESC) also looks cruel to me. That restriction has been there since day one, and we have had a grand total of zero complaints about it. > BTW with playing with this stuff I came across another issue, > which seems unrelated to collations: Yeah, see https://www.postgresql.org/message-id/flat/10365.1558909428@sss.pgh.pa.us If you'd like to help move things along by reviewing that patch, it'd be great. regards, tom lane
> Well, I disagree that that's a high-priority use case, but it seems to me > that you can still do it. You just can't call the index the pkey until > the column collation agrees. So roughly it'd be > > * CREATE UNIQUE INDEX ... (col COLLATE new-collation) > * Drop old pkey (the new index is still enforcing uniqueness) > * ALTER TABLE ... ALTER COLUMN ... COLLATE new-collation > * ALTER TABLE ADD PRIMARY KEY USING INDEX > > These seem to me to be pretty much exactly the same steps you'd need > today, although maybe the current code is more forgiving about > their ordering. Agreed. >> Disallowing changing the direction (ASC/DESC) also looks cruel to me. > That restriction has been there since day one, and we have had a grand > total of zero complaints about it. True, my bad. What do you think of making pg_dump warn the user if they are trying to dump a weird PK/UK which has collations in index and column not matching? And maybe even throw an error in case of --binary-upgrade? >> BTW with playing with this stuff I came across another issue, >> which seems unrelated to collations: > Yeah, see > https://www.postgresql.org/message-id/flat/10365.1558909428@sss.pgh.pa.us > If you'd like to help move things along by reviewing that patch, > it'd be great. I'll have a look. Best, Alex
Alexey Bashtanov <bashtanov@imap.cc> writes:
> What do you think of making pg_dump warn the user if they are trying
> to dump a weird PK/UK which has collations in index and column not matching?
> And maybe even throw an error in case of --binary-upgrade?
I can't get excited about spending any additional effort on this issue.
If I thought it were actually happening in the field to any significant
extent, I'd be more interested in supporting the case instead of just
disallowing it. But I think the actual occurrences are epsilon, so
there are better places to be spending our time.
regards, tom lane