pg.ddx.io  pgsql-bugs@postgresql.org mailing list archive  
help / color / mirror / Atom feed
pg_dump losing index column collations for unique and primary keys
7+ messages / 2 participants
[nested] [flat]

* pg_dump losing index column collations for unique and primary keys
@ 2019-12-03 16:35  Alexey Bashtanov <bashtanov@imap.cc>
  0 siblings, 1 reply; 7+ messages in thread

From: Alexey Bashtanov @ 2019-12-03 16:35 UTC (permalink / raw)
  To: pgsql-bugs

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

Attachments:

  [text/x-patch] pg_dump_pk_collation_v001.patch (9.5K, ../../b05ce36a-cefb-ca5e-b386-a400535b1c0b@imap.cc/2-pg_dump_pk_collation_v001.patch)
  download | inline diff:
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 37432a6f4b..75f1e358d8 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -775,6 +775,7 @@ getDumpableObjects(DumpableObject ***objs, int *numObjs)
 /*
  * Add a dependency link to a DumpableObject
  *
+ * To force refId appear before dobj.
  * Note: duplicate dependencies are currently not eliminated
  */
 void
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 08658c8e86..01d5b37383 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7132,9 +7132,19 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
 				constrinfo[j].condomain = NULL;
 				constrinfo[j].contype = contype;
 				if (contype == 'x')
+				{
 					constrinfo[j].condef = pg_strdup(PQgetvalue(res, j, i_condef));
+				}
 				else
+				{
 					constrinfo[j].condef = NULL;
+					/*
+					 * for these types constraint definition refers to the
+					 * index definition, which we also dump, so we need
+					 * to maintain the order
+					 */
+					addObjectDependency(&constrinfo[j].dobj, indxinfo[j].dobj.dumpId);
+				}
 				constrinfo[j].confrelid = InvalidOid;
 				constrinfo[j].conindex = indxinfo[j].dobj.dumpId;
 				constrinfo[j].condeferrable = *(PQgetvalue(res, j, i_condeferrable)) == 't';
@@ -7142,12 +7152,12 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
 				constrinfo[j].conislocal = true;
 				constrinfo[j].separate = true;
 
-				indxinfo[j].indexconstraint = constrinfo[j].dobj.dumpId;
+				indxinfo[j].indexconstraint = &constrinfo[j];
 			}
 			else
 			{
 				/* Plain secondary index */
-				indxinfo[j].indexconstraint = 0;
+				indxinfo[j].indexconstraint = NULL;
 			}
 		}
 
@@ -16355,7 +16365,9 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
 {
 	DumpOptions *dopt = fout->dopt;
 	TableInfo  *tbinfo = indxinfo->indextable;
-	bool		is_constraint = (indxinfo->indexconstraint != 0);
+	bool		is_exclusion_constraint =
+		(indxinfo->indexconstraint != NULL &&
+		 indxinfo->indexconstraint->contype == 'x');
 	PQExpBuffer q;
 	PQExpBuffer delq;
 	char	   *qindxname;
@@ -16369,13 +16381,16 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
 	qindxname = pg_strdup(fmtId(indxinfo->dobj.name));
 
 	/*
-	 * If there's an associated constraint, don't dump the index per se, but
+	 * If there's an associated EXCLUDE constraint, don't dump the index per se, but
 	 * do dump any comment for it.  (This is safe because dependency ordering
 	 * will have ensured the constraint is emitted first.)	Note that the
 	 * emitted comment has to be shown as depending on the constraint, not the
 	 * index, in such cases.
+	 * We need to dump indexes that support PRIMARY and UNIQUE constraints, as
+	 * such an index may have special options like collations which cannot be
+	 * expressed in the ALTER TABLE ... ADD CONSTRAINT statement.
 	 */
-	if (!is_constraint)
+	if (!is_exclusion_constraint)
 	{
 		char	   *indstatcols = indxinfo->indstatcols;
 		char	   *indstatvals = indxinfo->indstatvals;
@@ -16469,8 +16484,9 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
 					tbinfo->dobj.namespace->dobj.name,
 					tbinfo->rolname,
 					indxinfo->dobj.catId, 0,
-					is_constraint ? indxinfo->indexconstraint :
-					indxinfo->dobj.dumpId);
+					is_exclusion_constraint ?
+						indxinfo->indexconstraint->dobj.dumpId :
+						indxinfo->dobj.dumpId);
 
 	destroyPQExpBuffer(q);
 	destroyPQExpBuffer(delq);
@@ -16635,47 +16651,9 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo)
 		}
 		else
 		{
-			appendPQExpBuffer(q, "%s (",
-							  coninfo->contype == 'p' ? "PRIMARY KEY" : "UNIQUE");
-			for (k = 0; k < indxinfo->indnkeyattrs; k++)
-			{
-				int			indkey = (int) indxinfo->indkeys[k];
-				const char *attname;
-
-				if (indkey == InvalidAttrNumber)
-					break;
-				attname = getAttrName(indkey, tbinfo);
-
-				appendPQExpBuffer(q, "%s%s",
-								  (k == 0) ? "" : ", ",
-								  fmtId(attname));
-			}
-
-			if (indxinfo->indnkeyattrs < indxinfo->indnattrs)
-				appendPQExpBufferStr(q, ") INCLUDE (");
-
-			for (k = indxinfo->indnkeyattrs; k < indxinfo->indnattrs; k++)
-			{
-				int			indkey = (int) indxinfo->indkeys[k];
-				const char *attname;
-
-				if (indkey == InvalidAttrNumber)
-					break;
-				attname = getAttrName(indkey, tbinfo);
-
-				appendPQExpBuffer(q, "%s%s",
-								  (k == indxinfo->indnkeyattrs) ? "" : ", ",
-								  fmtId(attname));
-			}
-
-			appendPQExpBufferChar(q, ')');
-
-			if (nonemptyReloptions(indxinfo->indreloptions))
-			{
-				appendPQExpBufferStr(q, " WITH (");
-				appendReloptionsArrayAH(q, indxinfo->indreloptions, "", fout);
-				appendPQExpBufferChar(q, ')');
-			}
+			appendPQExpBuffer(q, "%s USING INDEX %s",
+							  coninfo->contype == 'p' ? "PRIMARY KEY" : "UNIQUE",
+							  fmtId(indxinfo->dobj.name));
 
 			if (coninfo->condeferrable)
 			{
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 7b2c1524a5..7984566b93 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -351,6 +351,29 @@ typedef struct _tableDataInfo
 	char	   *filtercond;		/* WHERE condition to limit rows dumped */
 } TableDataInfo;
 
+/*
+ * struct ConstraintInfo is used for all constraint types.  However we
+ * use a different objType for foreign key constraints, to make it easier
+ * to sort them the way we want.
+ *
+ * Note: condeferrable and condeferred are currently only valid for
+ * unique/primary-key constraints.  Otherwise that info is in condef.
+ */
+typedef struct _constraintInfo
+{
+	DumpableObject dobj;
+	TableInfo  *contable;		/* NULL if domain constraint */
+	TypeInfo   *condomain;		/* NULL if table constraint */
+	char		contype;
+	char	   *condef;			/* definition, if CHECK or FOREIGN KEY */
+	Oid			confrelid;		/* referenced table, if FOREIGN KEY */
+	DumpId		conindex;		/* identifies associated index if any */
+	bool		condeferrable;	/* true if constraint is DEFERRABLE */
+	bool		condeferred;	/* true if constraint is INITIALLY DEFERRED */
+	bool		conislocal;		/* true if constraint has local definition */
+	bool		separate;		/* true if must dump as separate item */
+} ConstraintInfo;
+
 typedef struct _indxInfo
 {
 	DumpableObject dobj;
@@ -369,8 +392,8 @@ typedef struct _indxInfo
 	Oid			parentidx;		/* if partitioned, parent index OID */
 	SimplePtrList partattaches;	/* if partitioned, partition attach objects */
 
-	/* if there is an associated constraint object, its dumpId: */
-	DumpId		indexconstraint;
+	/* an associated constraint object, or NULL if none */
+	ConstraintInfo *indexconstraint;
 } IndxInfo;
 
 typedef struct _indexAttachInfo
@@ -427,29 +450,6 @@ typedef struct _evttriggerInfo
 	char		evtenabled;
 } EventTriggerInfo;
 
-/*
- * struct ConstraintInfo is used for all constraint types.  However we
- * use a different objType for foreign key constraints, to make it easier
- * to sort them the way we want.
- *
- * Note: condeferrable and condeferred are currently only valid for
- * unique/primary-key constraints.  Otherwise that info is in condef.
- */
-typedef struct _constraintInfo
-{
-	DumpableObject dobj;
-	TableInfo  *contable;		/* NULL if domain constraint */
-	TypeInfo   *condomain;		/* NULL if table constraint */
-	char		contype;
-	char	   *condef;			/* definition, if CHECK or FOREIGN KEY */
-	Oid			confrelid;		/* referenced table, if FOREIGN KEY */
-	DumpId		conindex;		/* identifies associated index if any */
-	bool		condeferrable;	/* true if constraint is DEFERRABLE */
-	bool		condeferred;	/* true if constraint is INITIALLY DEFERRED */
-	bool		conislocal;		/* true if constraint has local definition */
-	bool		separate;		/* true if must dump as separate item */
-} ConstraintInfo;
-
 typedef struct _procLangInfo
 {
 	DumpableObject dobj;
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 1163420491..9938e2754a 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -615,10 +615,26 @@ my %tests = (
 		},
 	},
 
+	'CREATE UNIQUE INDEX ON test_table ... (for primary key)' => {
+		regexp => qr/^
+			\QCREATE UNIQUE INDEX test_table_pkey ON dump_test.test_table USING btree (col1);\E
+			/xm,
+		like => {
+			%full_runs,
+			%dump_test_schema_runs,
+			only_dump_test_table => 1,
+			section_post_data    => 1,
+		},
+		unlike => {
+			exclude_dump_test_schema => 1,
+			exclude_test_table       => 1,
+		},
+	},
+
 	'ALTER TABLE ONLY test_table ADD CONSTRAINT ... PRIMARY KEY' => {
 		regexp => qr/^
 			\QALTER TABLE ONLY dump_test.test_table\E \n^\s+
-			\QADD CONSTRAINT test_table_pkey PRIMARY KEY (col1);\E
+			\QADD CONSTRAINT test_table_pkey PRIMARY KEY USING INDEX test_table_pkey;\E
 			/xm,
 		like => {
 			%full_runs,
@@ -2607,6 +2623,17 @@ my %tests = (
 		},
 	},
 
+	'CREATE UNIQUE INDEX ON measurement ... (for primary key)' => {
+		all_runs     => 1,
+		catch_all    => 'CREATE ... commands',
+		regexp => qr/^
+			\QCREATE UNIQUE INDEX measurement_pkey ON ONLY dump_test.measurement USING btree (city_id, logdate);\E
+		/xm,
+		like =>
+		  { %full_runs, %dump_test_schema_runs, section_post_data => 1, },
+		unlike => { exclude_dump_test_schema => 1, },
+	},
+
 	'ALTER TABLE measurement PRIMARY KEY' => {
 		all_runs     => 1,
 		catch_all    => 'CREATE ... commands',
@@ -2615,7 +2642,7 @@ my %tests = (
 		  'ALTER TABLE dump_test.measurement ADD PRIMARY KEY (city_id, logdate);',
 		regexp => qr/^
 			\QALTER TABLE ONLY dump_test.measurement\E \n^\s+
-			\QADD CONSTRAINT measurement_pkey PRIMARY KEY (city_id, logdate);\E
+			\QADD CONSTRAINT measurement_pkey PRIMARY KEY USING INDEX measurement_pkey;\E
 		/xm,
 		like =>
 		  { %full_runs, %dump_test_schema_runs, section_post_data => 1, },

^ permalink  raw  reply  [nested|flat] 7+ messages in thread

* Re: pg_dump losing index column collations for unique and primary keys
@ 2019-12-03 16:57  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Alexey Bashtanov <bashtanov@imap.cc>
  0 siblings, 1 reply; 7+ messages in thread

From: Tom Lane @ 2019-12-03 16:57 UTC (permalink / raw)
  To: Alexey Bashtanov <bashtanov@imap.cc>; +Cc: pgsql-bugs

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





^ permalink  raw  reply  [nested|flat] 7+ messages in thread

* Re: pg_dump losing index column collations for unique and primary keys
@ 2019-12-03 18:24  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 7+ messages in thread

From: Tom Lane @ 2019-12-03 18:24 UTC (permalink / raw)
  To: Alexey Bashtanov <bashtanov@imap.cc>; +Cc: pgsql-bugs

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

Attachments:

  [text/x-diff] disallow-collation-mismatch-in-ADD-USING-INDEX.patch (3.6K, ../../6484.1575397478@sss.pgh.pa.us/2-disallow-collation-mismatch-in-ADD-USING-INDEX.patch)
  download | inline diff:
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

^ permalink  raw  reply  [nested|flat] 7+ messages in thread

* Re: pg_dump losing index column collations for unique and primary keys
@ 2019-12-03 19:30  Alexey Bashtanov <bashtanov@imap.cc>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 7+ messages in thread

From: Alexey Bashtanov @ 2019-12-03 19:30 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs

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





^ permalink  raw  reply  [nested|flat] 7+ messages in thread

* Re: pg_dump losing index column collations for unique and primary keys
@ 2019-12-03 19:52  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Alexey Bashtanov <bashtanov@imap.cc>
  0 siblings, 1 reply; 7+ messages in thread

From: Tom Lane @ 2019-12-03 19:52 UTC (permalink / raw)
  To: Alexey Bashtanov <bashtanov@imap.cc>; +Cc: pgsql-bugs

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





^ permalink  raw  reply  [nested|flat] 7+ messages in thread

* Re: pg_dump losing index column collations for unique and primary keys
@ 2019-12-04 13:51  Alexey Bashtanov <bashtanov@imap.cc>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 7+ messages in thread

From: Alexey Bashtanov @ 2019-12-04 13:51 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs


> 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





^ permalink  raw  reply  [nested|flat] 7+ messages in thread

* Re: pg_dump losing index column collations for unique and primary keys
@ 2019-12-04 15:48  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Alexey Bashtanov <bashtanov@imap.cc>
  0 siblings, 0 replies; 7+ messages in thread

From: Tom Lane @ 2019-12-04 15:48 UTC (permalink / raw)
  To: Alexey Bashtanov <bashtanov@imap.cc>; +Cc: pgsql-bugs

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





^ permalink  raw  reply  [nested|flat] 7+ messages in thread


end of thread, other threads:[~2019-12-04 15:48 UTC | newest]

Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-12-03 16:35 pg_dump losing index column collations for unique and primary keys Alexey Bashtanov <bashtanov@imap.cc>
2019-12-03 16:57 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-12-03 18:24   ` Tom Lane <tgl@sss.pgh.pa.us>
2019-12-03 19:30     ` Alexey Bashtanov <bashtanov@imap.cc>
2019-12-03 19:52       ` Tom Lane <tgl@sss.pgh.pa.us>
2019-12-04 13:51         ` Alexey Bashtanov <bashtanov@imap.cc>
2019-12-04 15:48           ` Tom Lane <tgl@sss.pgh.pa.us>

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox