From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>
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: Sun, 13 Oct 2019 02:10:32 +0200
Message-ID: <20191013001032.lqrn4kqaovhrf77l@development> (raw)
In-Reply-To: <20191010204022.6hiiubm2cueu3ghk@development>
References: <20191009230759.ge66neof5qj627a2@development>
<19314.1570663125@sss.pgh.pa.us>
<20191009232836.jg77wccqwy5fgs6s@development>
<20265.1570664514@sss.pgh.pa.us>
<20191010014813.3b2btmkakhrskwgd@alap3.anarazel.de>
<20191010134303.bkwjzchjb2w66iro@development>
<30706.1570717152@sss.pgh.pa.us>
<20191010163350.2rtcjtje6tkgilqi@development>
<1542.1570738460@sss.pgh.pa.us>
<20191010204022.6hiiubm2cueu3ghk@development>
OK,
here is an updated patch, with the recursive CTE. I've done a fair
amount of testing on it on older versions (up to 9.4), and it seems to
work just fine.
Another thing that I noticed is that the query does not need to look at
RELKIND_COMPOSITE_TYPE, because we only really care about cases when
sql_identifier is stored on-disk. Composite type alone does not do that,
and the CTE includes OIDs of composite types that we then check against
relations and matviews.
Barring objections, I'll push this early next week.
BTW the query (including the RELKIND_COMPSITE_TYPE) was essentially just
a lightly-massaged copy of old_9_6_check_for_unknown_data_type_usage, so
that seems wrong too. The comment explicitly says:
* Also check composite types, in case they are used for table columns.
but even a simple "create type c as (a unknown, b int)" without any
table using it enough to trigger the failure. But maybe it's
intentional, not sure.
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..e0b9a9f574 100644--- a/src/bin/pg_upgrade/version.c+++ b/src/bin/pg_upgrade/version.c@@ -399,3 +399,121 @@ 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);++ /*+ * We need the recursive CTE because the sql_identifier may be wrapped+ * either in a domain or composite type, or both (in arbitrary order).+ */+ res = executeQueryOrDie(conn,+ "WITH RECURSIVE oids AS ( "+ /* the sql_identifier type itself */+ " SELECT 'information_schema.sql_identifier'::regtype AS oid "+ " UNION ALL "+ " SELECT * FROM ( "+ /* domains on the type */+ " WITH x AS (SELECT oid FROM oids) "+ " SELECT t.oid FROM pg_catalog.pg_type t, x WHERE typbasetype = x.oid AND typtype = 'd' "+ " UNION "+ /* composite types containing the type */+ " SELECT t.oid FROM pg_catalog.pg_type t, pg_catalog.pg_class c, pg_catalog.pg_attribute a, x "+ " WHERE t.typtype = 'c' AND "+ " t.oid = c.reltype AND "+ " c.oid = a.attrelid AND "+ " a.atttypid = x.oid "+ " ) foo "+ ") "+ "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 IN (SELECT oid FROM oids) AND "+ " c.relkind IN ("+ CppAsString2(RELKIND_RELATION) ", "+ 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-v2.patch (5.6K, ../20191013001032.lqrn4kqaovhrf77l@development/2-pg-upgrade-sql-identifier-fix-v2.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..e0b9a9f574 100644--- a/src/bin/pg_upgrade/version.c+++ b/src/bin/pg_upgrade/version.c@@ -399,3 +399,121 @@ 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);++ /*+ * We need the recursive CTE because the sql_identifier may be wrapped+ * either in a domain or composite type, or both (in arbitrary order).+ */+ res = executeQueryOrDie(conn,+ "WITH RECURSIVE oids AS ( "+ /* the sql_identifier type itself */+ " SELECT 'information_schema.sql_identifier'::regtype AS oid "+ " UNION ALL "+ " SELECT * FROM ( "+ /* domains on the type */+ " WITH x AS (SELECT oid FROM oids) "+ " SELECT t.oid FROM pg_catalog.pg_type t, x WHERE typbasetype = x.oid AND typtype = 'd' "+ " UNION "+ /* composite types containing the type */+ " SELECT t.oid FROM pg_catalog.pg_type t, pg_catalog.pg_class c, pg_catalog.pg_attribute a, x "+ " WHERE t.typtype = 'c' AND "+ " t.oid = c.reltype AND "+ " c.oid = a.attrelid AND "+ " a.atttypid = x.oid "+ " ) foo "+ ") "+ "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 IN (SELECT oid FROM oids) AND "+ " c.relkind IN ("+ CppAsString2(RELKIND_RELATION) ", "+ 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, tgl@sss.pgh.pa.us, andres@anarazel.de, 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: <20191013001032.lqrn4kqaovhrf77l@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