To: pgsql-bugs@postgresql.org
From: Alexey Bashtanov <bashtanov@imap.cc>
Subject: pg_dump losing index column collations for unique and primary keys
Message-ID: <b05ce36a-cefb-ca5e-b386-a400535b1c0b@imap.cc>
Date: Tue, 3 Dec 2019 16:35:20 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="------------14B21F33C79FA7317FDD6DD7"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------14B21F33C79FA7317FDD6DD7
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

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

--------------14B21F33C79FA7317FDD6DD7
Content-Type: text/x-patch;
 name="pg_dump_pk_collation_v001.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="pg_dump_pk_collation_v001.patch"

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, },

--------------14B21F33C79FA7317FDD6DD7--