From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: buschmann@nidsa.net
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16045: vacuum_db crash and illegal memory alloc after pg_upgrade from PG11 to PG12
Date: Thu, 10 Oct 2019 15:43:03 +0200
Message-ID: <20191010134303.bkwjzchjb2w66iro@development> (raw)
In-Reply-To: <20191010014813.3b2btmkakhrskwgd@alap3.anarazel.de>
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>
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
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.cindex 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.hindex 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.cindex 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();+}Attachments:
[text/plain] pg-upgrade-sql-identifier-fix.patch (4.8K, ../20191010134303.bkwjzchjb2w66iro@development/2-pg-upgrade-sql-identifier-fix.patch)
download | inline diff:diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.cindex 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.hindex 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.cindex 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();+}
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, andres@anarazel.de, 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
In-Reply-To: <20191010134303.bkwjzchjb2w66iro@development>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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