Date: Thu, 10 Oct 2019 15:43:03 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, buschmann@nidsa.net,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16045: vacuum_db crash and illegal memory alloc after
 pg_upgrade from PG11 to PG12
Message-ID: <20191010134303.bkwjzchjb2w66iro@development>
References: <16045-673e8fa6b5ace196@postgresql.org>
 <20191009132436.uugh4r3be6vw6ex4@development>
 <20191009135907.km2b5nrzflcz6xey@development>
 <20191009141841.my6k44wfiaquj3ks@development>
 <20191009230759.ge66neof5qj627a2@development>
 <19314.1570663125@sss.pgh.pa.us>
 <20191009232836.jg77wccqwy5fgs6s@development>
 <20265.1570664514@sss.pgh.pa.us>
 <20191010014813.3b2btmkakhrskwgd@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="to4lpegnfgybv64f"
Content-Disposition: inline
In-Reply-To: <20191010014813.3b2btmkakhrskwgd@alap3.anarazel.de>
User-Agent: NeoMutt/20180716
Precedence: bulk


--to4lpegnfgybv64f
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline

On Wed, Oct 09, 2019 at 06:48:13PM -0700, Andres Freund wrote:
>On 2019-10-09 19:41:54 -0400, Tom Lane wrote:
>> Also, even if we did try to silently convert such columns that way,
>> I bet we'd get other bug reports about "why'd my columns suddenly
>> change type?".  So I'd rather force the user to be involved.
>
>+1

Fair enough, attached is a patch doing that, I think. Maybe the file
should be named differently, as it contains other objects than just
tables.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--to4lpegnfgybv64f
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
 filename="pg-upgrade-sql-identifier-fix.patch"

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 617270f101..c6dc50f3e6 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -108,6 +108,13 @@ check_and_dump_old_cluster(bool live_check)
 	if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1100)
 		check_for_tables_with_oids(&old_cluster);
 
+	/*
+	 * PG 12 changed the 'sql_identifier' type storage format, so we need
+	 * to prevent upgrade when used in user objects (tables, indexes, ...)
+	 */
+	if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1100)
+		old_11_check_for_sql_identifier_data_type_usage(&old_cluster);
+
 	/*
 	 * Pre-PG 10 allowed tables with 'unknown' type columns and non WAL logged
 	 * hash indexes
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 5d31750d86..63574b51bc 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -458,6 +458,8 @@ void		old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster);
 void		old_9_6_invalidate_hash_indexes(ClusterInfo *cluster,
 											bool check_mode);
 
+void		old_11_check_for_sql_identifier_data_type_usage(ClusterInfo *cluster);
+
 /* parallel.c */
 void		parallel_exec_prog(const char *log_file, const char *opt_log_file,
 							   const char *fmt,...) pg_attribute_printf(3, 4);
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index 10cb362e09..8d766d3d3a 100644
--- a/src/bin/pg_upgrade/version.c
+++ b/src/bin/pg_upgrade/version.c
@@ -399,3 +399,101 @@ old_9_6_invalidate_hash_indexes(ClusterInfo *cluster, bool check_mode)
 	else
 		check_ok();
 }
+
+/*
+ * old_11_check_for_sql_identifier_data_type_usage()
+ *	11 -> 12
+ *	In 12, the sql_identifier data type was switched from name to varchar,
+ *	which does affect the storage (name is by-ref, but not varlena). This
+ *	means user tables using sql_identifier for columns are broken because
+ *	the on-disk format is different.
+ *
+ *	We need to check all objects that might store sql_identifier on disk,
+ *	i.e. tables, matviews and indexes. Also check composite types in case
+ *	they are used in this context.
+ */
+void
+old_11_check_for_sql_identifier_data_type_usage(ClusterInfo *cluster)
+{
+	int			dbnum;
+	FILE	   *script = NULL;
+	bool		found = false;
+	char		output_path[MAXPGPATH];
+
+	prep_status("Checking for invalid \"sql_identifier\" user columns");
+
+	snprintf(output_path, sizeof(output_path), "tables_using_sql_identifier.txt");
+
+	for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+	{
+		PGresult   *res;
+		bool		db_used = false;
+		int			ntups;
+		int			rowno;
+		int			i_nspname,
+					i_relname,
+					i_attname;
+		DbInfo	   *active_db = &cluster->dbarr.dbs[dbnum];
+		PGconn	   *conn = connectToServer(cluster, active_db->db_name);
+
+		res = executeQueryOrDie(conn,
+								"SELECT n.nspname, c.relname, a.attname "
+								"FROM	pg_catalog.pg_class c, "
+								"		pg_catalog.pg_namespace n, "
+								"		pg_catalog.pg_attribute a "
+								"WHERE	c.oid = a.attrelid AND "
+								"		NOT a.attisdropped AND "
+								"		a.atttypid = 'information_schema.sql_identifier'::pg_catalog.regtype AND "
+								"		c.relkind IN ("
+								CppAsString2(RELKIND_RELATION) ", "
+								CppAsString2(RELKIND_COMPOSITE_TYPE) ", "
+								CppAsString2(RELKIND_MATVIEW) ", "
+								CppAsString2(RELKIND_INDEX) ") AND "
+								"		c.relnamespace = n.oid AND "
+		/* exclude possible orphaned temp tables */
+								"		n.nspname !~ '^pg_temp_' AND "
+								"		n.nspname !~ '^pg_toast_temp_' AND "
+								"		n.nspname NOT IN ('pg_catalog', 'information_schema')");
+
+		ntups = PQntuples(res);
+		i_nspname = PQfnumber(res, "nspname");
+		i_relname = PQfnumber(res, "relname");
+		i_attname = PQfnumber(res, "attname");
+		for (rowno = 0; rowno < ntups; rowno++)
+		{
+			found = true;
+			if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+				pg_fatal("could not open file \"%s\": %s\n", output_path,
+						 strerror(errno));
+			if (!db_used)
+			{
+				fprintf(script, "Database: %s\n", active_db->db_name);
+				db_used = true;
+			}
+			fprintf(script, "  %s.%s.%s\n",
+					PQgetvalue(res, rowno, i_nspname),
+					PQgetvalue(res, rowno, i_relname),
+					PQgetvalue(res, rowno, i_attname));
+		}
+
+		PQclear(res);
+
+		PQfinish(conn);
+	}
+
+	if (script)
+		fclose(script);
+
+	if (found)
+	{
+		pg_log(PG_REPORT, "fatal\n");
+		pg_fatal("Your installation contains the \"sql_identifier\" data type in user tables\n"
+				 "and/or indexes.  The on-disk format for this data type has changed, so this\n"
+				 "cluster cannot currently be upgraded.  You can remove the problem tables or\n"
+				 "change the data type to \"name\" and restart the upgrade.\n"
+				 "A list of the problem columns is in the file:\n"
+				 "    %s\n\n", output_path);
+	}
+	else
+		check_ok();
+}

--to4lpegnfgybv64f--