public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v25 4/4] Avoid some calls to RelationGetRelationName
18+ messages / 8 participants
[nested] [flat]
* [PATCH v25 4/4] Avoid some calls to RelationGetRelationName
@ 2020-02-27 01:22 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Justin Pryzby @ 2020-02-27 01:22 UTC (permalink / raw)
---
src/backend/access/heap/vacuumlazy.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index ed7ff5e1c4..0b6d9a59f3 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -622,8 +622,8 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params,
}
appendStringInfo(&buf, msgfmt,
get_database_name(MyDatabaseId),
- get_namespace_name(RelationGetNamespace(onerel)),
- RelationGetRelationName(onerel),
+ vacrelstats->relnamespace,
+ vacrelstats->relname,
vacrelstats->num_index_scans);
appendStringInfo(&buf, _("pages: %u removed, %u remain, %u skipped due to pins, %u skipped frozen\n"),
vacrelstats->pages_removed,
@@ -795,7 +795,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
if (params->nworkers > 0)
ereport(WARNING,
(errmsg("disabling parallel option of vacuum on \"%s\" --- cannot vacuum temporary tables in parallel",
- RelationGetRelationName(onerel))));
+ vacrelstats->relname)));
}
else
lps = begin_parallel_vacuum(RelationGetRelid(onerel), Irel,
@@ -1718,7 +1718,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
if (vacuumed_pages)
ereport(elevel,
(errmsg("\"%s\": removed %.0f row versions in %u pages",
- RelationGetRelationName(onerel),
+ vacrelstats->relname,
tups_vacuumed, vacuumed_pages)));
/*
@@ -1747,7 +1747,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
ereport(elevel,
(errmsg("\"%s\": found %.0f removable, %.0f nonremovable row versions in %u out of %u pages",
- RelationGetRelationName(onerel),
+ vacrelstats->relname,
tups_vacuumed, num_tuples,
vacrelstats->scanned_pages, nblocks),
errdetail_internal("%s", buf.data)));
@@ -1877,7 +1877,7 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
ereport(elevel,
(errmsg("\"%s\": removed %d row versions in %d pages",
- RelationGetRelationName(onerel),
+ vacrelstats->relname,
tupindex, npages),
errdetail_internal("%s", pg_rusage_show(&ru0))));
}
@@ -2406,7 +2406,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
ereport(elevel,
(errmsg(msg,
- RelationGetRelationName(indrel),
+ vacrelstats->relname,
dead_tuples->num_tuples),
errdetail_internal("%s", pg_rusage_show(&ru0))));
}
@@ -2555,7 +2555,7 @@ lazy_truncate_heap(Relation onerel, LVRelStats *vacrelstats)
vacrelstats->lock_waiter_detected = true;
ereport(elevel,
(errmsg("\"%s\": stopping truncate due to conflicting lock request",
- RelationGetRelationName(onerel))));
+ vacrelstats->relname)));
return;
}
@@ -2624,7 +2624,7 @@ lazy_truncate_heap(Relation onerel, LVRelStats *vacrelstats)
ereport(elevel,
(errmsg("\"%s\": truncated %u to %u pages",
- RelationGetRelationName(onerel),
+ vacrelstats->relname,
old_rel_pages, new_rel_pages),
errdetail_internal("%s",
pg_rusage_show(&ru0))));
@@ -2689,7 +2689,7 @@ count_nondeletable_pages(Relation onerel, LVRelStats *vacrelstats)
{
ereport(elevel,
(errmsg("\"%s\": suspending truncate due to conflicting lock request",
- RelationGetRelationName(onerel))));
+ vacrelstats->relname)));
vacrelstats->lock_waiter_detected = true;
return blkno;
--
2.17.0
--EcLUEBHJhOmOUQ0C--
^ permalink raw reply [nested|flat] 18+ messages in thread
* [PATCH v4 1/1] pg_upgrade: run all data type checks per connection
@ 2023-03-13 13:46 Daniel Gustafsson <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Gustafsson @ 2023-03-13 13:46 UTC (permalink / raw)
The checks for data type usage were each connecting to all databases
in the cluster and running their query. On cluster which have a lot
of databases this can become unnecessarily expensive. This moves the
checks to run in a single connection instead to minimize connection
setup/teardown overhead.
Reviewed-by: Nathan Bossart <[email protected]>
Reviewed-by: Justin Pryzby <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/bin/pg_upgrade/check.c | 575 ++++++++++++++++++++------------
src/bin/pg_upgrade/pg_upgrade.h | 29 +-
src/bin/pg_upgrade/version.c | 289 +++-------------
3 files changed, 433 insertions(+), 460 deletions(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 64024e3b9e..c829aed26e 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -10,6 +10,7 @@
#include "postgres_fe.h"
#include "catalog/pg_authid_d.h"
+#include "catalog/pg_class_d.h"
#include "catalog/pg_collation.h"
#include "fe_utils/string_utils.h"
#include "mb/pg_wchar.h"
@@ -23,14 +24,375 @@ static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
static void check_for_user_defined_postfix_ops(ClusterInfo *cluster);
static void check_for_incompatible_polymorphics(ClusterInfo *cluster);
static void check_for_tables_with_oids(ClusterInfo *cluster);
-static void check_for_composite_data_type_usage(ClusterInfo *cluster);
-static void check_for_reg_data_type_usage(ClusterInfo *cluster);
-static void check_for_aclitem_data_type_usage(ClusterInfo *cluster);
-static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
static void check_for_pg_role_prefix(ClusterInfo *cluster);
static void check_for_new_tablespace_dir(ClusterInfo *new_cluster);
static void check_for_user_defined_encoding_conversions(ClusterInfo *cluster);
+/*
+ * Data type usage checks. Each check for problematic data type usage is
+ * defined in this array with metadata, SQL query for finding the data type
+ * and a function pointer for determining if the check should be executed
+ * for the current version.
+ */
+static int n_data_types_usage_checks = 7;
+static DataTypesUsageChecks data_types_usage_checks[] = {
+ /*
+ * Look for composite types that were made during initdb *or* belong to
+ * information_schema; that's important in case information_schema was
+ * dropped and reloaded.
+ *
+ * The cutoff OID here should match the source cluster's value of
+ * FirstNormalObjectId. We hardcode it rather than using that C #define
+ * because, if that #define is ever changed, our own version's value is
+ * NOT what to use. Eventually we may need a test on the source cluster's
+ * version to select the correct value.
+ */
+ {.status = "Checking for system-defined composite types in user tables",
+ .report_filename = "tables_using_composite.txt",
+ .base_query =
+ "SELECT t.oid FROM pg_catalog.pg_type t "
+ "LEFT JOIN pg_catalog.pg_namespace n ON t.typnamespace = n.oid "
+ " WHERE typtype = 'c' AND (t.oid < 16384 OR nspname = 'information_schema')",
+ .report_text =
+ "Your installation contains system-defined composite type(s) in user tables.\n"
+ "These type OIDs are not stable across PostgreSQL versions,\n"
+ "so this cluster cannot currently be upgraded. You can\n"
+ "drop the problem columns and restart the upgrade.\n"
+ "A list of the problem columns is in the file:",
+ .version_hook = NULL},
+
+ /*
+ * 9.3 -> 9.4
+ * Fully implement the 'line' data type in 9.4, which previously returned
+ * "not enabled" by default and was only functionally enabled with a
+ * compile-time switch; as of 9.4 "line" has a different on-disk
+ * representation format.
+ */
+ {.status = "Checking for incompatible \"line\" data type",
+ .report_filename = "tables_using_line.txt",
+ .base_query =
+ "SELECT 'pg_catalog.line'::pg_catalog.regtype AS oid",
+ .report_text =
+ "your installation contains the \"line\" data type in user tables.\n"
+ "this data type changed its internal and input/output format\n"
+ "between your old and new versions so this\n"
+ "cluster cannot currently be upgraded. you can\n"
+ "drop the problem columns and restart the upgrade.\n"
+ "a list of the problem columns is in the file:",
+ .version_hook = old_9_3_check_for_line_data_type_usage},
+
+ /*
+ * pg_upgrade only preserves these system values:
+ * pg_class.oid
+ * pg_type.oid
+ * pg_enum.oid
+ *
+ * Many of the reg* data types reference system catalog info that is
+ * not preserved, and hence these data types cannot be used in user
+ * tables upgraded by pg_upgrade.
+ */
+ {.status = "Checking for reg* data types in user tables",
+ .report_filename = "tables_using_reg.txt",
+ /*
+ * Note: older servers will not have all of these reg* types, so we have
+ * to write the query like this rather than depending on casts to regtype.
+ */
+ .base_query =
+ "SELECT oid FROM pg_catalog.pg_type t "
+ "WHERE t.typnamespace = "
+ " (SELECT oid FROM pg_catalog.pg_namespace "
+ " WHERE nspname = 'pg_catalog') "
+ " AND t.typname IN ( "
+ /* pg_class.oid is preserved, so 'regclass' is OK */
+ " 'regcollation', "
+ " 'regconfig', "
+ " 'regdictionary', "
+ " 'regnamespace', "
+ " 'regoper', "
+ " 'regoperator', "
+ " 'regproc', "
+ " 'regprocedure' "
+ /* pg_authid.oid is preserved, so 'regrole' is OK */
+ /* pg_type.oid is (mostly) preserved, so 'regtype' is OK */
+ " )",
+ .report_text =
+ "Your installation contains one of the reg* data types in user tables.\n"
+ "These data types reference system OIDs that are not preserved by\n"
+ "pg_upgrade, so this cluster cannot currently be upgraded. You can\n"
+ "drop the problem columns and restart the upgrade.\n"
+ "A list of the problem columns is in the file:",
+ .version_hook = NULL},
+
+ /*
+ * PG 16 increased the size of the 'aclitem' type, which breaks the on-disk
+ * format for existing data.
+ */
+ {.status = "Checking for incompatible aclitem data type in user tables",
+ .report_filename = "tables_using_aclitem.txt",
+ .base_query =
+ "SELECT 'pg_catalog.aclitem'::pg_catalog.regtype AS oid",
+ .report_text =
+ "Your installation contains the \"aclitem\" data type in user tables.\n"
+ "The internal format of \"aclitem\" changed in PostgreSQL version 16\n"
+ "so this cluster cannot currently be upgraded. You can drop the\n"
+ "problem columns and restart the upgrade. A list of the problem\n"
+ "columns is in the file:",
+ .version_hook = check_for_aclitem_data_type_usage},
+
+ /*
+ * It's no longer allowed to create tables or views with "unknown"-type
+ * columns. We do not complain about views with such columns, because
+ * they should get silently converted to "text" columns during the DDL
+ * dump and reload; it seems unlikely to be worth making users do that
+ * by hand. However, if there's a table with such a column, the DDL
+ * reload will fail, so we should pre-detect that rather than failing
+ * mid-upgrade. Worse, if there's a matview with such a column, the
+ * DDL reload will silently change it to "text" which won't match the
+ * on-disk storage (which is like "cstring"). So we *must* reject that.
+ */
+ {.status = "Checking for invalid \"unknown\" user columns",
+ .report_filename = "tables_using_unknown.txt",
+ .base_query =
+ "SELECT 'pg_catalog.unknown'::pg_catalog.regtype AS oid",
+ .report_text =
+ "Your installation contains the \"unknown\" data type in user tables.\n"
+ "This data type is no longer allowed in tables, so this\n"
+ "cluster cannot currently be upgraded. You can\n"
+ "drop the problem columns and restart the upgrade.\n"
+ "A list of the problem columns is in the file:",
+ .version_hook = old_9_6_check_for_unknown_data_type_usage},
+
+ /*
+ * PG 12 changed the 'sql_identifier' type storage to be based on name,
+ * not varchar, which breaks on-disk format for existing data. So we need
+ * to prevent upgrade when used in user objects (tables, indexes, ...).
+ * 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.
+ */
+ {.status = "Checking for invalid \"sql_identifier\" user columns",
+ .report_filename = "tables_using_sql_identifier.txt",
+ .base_query =
+ "SELECT 'information_schema.sql_identifier'::pg_catalog.regtype AS oid",
+ .report_text =
+ "Your installation contains the \"sql_identifier\" data type in user tables.\n"
+ "The on-disk format for this data type has changed, so this\n"
+ "cluster cannot currently be upgraded. You can\n"
+ "drop the problem columns and restart the upgrade.\n"
+ "A list of the problem columns is in the file:",
+ .version_hook = old_11_check_for_sql_identifier_data_type_usage},
+
+ /*
+ * JSONB changed its storage format during 9.4 beta, so check for it.
+ */
+ {.status = "Checking for incompatible \"jsonb\" data type",
+ .report_filename = "tables_using_jsonb.txt",
+ .base_query =
+ "SELECT 'pg_catalog.jsonb'::pg_catalog.regtype AS oid",
+ .report_text =
+ "Your installation contains the \"jsonb\" data type in user tables.\n"
+ "The internal format of \"jsonb\" changed during 9.4 beta so this\n"
+ "cluster cannot currently be upgraded. You can\n"
+ "drop the problem columns and restart the upgrade.\n"
+ "A list of the problem columns is in the file:",
+ .version_hook = check_for_jsonb_9_4_usage},
+};
+
+/*
+ * check_for_data_types_usage()
+ * Detect whether there are any stored columns depending on given type(s)
+ *
+ * If so, write a report to the given file name and signal a failure to the
+ * user.
+ *
+ * The checks to run are defined in a DataTypesUsageChecks structure where
+ * each check has a metadata for explaining errors to the user, a base_query,
+ * a report filename and a function pointer hook for validating if the check
+ * should be executed given the cluster at hand.
+ *
+ * base_query should be a SELECT yielding a single column named "oid",
+ * containing the pg_type OIDs of one or more types that are known to have
+ * inconsistent on-disk representations across server versions.
+ *
+ * We check for the type(s) in tables, matviews, and indexes, but not views;
+ * there's no storage involved in a view.
+ */
+static void
+check_for_data_types_usage(ClusterInfo *cluster, DataTypesUsageChecks *checks)
+{
+ bool found = false;
+ bool *results;
+ PQExpBufferData report;
+
+ prep_status("Checking for data type usage");
+
+ /* Prepare an array to store the results of checks in */
+ results = pg_malloc(sizeof(bool) * n_data_types_usage_checks);
+ memset(results, true, sizeof(*results));
+
+ prep_status_progress("checking all databases");
+
+ /*
+ * Connect to each database in the cluster and run all defined checks
+ * against that database before trying the next one.
+ */
+ for (int dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
+ PGconn *conn = connectToServer(cluster, active_db->db_name);
+
+ for (int checknum = 0; checknum < n_data_types_usage_checks; checknum++)
+ {
+ PGresult *res;
+ int ntups;
+ int i_nspname;
+ int i_relname;
+ int i_attname;
+ FILE *script = NULL;
+ bool db_used = false;
+ char output_path[MAXPGPATH];
+ DataTypesUsageChecks *cur_check = &checks[checknum];
+
+ /*
+ * Make sure that the check applies to the current cluster version
+ * and skip if not. If no check hook has been defined we run the
+ * check for all versions.
+ */
+ if (cur_check->version_hook && !cur_check->version_hook(cluster))
+ {
+ cur_check++;
+ continue;
+ }
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ cur_check->report_filename);
+
+ /*
+ * The type(s) of interest might be wrapped in a domain, array,
+ * composite, or range, and these container types can be nested (to
+ * varying extents depending on server version, but that's not of
+ * concern here). To handle all these cases we need a recursive CTE.
+ */
+ res = executeQueryOrDie(conn,
+ "WITH RECURSIVE oids AS ( "
+ /* start with the type(s) returned by base_query */
+ " %s "
+ " UNION ALL "
+ " SELECT * FROM ( "
+ /* inner WITH because we can only reference the CTE once */
+ " WITH x AS (SELECT oid FROM oids) "
+ /* domains on any type selected so far */
+ " SELECT t.oid FROM pg_catalog.pg_type t, x WHERE typbasetype = x.oid AND typtype = 'd' "
+ " UNION ALL "
+ /* arrays over any type selected so far */
+ " SELECT t.oid FROM pg_catalog.pg_type t, x WHERE typelem = x.oid AND typtype = 'b' "
+ " UNION ALL "
+ /* composite types containing any type selected so far */
+ " 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 "
+ " NOT a.attisdropped AND "
+ " a.atttypid = x.oid "
+ " UNION ALL "
+ /* ranges containing any type selected so far */
+ " SELECT t.oid FROM pg_catalog.pg_type t, pg_catalog.pg_range r, x "
+ " WHERE t.typtype = 'r' AND r.rngtypid = t.oid AND r.rngsubtype = x.oid"
+ " ) foo "
+ ") "
+ /* now look for stored columns of any such type */
+ "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 "
+ /* exclude system catalogs, too */
+ " n.nspname NOT IN ('pg_catalog', 'information_schema')",
+ cur_check->base_query);
+
+ ntups = PQntuples(res);
+
+ /*
+ * The datatype was found, so extract the data and log to the
+ * requested filename. We need to open the file for appending
+ * since the check might have already found the type in another
+ * database earlier in the loop.
+ */
+ if (ntups)
+ {
+ /*
+ * Make sure we have a buffer to save reports to now that we
+ * found a first failing check.
+ */
+ if (!found)
+ initPQExpBuffer(&report);
+ found = true;
+
+ /*
+ * If this is the first time we see an error for the check in
+ * question then print a status message of the failure.
+ */
+ if (results[checknum])
+ {
+ pg_log(PG_REPORT, " failed check: %s", cur_check->status);
+ appendPQExpBuffer(&report, "\n%s\n %s\n",
+ cur_check->report_text, output_path);
+ }
+ results[checknum] = false;
+
+ i_nspname = PQfnumber(res, "nspname");
+ i_relname = PQfnumber(res, "relname");
+ i_attname = PQfnumber(res, "attname");
+
+ for (int rowno = 0; rowno < ntups; rowno++)
+ {
+ found = true;
+ if (script == NULL && (script = fopen_priv(output_path, "a")) == NULL)
+ pg_fatal("could not open file \"%s\": %s",
+ output_path,
+ strerror(errno));
+ if (!db_used)
+ {
+ fprintf(script, "In 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));
+ }
+
+ if (script)
+ {
+ fclose(script);
+ script = NULL;
+ }
+ }
+
+ PQclear(res);
+ cur_check++;
+ }
+
+ PQfinish(conn);
+ }
+
+ if (found)
+ pg_fatal("Data type checks failed: %s", report.data);
+
+ check_ok();
+}
/*
* fix_path_separator
@@ -100,16 +462,9 @@ check_and_dump_old_cluster(bool live_check)
check_is_install_user(&old_cluster);
check_proper_datallowconn(&old_cluster);
check_for_prepared_transactions(&old_cluster);
- check_for_composite_data_type_usage(&old_cluster);
- check_for_reg_data_type_usage(&old_cluster);
check_for_isn_and_int8_passing_mismatch(&old_cluster);
- /*
- * PG 16 increased the size of the 'aclitem' type, which breaks the
- * on-disk format for existing data.
- */
- if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1500)
- check_for_aclitem_data_type_usage(&old_cluster);
+ check_for_data_types_usage(&old_cluster, data_types_usage_checks);
/*
* PG 14 changed the function signature of encoding conversion functions.
@@ -141,21 +496,12 @@ 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 to be based on name,
- * not varchar, which breaks on-disk format for existing data. 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
*/
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 906)
{
- old_9_6_check_for_unknown_data_type_usage(&old_cluster);
if (user_opts.check)
old_9_6_invalidate_hash_indexes(&old_cluster, true);
}
@@ -164,14 +510,6 @@ check_and_dump_old_cluster(bool live_check)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 905)
check_for_pg_role_prefix(&old_cluster);
- if (GET_MAJOR_VERSION(old_cluster.major_version) == 904 &&
- old_cluster.controldata.cat_ver < JSONB_FORMAT_CHANGE_CAT_VER)
- check_for_jsonb_9_4_usage(&old_cluster);
-
- /* Pre-PG 9.4 had a different 'line' data type internal format */
- if (GET_MAJOR_VERSION(old_cluster.major_version) <= 903)
- old_9_3_check_for_line_data_type_usage(&old_cluster);
-
/*
* While not a check option, we do this now because this is the only time
* the old server is running.
@@ -1084,185 +1422,6 @@ check_for_tables_with_oids(ClusterInfo *cluster)
check_ok();
}
-
-/*
- * check_for_composite_data_type_usage()
- * Check for system-defined composite types used in user tables.
- *
- * The OIDs of rowtypes of system catalogs and information_schema views
- * can change across major versions; unlike user-defined types, we have
- * no mechanism for forcing them to be the same in the new cluster.
- * Hence, if any user table uses one, that's problematic for pg_upgrade.
- */
-static void
-check_for_composite_data_type_usage(ClusterInfo *cluster)
-{
- bool found;
- Oid firstUserOid;
- char output_path[MAXPGPATH];
- char *base_query;
-
- prep_status("Checking for system-defined composite types in user tables");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_composite.txt");
-
- /*
- * Look for composite types that were made during initdb *or* belong to
- * information_schema; that's important in case information_schema was
- * dropped and reloaded.
- *
- * The cutoff OID here should match the source cluster's value of
- * FirstNormalObjectId. We hardcode it rather than using that C #define
- * because, if that #define is ever changed, our own version's value is
- * NOT what to use. Eventually we may need a test on the source cluster's
- * version to select the correct value.
- */
- firstUserOid = 16384;
-
- base_query = psprintf("SELECT t.oid FROM pg_catalog.pg_type t "
- "LEFT JOIN pg_catalog.pg_namespace n ON t.typnamespace = n.oid "
- " WHERE typtype = 'c' AND (t.oid < %u OR nspname = 'information_schema')",
- firstUserOid);
-
- found = check_for_data_types_usage(cluster, base_query, output_path);
-
- free(base_query);
-
- if (found)
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains system-defined composite type(s) in user tables.\n"
- "These type OIDs are not stable across PostgreSQL versions,\n"
- "so this cluster cannot currently be upgraded. You can\n"
- "drop the problem columns and restart the upgrade.\n"
- "A list of the problem columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
-}
-
-/*
- * check_for_reg_data_type_usage()
- * pg_upgrade only preserves these system values:
- * pg_class.oid
- * pg_type.oid
- * pg_enum.oid
- *
- * Many of the reg* data types reference system catalog info that is
- * not preserved, and hence these data types cannot be used in user
- * tables upgraded by pg_upgrade.
- */
-static void
-check_for_reg_data_type_usage(ClusterInfo *cluster)
-{
- bool found;
- char output_path[MAXPGPATH];
-
- prep_status("Checking for reg* data types in user tables");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_reg.txt");
-
- /*
- * Note: older servers will not have all of these reg* types, so we have
- * to write the query like this rather than depending on casts to regtype.
- */
- found = check_for_data_types_usage(cluster,
- "SELECT oid FROM pg_catalog.pg_type t "
- "WHERE t.typnamespace = "
- " (SELECT oid FROM pg_catalog.pg_namespace "
- " WHERE nspname = 'pg_catalog') "
- " AND t.typname IN ( "
- /* pg_class.oid is preserved, so 'regclass' is OK */
- " 'regcollation', "
- " 'regconfig', "
- " 'regdictionary', "
- " 'regnamespace', "
- " 'regoper', "
- " 'regoperator', "
- " 'regproc', "
- " 'regprocedure' "
- /* pg_authid.oid is preserved, so 'regrole' is OK */
- /* pg_type.oid is (mostly) preserved, so 'regtype' is OK */
- " )",
- output_path);
-
- if (found)
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains one of the reg* data types in user tables.\n"
- "These data types reference system OIDs that are not preserved by\n"
- "pg_upgrade, so this cluster cannot currently be upgraded. You can\n"
- "drop the problem columns and restart the upgrade.\n"
- "A list of the problem columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
-}
-
-/*
- * check_for_aclitem_data_type_usage
- *
- * aclitem changed its storage format in 16, so check for it.
- */
-static void
-check_for_aclitem_data_type_usage(ClusterInfo *cluster)
-{
- char output_path[MAXPGPATH];
-
- prep_status("Checking for incompatible \"aclitem\" data type in user tables");
-
- snprintf(output_path, sizeof(output_path), "tables_using_aclitem.txt");
-
- if (check_for_data_type_usage(cluster, "pg_catalog.aclitem", output_path))
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains the \"aclitem\" data type in user tables.\n"
- "The internal format of \"aclitem\" changed in PostgreSQL version 16\n"
- "so this cluster cannot currently be upgraded. You can drop the\n"
- "problem columns and restart the upgrade. A list of the problem\n"
- "columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
-}
-
-/*
- * check_for_jsonb_9_4_usage()
- *
- * JSONB changed its storage format during 9.4 beta, so check for it.
- */
-static void
-check_for_jsonb_9_4_usage(ClusterInfo *cluster)
-{
- char output_path[MAXPGPATH];
-
- prep_status("Checking for incompatible \"jsonb\" data type");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_jsonb.txt");
-
- if (check_for_data_type_usage(cluster, "pg_catalog.jsonb", output_path))
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains the \"jsonb\" data type in user tables.\n"
- "The internal format of \"jsonb\" changed during 9.4 beta so this\n"
- "cluster cannot currently be upgraded. You can\n"
- "drop the problem columns and restart the upgrade.\n"
- "A list of the problem columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
-}
-
/*
* check_for_pg_role_prefix()
*
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 3eea0139c7..208bfbb68e 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -328,6 +328,21 @@ typedef struct
} OSInfo;
+/* Function signature for data type check version hook */
+typedef bool (*DataTypesUsageVersionCheck)(ClusterInfo *cluster);
+
+/*
+ * DataTypesUsageChecks
+ */
+typedef struct
+{
+ const char *status; /* status line to print to the user */
+ const char *report_filename; /* filename to store report to */
+ const char *base_query; /* Query to extract the oid of the datatype */
+ const char *report_text; /* Text to store to report in case of error */
+ DataTypesUsageVersionCheck version_hook;
+} DataTypesUsageChecks;
+
/*
* Global variables
*/
@@ -450,19 +465,15 @@ unsigned int str2uint(const char *str);
/* version.c */
-bool check_for_data_types_usage(ClusterInfo *cluster,
- const char *base_query,
- const char *output_path);
-bool check_for_data_type_usage(ClusterInfo *cluster,
- const char *type_name,
- const char *output_path);
-void old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster);
-void old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster);
+bool old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster);
+bool check_for_jsonb_9_4_usage(ClusterInfo *cluster);
+bool old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster);
+bool old_11_check_for_sql_identifier_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);
void report_extension_updates(ClusterInfo *cluster);
+bool check_for_aclitem_data_type_usage(ClusterInfo *cluster);
/* parallel.c */
void parallel_exec_prog(const char *log_file, const char *opt_log_file,
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index 403a6d7cfa..828a975ac0 100644
--- a/src/bin/pg_upgrade/version.c
+++ b/src/bin/pg_upgrade/version.c
@@ -9,236 +9,41 @@
#include "postgres_fe.h"
-#include "catalog/pg_class_d.h"
#include "fe_utils/string_utils.h"
#include "pg_upgrade.h"
-
-/*
- * check_for_data_types_usage()
- * Detect whether there are any stored columns depending on given type(s)
- *
- * If so, write a report to the given file name, and return true.
- *
- * base_query should be a SELECT yielding a single column named "oid",
- * containing the pg_type OIDs of one or more types that are known to have
- * inconsistent on-disk representations across server versions.
- *
- * We check for the type(s) in tables, matviews, and indexes, but not views;
- * there's no storage involved in a view.
- */
bool
-check_for_data_types_usage(ClusterInfo *cluster,
- const char *base_query,
- const char *output_path)
+old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)
{
- bool found = false;
- FILE *script = NULL;
- int dbnum;
-
- for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
- {
- DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
- PGconn *conn = connectToServer(cluster, active_db->db_name);
- PQExpBufferData querybuf;
- PGresult *res;
- bool db_used = false;
- int ntups;
- int rowno;
- int i_nspname,
- i_relname,
- i_attname;
-
- /*
- * The type(s) of interest might be wrapped in a domain, array,
- * composite, or range, and these container types can be nested (to
- * varying extents depending on server version, but that's not of
- * concern here). To handle all these cases we need a recursive CTE.
- */
- initPQExpBuffer(&querybuf);
- appendPQExpBuffer(&querybuf,
- "WITH RECURSIVE oids AS ( "
- /* start with the type(s) returned by base_query */
- " %s "
- " UNION ALL "
- " SELECT * FROM ( "
- /* inner WITH because we can only reference the CTE once */
- " WITH x AS (SELECT oid FROM oids) "
- /* domains on any type selected so far */
- " SELECT t.oid FROM pg_catalog.pg_type t, x WHERE typbasetype = x.oid AND typtype = 'd' "
- " UNION ALL "
- /* arrays over any type selected so far */
- " SELECT t.oid FROM pg_catalog.pg_type t, x WHERE typelem = x.oid AND typtype = 'b' "
- " UNION ALL "
- /* composite types containing any type selected so far */
- " 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 "
- " NOT a.attisdropped AND "
- " a.atttypid = x.oid "
- " UNION ALL "
- /* ranges containing any type selected so far */
- " SELECT t.oid FROM pg_catalog.pg_type t, pg_catalog.pg_range r, x "
- " WHERE t.typtype = 'r' AND r.rngtypid = t.oid AND r.rngsubtype = x.oid"
- " ) foo "
- ") "
- /* now look for stored columns of any such type */
- "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 "
- /* exclude system catalogs, too */
- " n.nspname NOT IN ('pg_catalog', 'information_schema')",
- base_query);
-
- res = executeQueryOrDie(conn, "%s", querybuf.data);
-
- 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", output_path,
- strerror(errno));
- if (!db_used)
- {
- fprintf(script, "In 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);
-
- termPQExpBuffer(&querybuf);
-
- PQfinish(conn);
- }
-
- if (script)
- fclose(script);
+ /* Pre-PG 9.4 had a different 'line' data type internal format */
+ if (GET_MAJOR_VERSION(cluster->major_version) <= 903)
+ return true;
- return found;
+ return false;
}
/*
- * check_for_data_type_usage()
- * Detect whether there are any stored columns depending on the given type
- *
- * If so, write a report to the given file name, and return true.
+ * check_for_jsonb_9_4_usage()
*
- * type_name should be a fully qualified type name. This is just a
- * trivial wrapper around check_for_data_types_usage() to convert a
- * type name into a base query.
+ * JSONB changed its storage format during 9.4 beta, so check for it.
*/
bool
-check_for_data_type_usage(ClusterInfo *cluster,
- const char *type_name,
- const char *output_path)
-{
- bool found;
- char *base_query;
-
- base_query = psprintf("SELECT '%s'::pg_catalog.regtype AS oid",
- type_name);
-
- found = check_for_data_types_usage(cluster, base_query, output_path);
-
- free(base_query);
-
- return found;
-}
-
-
-/*
- * old_9_3_check_for_line_data_type_usage()
- * 9.3 -> 9.4
- * Fully implement the 'line' data type in 9.4, which previously returned
- * "not enabled" by default and was only functionally enabled with a
- * compile-time switch; as of 9.4 "line" has a different on-disk
- * representation format.
- */
-void
-old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)
+check_for_jsonb_9_4_usage(ClusterInfo *cluster)
{
- char output_path[MAXPGPATH];
+ if (GET_MAJOR_VERSION(cluster->major_version) == 904 &&
+ cluster->controldata.cat_ver < JSONB_FORMAT_CHANGE_CAT_VER)
+ return true;
- prep_status("Checking for incompatible \"line\" data type");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_line.txt");
-
- if (check_for_data_type_usage(cluster, "pg_catalog.line", output_path))
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains the \"line\" data type in user tables.\n"
- "This data type changed its internal and input/output format\n"
- "between your old and new versions so this\n"
- "cluster cannot currently be upgraded. You can\n"
- "drop the problem columns and restart the upgrade.\n"
- "A list of the problem columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
+ return false;
}
-
-/*
- * old_9_6_check_for_unknown_data_type_usage()
- * 9.6 -> 10
- * It's no longer allowed to create tables or views with "unknown"-type
- * columns. We do not complain about views with such columns, because
- * they should get silently converted to "text" columns during the DDL
- * dump and reload; it seems unlikely to be worth making users do that
- * by hand. However, if there's a table with such a column, the DDL
- * reload will fail, so we should pre-detect that rather than failing
- * mid-upgrade. Worse, if there's a matview with such a column, the
- * DDL reload will silently change it to "text" which won't match the
- * on-disk storage (which is like "cstring"). So we *must* reject that.
- */
-void
+bool
old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster)
{
- char output_path[MAXPGPATH];
-
- prep_status("Checking for invalid \"unknown\" user columns");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_unknown.txt");
-
- if (check_for_data_type_usage(cluster, "pg_catalog.unknown", output_path))
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains the \"unknown\" data type in user tables.\n"
- "This data type is no longer allowed in tables, so this\n"
- "cluster cannot currently be upgraded. You can\n"
- "drop the problem columns and restart the upgrade.\n"
- "A list of the problem columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
+ /* Pre-PG 10 allowed tables with 'unknown' type columns */
+ if (GET_MAJOR_VERSION(cluster->major_version) <= 906)
+ return true;
+ return false;
}
/*
@@ -353,41 +158,20 @@ old_9_6_invalidate_hash_indexes(ClusterInfo *cluster, bool check_mode)
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.
- */
-void
+bool
old_11_check_for_sql_identifier_data_type_usage(ClusterInfo *cluster)
{
- char output_path[MAXPGPATH];
-
- prep_status("Checking for invalid \"sql_identifier\" user columns");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_sql_identifier.txt");
-
- if (check_for_data_type_usage(cluster, "information_schema.sql_identifier",
- output_path))
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains the \"sql_identifier\" data type in user tables.\n"
- "The on-disk format for this data type has changed, so this\n"
- "cluster cannot currently be upgraded. You can\n"
- "drop the problem columns and restart the upgrade.\n"
- "A list of the problem columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
+ /*
+ * PG 12 changed the 'sql_identifier' type storage to be based on name,
+ * not varchar, which breaks on-disk format for existing data. So we need
+ * to prevent upgrade when used in user objects (tables, indexes, ...).
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1100)
+ return true;
+
+ return false;
}
-
/*
* report_extension_updates()
* Report extensions that should be updated.
@@ -459,3 +243,22 @@ report_extension_updates(ClusterInfo *cluster)
else
check_ok();
}
+
+/*
+ * check_for_aclitem_data_type_usage
+ *
+ * aclitem changed its storage format in 16, so check for it.
+ */
+bool
+check_for_aclitem_data_type_usage(ClusterInfo *cluster)
+{
+ /*
+ * PG 16 increased the size of the 'aclitem' type, which breaks the on-disk
+ * format for existing data.
+ */
+ if (GET_MAJOR_VERSION(cluster->major_version) <= 1500)
+ return true;
+
+ return false;
+}
+
--
2.25.1
--tKW2IUtsqtDRztdT--
^ permalink raw reply [nested|flat] 18+ messages in thread
* [PATCH v4 1/1] pg_upgrade: run all data type checks per connection
@ 2023-03-13 13:46 Daniel Gustafsson <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Gustafsson @ 2023-03-13 13:46 UTC (permalink / raw)
The checks for data type usage were each connecting to all databases
in the cluster and running their query. On cluster which have a lot
of databases this can become unnecessarily expensive. This moves the
checks to run in a single connection instead to minimize connection
setup/teardown overhead.
Reviewed-by: Nathan Bossart <[email protected]>
Reviewed-by: Justin Pryzby <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/bin/pg_upgrade/check.c | 575 ++++++++++++++++++++------------
src/bin/pg_upgrade/pg_upgrade.h | 29 +-
src/bin/pg_upgrade/version.c | 289 +++-------------
3 files changed, 433 insertions(+), 460 deletions(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 64024e3b9e..c829aed26e 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -10,6 +10,7 @@
#include "postgres_fe.h"
#include "catalog/pg_authid_d.h"
+#include "catalog/pg_class_d.h"
#include "catalog/pg_collation.h"
#include "fe_utils/string_utils.h"
#include "mb/pg_wchar.h"
@@ -23,14 +24,375 @@ static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
static void check_for_user_defined_postfix_ops(ClusterInfo *cluster);
static void check_for_incompatible_polymorphics(ClusterInfo *cluster);
static void check_for_tables_with_oids(ClusterInfo *cluster);
-static void check_for_composite_data_type_usage(ClusterInfo *cluster);
-static void check_for_reg_data_type_usage(ClusterInfo *cluster);
-static void check_for_aclitem_data_type_usage(ClusterInfo *cluster);
-static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
static void check_for_pg_role_prefix(ClusterInfo *cluster);
static void check_for_new_tablespace_dir(ClusterInfo *new_cluster);
static void check_for_user_defined_encoding_conversions(ClusterInfo *cluster);
+/*
+ * Data type usage checks. Each check for problematic data type usage is
+ * defined in this array with metadata, SQL query for finding the data type
+ * and a function pointer for determining if the check should be executed
+ * for the current version.
+ */
+static int n_data_types_usage_checks = 7;
+static DataTypesUsageChecks data_types_usage_checks[] = {
+ /*
+ * Look for composite types that were made during initdb *or* belong to
+ * information_schema; that's important in case information_schema was
+ * dropped and reloaded.
+ *
+ * The cutoff OID here should match the source cluster's value of
+ * FirstNormalObjectId. We hardcode it rather than using that C #define
+ * because, if that #define is ever changed, our own version's value is
+ * NOT what to use. Eventually we may need a test on the source cluster's
+ * version to select the correct value.
+ */
+ {.status = "Checking for system-defined composite types in user tables",
+ .report_filename = "tables_using_composite.txt",
+ .base_query =
+ "SELECT t.oid FROM pg_catalog.pg_type t "
+ "LEFT JOIN pg_catalog.pg_namespace n ON t.typnamespace = n.oid "
+ " WHERE typtype = 'c' AND (t.oid < 16384 OR nspname = 'information_schema')",
+ .report_text =
+ "Your installation contains system-defined composite type(s) in user tables.\n"
+ "These type OIDs are not stable across PostgreSQL versions,\n"
+ "so this cluster cannot currently be upgraded. You can\n"
+ "drop the problem columns and restart the upgrade.\n"
+ "A list of the problem columns is in the file:",
+ .version_hook = NULL},
+
+ /*
+ * 9.3 -> 9.4
+ * Fully implement the 'line' data type in 9.4, which previously returned
+ * "not enabled" by default and was only functionally enabled with a
+ * compile-time switch; as of 9.4 "line" has a different on-disk
+ * representation format.
+ */
+ {.status = "Checking for incompatible \"line\" data type",
+ .report_filename = "tables_using_line.txt",
+ .base_query =
+ "SELECT 'pg_catalog.line'::pg_catalog.regtype AS oid",
+ .report_text =
+ "your installation contains the \"line\" data type in user tables.\n"
+ "this data type changed its internal and input/output format\n"
+ "between your old and new versions so this\n"
+ "cluster cannot currently be upgraded. you can\n"
+ "drop the problem columns and restart the upgrade.\n"
+ "a list of the problem columns is in the file:",
+ .version_hook = old_9_3_check_for_line_data_type_usage},
+
+ /*
+ * pg_upgrade only preserves these system values:
+ * pg_class.oid
+ * pg_type.oid
+ * pg_enum.oid
+ *
+ * Many of the reg* data types reference system catalog info that is
+ * not preserved, and hence these data types cannot be used in user
+ * tables upgraded by pg_upgrade.
+ */
+ {.status = "Checking for reg* data types in user tables",
+ .report_filename = "tables_using_reg.txt",
+ /*
+ * Note: older servers will not have all of these reg* types, so we have
+ * to write the query like this rather than depending on casts to regtype.
+ */
+ .base_query =
+ "SELECT oid FROM pg_catalog.pg_type t "
+ "WHERE t.typnamespace = "
+ " (SELECT oid FROM pg_catalog.pg_namespace "
+ " WHERE nspname = 'pg_catalog') "
+ " AND t.typname IN ( "
+ /* pg_class.oid is preserved, so 'regclass' is OK */
+ " 'regcollation', "
+ " 'regconfig', "
+ " 'regdictionary', "
+ " 'regnamespace', "
+ " 'regoper', "
+ " 'regoperator', "
+ " 'regproc', "
+ " 'regprocedure' "
+ /* pg_authid.oid is preserved, so 'regrole' is OK */
+ /* pg_type.oid is (mostly) preserved, so 'regtype' is OK */
+ " )",
+ .report_text =
+ "Your installation contains one of the reg* data types in user tables.\n"
+ "These data types reference system OIDs that are not preserved by\n"
+ "pg_upgrade, so this cluster cannot currently be upgraded. You can\n"
+ "drop the problem columns and restart the upgrade.\n"
+ "A list of the problem columns is in the file:",
+ .version_hook = NULL},
+
+ /*
+ * PG 16 increased the size of the 'aclitem' type, which breaks the on-disk
+ * format for existing data.
+ */
+ {.status = "Checking for incompatible aclitem data type in user tables",
+ .report_filename = "tables_using_aclitem.txt",
+ .base_query =
+ "SELECT 'pg_catalog.aclitem'::pg_catalog.regtype AS oid",
+ .report_text =
+ "Your installation contains the \"aclitem\" data type in user tables.\n"
+ "The internal format of \"aclitem\" changed in PostgreSQL version 16\n"
+ "so this cluster cannot currently be upgraded. You can drop the\n"
+ "problem columns and restart the upgrade. A list of the problem\n"
+ "columns is in the file:",
+ .version_hook = check_for_aclitem_data_type_usage},
+
+ /*
+ * It's no longer allowed to create tables or views with "unknown"-type
+ * columns. We do not complain about views with such columns, because
+ * they should get silently converted to "text" columns during the DDL
+ * dump and reload; it seems unlikely to be worth making users do that
+ * by hand. However, if there's a table with such a column, the DDL
+ * reload will fail, so we should pre-detect that rather than failing
+ * mid-upgrade. Worse, if there's a matview with such a column, the
+ * DDL reload will silently change it to "text" which won't match the
+ * on-disk storage (which is like "cstring"). So we *must* reject that.
+ */
+ {.status = "Checking for invalid \"unknown\" user columns",
+ .report_filename = "tables_using_unknown.txt",
+ .base_query =
+ "SELECT 'pg_catalog.unknown'::pg_catalog.regtype AS oid",
+ .report_text =
+ "Your installation contains the \"unknown\" data type in user tables.\n"
+ "This data type is no longer allowed in tables, so this\n"
+ "cluster cannot currently be upgraded. You can\n"
+ "drop the problem columns and restart the upgrade.\n"
+ "A list of the problem columns is in the file:",
+ .version_hook = old_9_6_check_for_unknown_data_type_usage},
+
+ /*
+ * PG 12 changed the 'sql_identifier' type storage to be based on name,
+ * not varchar, which breaks on-disk format for existing data. So we need
+ * to prevent upgrade when used in user objects (tables, indexes, ...).
+ * 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.
+ */
+ {.status = "Checking for invalid \"sql_identifier\" user columns",
+ .report_filename = "tables_using_sql_identifier.txt",
+ .base_query =
+ "SELECT 'information_schema.sql_identifier'::pg_catalog.regtype AS oid",
+ .report_text =
+ "Your installation contains the \"sql_identifier\" data type in user tables.\n"
+ "The on-disk format for this data type has changed, so this\n"
+ "cluster cannot currently be upgraded. You can\n"
+ "drop the problem columns and restart the upgrade.\n"
+ "A list of the problem columns is in the file:",
+ .version_hook = old_11_check_for_sql_identifier_data_type_usage},
+
+ /*
+ * JSONB changed its storage format during 9.4 beta, so check for it.
+ */
+ {.status = "Checking for incompatible \"jsonb\" data type",
+ .report_filename = "tables_using_jsonb.txt",
+ .base_query =
+ "SELECT 'pg_catalog.jsonb'::pg_catalog.regtype AS oid",
+ .report_text =
+ "Your installation contains the \"jsonb\" data type in user tables.\n"
+ "The internal format of \"jsonb\" changed during 9.4 beta so this\n"
+ "cluster cannot currently be upgraded. You can\n"
+ "drop the problem columns and restart the upgrade.\n"
+ "A list of the problem columns is in the file:",
+ .version_hook = check_for_jsonb_9_4_usage},
+};
+
+/*
+ * check_for_data_types_usage()
+ * Detect whether there are any stored columns depending on given type(s)
+ *
+ * If so, write a report to the given file name and signal a failure to the
+ * user.
+ *
+ * The checks to run are defined in a DataTypesUsageChecks structure where
+ * each check has a metadata for explaining errors to the user, a base_query,
+ * a report filename and a function pointer hook for validating if the check
+ * should be executed given the cluster at hand.
+ *
+ * base_query should be a SELECT yielding a single column named "oid",
+ * containing the pg_type OIDs of one or more types that are known to have
+ * inconsistent on-disk representations across server versions.
+ *
+ * We check for the type(s) in tables, matviews, and indexes, but not views;
+ * there's no storage involved in a view.
+ */
+static void
+check_for_data_types_usage(ClusterInfo *cluster, DataTypesUsageChecks *checks)
+{
+ bool found = false;
+ bool *results;
+ PQExpBufferData report;
+
+ prep_status("Checking for data type usage");
+
+ /* Prepare an array to store the results of checks in */
+ results = pg_malloc(sizeof(bool) * n_data_types_usage_checks);
+ memset(results, true, sizeof(*results));
+
+ prep_status_progress("checking all databases");
+
+ /*
+ * Connect to each database in the cluster and run all defined checks
+ * against that database before trying the next one.
+ */
+ for (int dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
+ PGconn *conn = connectToServer(cluster, active_db->db_name);
+
+ for (int checknum = 0; checknum < n_data_types_usage_checks; checknum++)
+ {
+ PGresult *res;
+ int ntups;
+ int i_nspname;
+ int i_relname;
+ int i_attname;
+ FILE *script = NULL;
+ bool db_used = false;
+ char output_path[MAXPGPATH];
+ DataTypesUsageChecks *cur_check = &checks[checknum];
+
+ /*
+ * Make sure that the check applies to the current cluster version
+ * and skip if not. If no check hook has been defined we run the
+ * check for all versions.
+ */
+ if (cur_check->version_hook && !cur_check->version_hook(cluster))
+ {
+ cur_check++;
+ continue;
+ }
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ cur_check->report_filename);
+
+ /*
+ * The type(s) of interest might be wrapped in a domain, array,
+ * composite, or range, and these container types can be nested (to
+ * varying extents depending on server version, but that's not of
+ * concern here). To handle all these cases we need a recursive CTE.
+ */
+ res = executeQueryOrDie(conn,
+ "WITH RECURSIVE oids AS ( "
+ /* start with the type(s) returned by base_query */
+ " %s "
+ " UNION ALL "
+ " SELECT * FROM ( "
+ /* inner WITH because we can only reference the CTE once */
+ " WITH x AS (SELECT oid FROM oids) "
+ /* domains on any type selected so far */
+ " SELECT t.oid FROM pg_catalog.pg_type t, x WHERE typbasetype = x.oid AND typtype = 'd' "
+ " UNION ALL "
+ /* arrays over any type selected so far */
+ " SELECT t.oid FROM pg_catalog.pg_type t, x WHERE typelem = x.oid AND typtype = 'b' "
+ " UNION ALL "
+ /* composite types containing any type selected so far */
+ " 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 "
+ " NOT a.attisdropped AND "
+ " a.atttypid = x.oid "
+ " UNION ALL "
+ /* ranges containing any type selected so far */
+ " SELECT t.oid FROM pg_catalog.pg_type t, pg_catalog.pg_range r, x "
+ " WHERE t.typtype = 'r' AND r.rngtypid = t.oid AND r.rngsubtype = x.oid"
+ " ) foo "
+ ") "
+ /* now look for stored columns of any such type */
+ "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 "
+ /* exclude system catalogs, too */
+ " n.nspname NOT IN ('pg_catalog', 'information_schema')",
+ cur_check->base_query);
+
+ ntups = PQntuples(res);
+
+ /*
+ * The datatype was found, so extract the data and log to the
+ * requested filename. We need to open the file for appending
+ * since the check might have already found the type in another
+ * database earlier in the loop.
+ */
+ if (ntups)
+ {
+ /*
+ * Make sure we have a buffer to save reports to now that we
+ * found a first failing check.
+ */
+ if (!found)
+ initPQExpBuffer(&report);
+ found = true;
+
+ /*
+ * If this is the first time we see an error for the check in
+ * question then print a status message of the failure.
+ */
+ if (results[checknum])
+ {
+ pg_log(PG_REPORT, " failed check: %s", cur_check->status);
+ appendPQExpBuffer(&report, "\n%s\n %s\n",
+ cur_check->report_text, output_path);
+ }
+ results[checknum] = false;
+
+ i_nspname = PQfnumber(res, "nspname");
+ i_relname = PQfnumber(res, "relname");
+ i_attname = PQfnumber(res, "attname");
+
+ for (int rowno = 0; rowno < ntups; rowno++)
+ {
+ found = true;
+ if (script == NULL && (script = fopen_priv(output_path, "a")) == NULL)
+ pg_fatal("could not open file \"%s\": %s",
+ output_path,
+ strerror(errno));
+ if (!db_used)
+ {
+ fprintf(script, "In 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));
+ }
+
+ if (script)
+ {
+ fclose(script);
+ script = NULL;
+ }
+ }
+
+ PQclear(res);
+ cur_check++;
+ }
+
+ PQfinish(conn);
+ }
+
+ if (found)
+ pg_fatal("Data type checks failed: %s", report.data);
+
+ check_ok();
+}
/*
* fix_path_separator
@@ -100,16 +462,9 @@ check_and_dump_old_cluster(bool live_check)
check_is_install_user(&old_cluster);
check_proper_datallowconn(&old_cluster);
check_for_prepared_transactions(&old_cluster);
- check_for_composite_data_type_usage(&old_cluster);
- check_for_reg_data_type_usage(&old_cluster);
check_for_isn_and_int8_passing_mismatch(&old_cluster);
- /*
- * PG 16 increased the size of the 'aclitem' type, which breaks the
- * on-disk format for existing data.
- */
- if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1500)
- check_for_aclitem_data_type_usage(&old_cluster);
+ check_for_data_types_usage(&old_cluster, data_types_usage_checks);
/*
* PG 14 changed the function signature of encoding conversion functions.
@@ -141,21 +496,12 @@ 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 to be based on name,
- * not varchar, which breaks on-disk format for existing data. 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
*/
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 906)
{
- old_9_6_check_for_unknown_data_type_usage(&old_cluster);
if (user_opts.check)
old_9_6_invalidate_hash_indexes(&old_cluster, true);
}
@@ -164,14 +510,6 @@ check_and_dump_old_cluster(bool live_check)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 905)
check_for_pg_role_prefix(&old_cluster);
- if (GET_MAJOR_VERSION(old_cluster.major_version) == 904 &&
- old_cluster.controldata.cat_ver < JSONB_FORMAT_CHANGE_CAT_VER)
- check_for_jsonb_9_4_usage(&old_cluster);
-
- /* Pre-PG 9.4 had a different 'line' data type internal format */
- if (GET_MAJOR_VERSION(old_cluster.major_version) <= 903)
- old_9_3_check_for_line_data_type_usage(&old_cluster);
-
/*
* While not a check option, we do this now because this is the only time
* the old server is running.
@@ -1084,185 +1422,6 @@ check_for_tables_with_oids(ClusterInfo *cluster)
check_ok();
}
-
-/*
- * check_for_composite_data_type_usage()
- * Check for system-defined composite types used in user tables.
- *
- * The OIDs of rowtypes of system catalogs and information_schema views
- * can change across major versions; unlike user-defined types, we have
- * no mechanism for forcing them to be the same in the new cluster.
- * Hence, if any user table uses one, that's problematic for pg_upgrade.
- */
-static void
-check_for_composite_data_type_usage(ClusterInfo *cluster)
-{
- bool found;
- Oid firstUserOid;
- char output_path[MAXPGPATH];
- char *base_query;
-
- prep_status("Checking for system-defined composite types in user tables");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_composite.txt");
-
- /*
- * Look for composite types that were made during initdb *or* belong to
- * information_schema; that's important in case information_schema was
- * dropped and reloaded.
- *
- * The cutoff OID here should match the source cluster's value of
- * FirstNormalObjectId. We hardcode it rather than using that C #define
- * because, if that #define is ever changed, our own version's value is
- * NOT what to use. Eventually we may need a test on the source cluster's
- * version to select the correct value.
- */
- firstUserOid = 16384;
-
- base_query = psprintf("SELECT t.oid FROM pg_catalog.pg_type t "
- "LEFT JOIN pg_catalog.pg_namespace n ON t.typnamespace = n.oid "
- " WHERE typtype = 'c' AND (t.oid < %u OR nspname = 'information_schema')",
- firstUserOid);
-
- found = check_for_data_types_usage(cluster, base_query, output_path);
-
- free(base_query);
-
- if (found)
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains system-defined composite type(s) in user tables.\n"
- "These type OIDs are not stable across PostgreSQL versions,\n"
- "so this cluster cannot currently be upgraded. You can\n"
- "drop the problem columns and restart the upgrade.\n"
- "A list of the problem columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
-}
-
-/*
- * check_for_reg_data_type_usage()
- * pg_upgrade only preserves these system values:
- * pg_class.oid
- * pg_type.oid
- * pg_enum.oid
- *
- * Many of the reg* data types reference system catalog info that is
- * not preserved, and hence these data types cannot be used in user
- * tables upgraded by pg_upgrade.
- */
-static void
-check_for_reg_data_type_usage(ClusterInfo *cluster)
-{
- bool found;
- char output_path[MAXPGPATH];
-
- prep_status("Checking for reg* data types in user tables");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_reg.txt");
-
- /*
- * Note: older servers will not have all of these reg* types, so we have
- * to write the query like this rather than depending on casts to regtype.
- */
- found = check_for_data_types_usage(cluster,
- "SELECT oid FROM pg_catalog.pg_type t "
- "WHERE t.typnamespace = "
- " (SELECT oid FROM pg_catalog.pg_namespace "
- " WHERE nspname = 'pg_catalog') "
- " AND t.typname IN ( "
- /* pg_class.oid is preserved, so 'regclass' is OK */
- " 'regcollation', "
- " 'regconfig', "
- " 'regdictionary', "
- " 'regnamespace', "
- " 'regoper', "
- " 'regoperator', "
- " 'regproc', "
- " 'regprocedure' "
- /* pg_authid.oid is preserved, so 'regrole' is OK */
- /* pg_type.oid is (mostly) preserved, so 'regtype' is OK */
- " )",
- output_path);
-
- if (found)
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains one of the reg* data types in user tables.\n"
- "These data types reference system OIDs that are not preserved by\n"
- "pg_upgrade, so this cluster cannot currently be upgraded. You can\n"
- "drop the problem columns and restart the upgrade.\n"
- "A list of the problem columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
-}
-
-/*
- * check_for_aclitem_data_type_usage
- *
- * aclitem changed its storage format in 16, so check for it.
- */
-static void
-check_for_aclitem_data_type_usage(ClusterInfo *cluster)
-{
- char output_path[MAXPGPATH];
-
- prep_status("Checking for incompatible \"aclitem\" data type in user tables");
-
- snprintf(output_path, sizeof(output_path), "tables_using_aclitem.txt");
-
- if (check_for_data_type_usage(cluster, "pg_catalog.aclitem", output_path))
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains the \"aclitem\" data type in user tables.\n"
- "The internal format of \"aclitem\" changed in PostgreSQL version 16\n"
- "so this cluster cannot currently be upgraded. You can drop the\n"
- "problem columns and restart the upgrade. A list of the problem\n"
- "columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
-}
-
-/*
- * check_for_jsonb_9_4_usage()
- *
- * JSONB changed its storage format during 9.4 beta, so check for it.
- */
-static void
-check_for_jsonb_9_4_usage(ClusterInfo *cluster)
-{
- char output_path[MAXPGPATH];
-
- prep_status("Checking for incompatible \"jsonb\" data type");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_jsonb.txt");
-
- if (check_for_data_type_usage(cluster, "pg_catalog.jsonb", output_path))
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains the \"jsonb\" data type in user tables.\n"
- "The internal format of \"jsonb\" changed during 9.4 beta so this\n"
- "cluster cannot currently be upgraded. You can\n"
- "drop the problem columns and restart the upgrade.\n"
- "A list of the problem columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
-}
-
/*
* check_for_pg_role_prefix()
*
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 3eea0139c7..208bfbb68e 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -328,6 +328,21 @@ typedef struct
} OSInfo;
+/* Function signature for data type check version hook */
+typedef bool (*DataTypesUsageVersionCheck)(ClusterInfo *cluster);
+
+/*
+ * DataTypesUsageChecks
+ */
+typedef struct
+{
+ const char *status; /* status line to print to the user */
+ const char *report_filename; /* filename to store report to */
+ const char *base_query; /* Query to extract the oid of the datatype */
+ const char *report_text; /* Text to store to report in case of error */
+ DataTypesUsageVersionCheck version_hook;
+} DataTypesUsageChecks;
+
/*
* Global variables
*/
@@ -450,19 +465,15 @@ unsigned int str2uint(const char *str);
/* version.c */
-bool check_for_data_types_usage(ClusterInfo *cluster,
- const char *base_query,
- const char *output_path);
-bool check_for_data_type_usage(ClusterInfo *cluster,
- const char *type_name,
- const char *output_path);
-void old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster);
-void old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster);
+bool old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster);
+bool check_for_jsonb_9_4_usage(ClusterInfo *cluster);
+bool old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster);
+bool old_11_check_for_sql_identifier_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);
void report_extension_updates(ClusterInfo *cluster);
+bool check_for_aclitem_data_type_usage(ClusterInfo *cluster);
/* parallel.c */
void parallel_exec_prog(const char *log_file, const char *opt_log_file,
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index 403a6d7cfa..828a975ac0 100644
--- a/src/bin/pg_upgrade/version.c
+++ b/src/bin/pg_upgrade/version.c
@@ -9,236 +9,41 @@
#include "postgres_fe.h"
-#include "catalog/pg_class_d.h"
#include "fe_utils/string_utils.h"
#include "pg_upgrade.h"
-
-/*
- * check_for_data_types_usage()
- * Detect whether there are any stored columns depending on given type(s)
- *
- * If so, write a report to the given file name, and return true.
- *
- * base_query should be a SELECT yielding a single column named "oid",
- * containing the pg_type OIDs of one or more types that are known to have
- * inconsistent on-disk representations across server versions.
- *
- * We check for the type(s) in tables, matviews, and indexes, but not views;
- * there's no storage involved in a view.
- */
bool
-check_for_data_types_usage(ClusterInfo *cluster,
- const char *base_query,
- const char *output_path)
+old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)
{
- bool found = false;
- FILE *script = NULL;
- int dbnum;
-
- for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
- {
- DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
- PGconn *conn = connectToServer(cluster, active_db->db_name);
- PQExpBufferData querybuf;
- PGresult *res;
- bool db_used = false;
- int ntups;
- int rowno;
- int i_nspname,
- i_relname,
- i_attname;
-
- /*
- * The type(s) of interest might be wrapped in a domain, array,
- * composite, or range, and these container types can be nested (to
- * varying extents depending on server version, but that's not of
- * concern here). To handle all these cases we need a recursive CTE.
- */
- initPQExpBuffer(&querybuf);
- appendPQExpBuffer(&querybuf,
- "WITH RECURSIVE oids AS ( "
- /* start with the type(s) returned by base_query */
- " %s "
- " UNION ALL "
- " SELECT * FROM ( "
- /* inner WITH because we can only reference the CTE once */
- " WITH x AS (SELECT oid FROM oids) "
- /* domains on any type selected so far */
- " SELECT t.oid FROM pg_catalog.pg_type t, x WHERE typbasetype = x.oid AND typtype = 'd' "
- " UNION ALL "
- /* arrays over any type selected so far */
- " SELECT t.oid FROM pg_catalog.pg_type t, x WHERE typelem = x.oid AND typtype = 'b' "
- " UNION ALL "
- /* composite types containing any type selected so far */
- " 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 "
- " NOT a.attisdropped AND "
- " a.atttypid = x.oid "
- " UNION ALL "
- /* ranges containing any type selected so far */
- " SELECT t.oid FROM pg_catalog.pg_type t, pg_catalog.pg_range r, x "
- " WHERE t.typtype = 'r' AND r.rngtypid = t.oid AND r.rngsubtype = x.oid"
- " ) foo "
- ") "
- /* now look for stored columns of any such type */
- "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 "
- /* exclude system catalogs, too */
- " n.nspname NOT IN ('pg_catalog', 'information_schema')",
- base_query);
-
- res = executeQueryOrDie(conn, "%s", querybuf.data);
-
- 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", output_path,
- strerror(errno));
- if (!db_used)
- {
- fprintf(script, "In 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);
-
- termPQExpBuffer(&querybuf);
-
- PQfinish(conn);
- }
-
- if (script)
- fclose(script);
+ /* Pre-PG 9.4 had a different 'line' data type internal format */
+ if (GET_MAJOR_VERSION(cluster->major_version) <= 903)
+ return true;
- return found;
+ return false;
}
/*
- * check_for_data_type_usage()
- * Detect whether there are any stored columns depending on the given type
- *
- * If so, write a report to the given file name, and return true.
+ * check_for_jsonb_9_4_usage()
*
- * type_name should be a fully qualified type name. This is just a
- * trivial wrapper around check_for_data_types_usage() to convert a
- * type name into a base query.
+ * JSONB changed its storage format during 9.4 beta, so check for it.
*/
bool
-check_for_data_type_usage(ClusterInfo *cluster,
- const char *type_name,
- const char *output_path)
-{
- bool found;
- char *base_query;
-
- base_query = psprintf("SELECT '%s'::pg_catalog.regtype AS oid",
- type_name);
-
- found = check_for_data_types_usage(cluster, base_query, output_path);
-
- free(base_query);
-
- return found;
-}
-
-
-/*
- * old_9_3_check_for_line_data_type_usage()
- * 9.3 -> 9.4
- * Fully implement the 'line' data type in 9.4, which previously returned
- * "not enabled" by default and was only functionally enabled with a
- * compile-time switch; as of 9.4 "line" has a different on-disk
- * representation format.
- */
-void
-old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)
+check_for_jsonb_9_4_usage(ClusterInfo *cluster)
{
- char output_path[MAXPGPATH];
+ if (GET_MAJOR_VERSION(cluster->major_version) == 904 &&
+ cluster->controldata.cat_ver < JSONB_FORMAT_CHANGE_CAT_VER)
+ return true;
- prep_status("Checking for incompatible \"line\" data type");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_line.txt");
-
- if (check_for_data_type_usage(cluster, "pg_catalog.line", output_path))
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains the \"line\" data type in user tables.\n"
- "This data type changed its internal and input/output format\n"
- "between your old and new versions so this\n"
- "cluster cannot currently be upgraded. You can\n"
- "drop the problem columns and restart the upgrade.\n"
- "A list of the problem columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
+ return false;
}
-
-/*
- * old_9_6_check_for_unknown_data_type_usage()
- * 9.6 -> 10
- * It's no longer allowed to create tables or views with "unknown"-type
- * columns. We do not complain about views with such columns, because
- * they should get silently converted to "text" columns during the DDL
- * dump and reload; it seems unlikely to be worth making users do that
- * by hand. However, if there's a table with such a column, the DDL
- * reload will fail, so we should pre-detect that rather than failing
- * mid-upgrade. Worse, if there's a matview with such a column, the
- * DDL reload will silently change it to "text" which won't match the
- * on-disk storage (which is like "cstring"). So we *must* reject that.
- */
-void
+bool
old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster)
{
- char output_path[MAXPGPATH];
-
- prep_status("Checking for invalid \"unknown\" user columns");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_unknown.txt");
-
- if (check_for_data_type_usage(cluster, "pg_catalog.unknown", output_path))
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains the \"unknown\" data type in user tables.\n"
- "This data type is no longer allowed in tables, so this\n"
- "cluster cannot currently be upgraded. You can\n"
- "drop the problem columns and restart the upgrade.\n"
- "A list of the problem columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
+ /* Pre-PG 10 allowed tables with 'unknown' type columns */
+ if (GET_MAJOR_VERSION(cluster->major_version) <= 906)
+ return true;
+ return false;
}
/*
@@ -353,41 +158,20 @@ old_9_6_invalidate_hash_indexes(ClusterInfo *cluster, bool check_mode)
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.
- */
-void
+bool
old_11_check_for_sql_identifier_data_type_usage(ClusterInfo *cluster)
{
- char output_path[MAXPGPATH];
-
- prep_status("Checking for invalid \"sql_identifier\" user columns");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_sql_identifier.txt");
-
- if (check_for_data_type_usage(cluster, "information_schema.sql_identifier",
- output_path))
- {
- pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains the \"sql_identifier\" data type in user tables.\n"
- "The on-disk format for this data type has changed, so this\n"
- "cluster cannot currently be upgraded. You can\n"
- "drop the problem columns and restart the upgrade.\n"
- "A list of the problem columns is in the file:\n"
- " %s", output_path);
- }
- else
- check_ok();
+ /*
+ * PG 12 changed the 'sql_identifier' type storage to be based on name,
+ * not varchar, which breaks on-disk format for existing data. So we need
+ * to prevent upgrade when used in user objects (tables, indexes, ...).
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1100)
+ return true;
+
+ return false;
}
-
/*
* report_extension_updates()
* Report extensions that should be updated.
@@ -459,3 +243,22 @@ report_extension_updates(ClusterInfo *cluster)
else
check_ok();
}
+
+/*
+ * check_for_aclitem_data_type_usage
+ *
+ * aclitem changed its storage format in 16, so check for it.
+ */
+bool
+check_for_aclitem_data_type_usage(ClusterInfo *cluster)
+{
+ /*
+ * PG 16 increased the size of the 'aclitem' type, which breaks the on-disk
+ * format for existing data.
+ */
+ if (GET_MAJOR_VERSION(cluster->major_version) <= 1500)
+ return true;
+
+ return false;
+}
+
--
2.25.1
--tKW2IUtsqtDRztdT--
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-07 22:47 David Rowley <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: David Rowley @ 2024-05-07 22:47 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Richard Guo <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, 8 May 2024 at 10:40, Tom Lane <[email protected]> wrote:
>
> David Rowley <[email protected]> writes:
> > We could make the policy copy-on-modify. If you put bms_copy around
> > the bms_del_member() calls in remove_rel_from_query(), does it pass
> > then?
>
> Didn't test, but that route seems awfully invasive and fragile: how
> will we find all the places to modify, or ensure that the policy
> is followed by future patches?
REALLOCATE_BITMAPSETS was invented for this and IMO, it found exactly
the problem it was invented to find.
Copy-on-modify is our policy for node mutation. Why is it ok there but
awfully fragile here?
David
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-07 22:55 Tom Lane <[email protected]>
parent: David Rowley <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Tom Lane @ 2024-05-07 22:55 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Richard Guo <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
David Rowley <[email protected]> writes:
> On Wed, 8 May 2024 at 10:40, Tom Lane <[email protected]> wrote:
>> Didn't test, but that route seems awfully invasive and fragile: how
>> will we find all the places to modify, or ensure that the policy
>> is followed by future patches?
> REALLOCATE_BITMAPSETS was invented for this and IMO, it found exactly
> the problem it was invented to find.
Not in a way that gives me any confidence that we found *all* the
problems. If check-world finds a problem that the core tests did not,
then there's no reason to think there aren't still more issues that
check-world happened not to trip over either.
> Copy-on-modify is our policy for node mutation. Why is it ok there but
> awfully fragile here?
It's only partly our policy: there are all those places where we don't
do it that way. The main problem that I see for trying to be 100%
consistent in that way is that once you modify a sub-node, full
copy-on-modify dictates replacing every ancestor node all the way to
the top of the tree. That's clearly impractical in the planner data
structures. So where are we going to stop exactly?
regards, tom lane
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-07 23:03 David Rowley <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: David Rowley @ 2024-05-07 23:03 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Richard Guo <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, 8 May 2024 at 10:55, Tom Lane <[email protected]> wrote:
>
> David Rowley <[email protected]> writes:
> > REALLOCATE_BITMAPSETS was invented for this and IMO, it found exactly
> > the problem it was invented to find.
>
> Not in a way that gives me any confidence that we found *all* the
> problems.
Here are some statements I believe to be true:
1. If REALLOCATE_BITMAPSETS is defined then modifications to a
Bitmapset will make a copy and free the original.
2. If a query runs successfully without REALLOCATE_BITMAPSETS and
Assert fails due to an invalid Bitmapset when REALLOCATE_BITMAPSETS is
defined, then we have > 1 pointer pointing to the same set and not all
of them are being updated when the members are added/removed.
Given the above, I can't see what Bitmapset sharing problems we won't
find with REALLOCATE_BITMAPSETS.
Can you share the exact scenario you're worried that we won't find so
I can understand your concern?
David
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-07 23:18 Tom Lane <[email protected]>
parent: David Rowley <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Tom Lane @ 2024-05-07 23:18 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Richard Guo <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
David Rowley <[email protected]> writes:
> On Wed, 8 May 2024 at 10:55, Tom Lane <[email protected]> wrote:
>> Not in a way that gives me any confidence that we found *all* the
>> problems.
> Here are some statements I believe to be true:
> 1. If REALLOCATE_BITMAPSETS is defined then modifications to a
> Bitmapset will make a copy and free the original.
> 2. If a query runs successfully without REALLOCATE_BITMAPSETS and
> Assert fails due to an invalid Bitmapset when REALLOCATE_BITMAPSETS is
> defined, then we have > 1 pointer pointing to the same set and not all
> of them are being updated when the members are added/removed.
> Given the above, I can't see what Bitmapset sharing problems we won't
> find with REALLOCATE_BITMAPSETS.
Anything where the trouble spots are in a code path we fail to
exercise with our available test suites. If you think there are
no such code paths, I'm sorry to disillusion you.
I spent a little bit of time wondering if we could find problems in a
more static way by marking bitmapset fields as "const", but I fear
that would create a huge number of false positives.
regards, tom lane
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-08 18:24 Tom Lane <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 2 replies; 18+ messages in thread
From: Tom Lane @ 2024-05-08 18:24 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Richard Guo <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
I traced down the other failure I was seeing in check-world, and
found that it came from deconstruct_distribute doing this:
distribute_quals_to_rels(root, my_quals,
jtitem,
sjinfo,
root->qual_security_level,
jtitem->qualscope,
ojscope, jtitem->nonnullable_rels,
NULL, /* incompatible_relids */
true, /* allow_equivalence */
false, false, /* not clones */
postponed_oj_qual_list);
where jtitem->nonnullable_rels is the same as the jtitem's left_rels,
which ends up as the syn_lefthand of the join's SpecialJoinInfo, and
then when remove_rel_from_query tries to adjust the syn_lefthand it
breaks the outer_relids of whatever RestrictInfos got made here.
I was able to fix that by not letting jtitem->nonnullable_rels be
the same as left_rels. The attached alternative_1.patch does pass
check-world. But I find it mighty unprincipled: the JoinTreeItem
data structures are full of shared relid sets, so why is this
particular sharing not OK? I still don't have any confidence that
there aren't more problems.
Along about here I started to wonder how come we are only seeing
SpecialJoinInfo-vs-RestrictInfo sharing as a problem, when surely
there is plenty of cross-RestrictInfo sharing going on as well.
(The above call is perfectly capable of making a lot of RestrictInfos,
all with the same outer_relids.) That thought led me to look at
remove_rel_from_restrictinfo, and darn if I didn't find this:
/*
* The clause_relids probably aren't shared with anything else, but let's
* copy them just to be sure.
*/
rinfo->clause_relids = bms_copy(rinfo->clause_relids);
...
/* Likewise for required_relids */
rinfo->required_relids = bms_copy(rinfo->required_relids);
So the reason we don't see cross-RestrictInfo breakage is that
analyzejoins.c is careful not to modify the original relid sets
when modifying a RestrictInfo. (This comment is clearly wrong.)
And that leads to the thought that we can fix our current sharing
problems by similarly avoiding overwriting the original sets
in remove_rel_from_query. The attached alternative-2.patch
attacks it that way, and also passes check-world.
I like alternative-2.patch a lot better, not least because it
only adds cycles when join removal actually fires. Basically
this is putting the onus on the data structure modifier to
cope with shared bitmapsets, rather than trying to say that
sharing is disallowed.
Thoughts?
regards, tom lane
Attachments:
[text/x-diff] alternative-1.patch (1.5K, ../../[email protected]/2-alternative-1.patch)
download | inline diff:
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index e2c68fe6f9..4198e9c83a 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -979,7 +979,7 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode,
right_item->inner_join_rels);
jtitem->left_rels = left_item->qualscope;
jtitem->right_rels = right_item->qualscope;
- jtitem->nonnullable_rels = left_item->qualscope;
+ jtitem->nonnullable_rels = bms_copy(left_item->qualscope);
break;
case JOIN_SEMI:
/* This node belongs to parent_domain, as do its children */
@@ -1053,7 +1053,7 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode,
jtitem->left_rels = left_item->qualscope;
jtitem->right_rels = right_item->qualscope;
/* each side is both outer and inner */
- jtitem->nonnullable_rels = jtitem->qualscope;
+ jtitem->nonnullable_rels = bms_copy(jtitem->qualscope);
break;
default:
/* JOIN_RIGHT was eliminated during reduce_outer_joins() */
@@ -1888,7 +1888,7 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
qualscope = bms_union(sjinfo->syn_lefthand, sjinfo->syn_righthand);
qualscope = bms_add_member(qualscope, sjinfo->ojrelid);
ojscope = bms_union(sjinfo->min_lefthand, sjinfo->min_righthand);
- nonnullable_rels = sjinfo->syn_lefthand;
+ nonnullable_rels = bms_copy(sjinfo->syn_lefthand);
/*
* If this join can commute with any other ones per outer-join identity 3,
[text/x-diff] alternative-2.patch (1.8K, ../../[email protected]/3-alternative-2.patch)
download | inline diff:
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index aa72592567..1c9acf864c 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -390,6 +390,17 @@ remove_rel_from_query(PlannerInfo *root, int relid, SpecialJoinInfo *sjinfo)
{
SpecialJoinInfo *sjinf = (SpecialJoinInfo *) lfirst(l);
+ /*
+ * initsplan.c is fairly cavalier about allowing SpecialJoinInfos'
+ * lefthand/righthand relid sets to be shared with other data
+ * structures. Ensure that we don't modify the original relid sets.
+ * (The commute_xxx sets are always per-SpecialJoinInfo though.)
+ */
+ sjinf->min_lefthand = bms_copy(sjinf->min_lefthand);
+ sjinf->min_righthand = bms_copy(sjinf->min_righthand);
+ sjinf->syn_lefthand = bms_copy(sjinf->syn_lefthand);
+ sjinf->syn_righthand = bms_copy(sjinf->syn_righthand);
+ /* Now remove relid and ojrelid bits from the sets: */
sjinf->min_lefthand = bms_del_member(sjinf->min_lefthand, relid);
sjinf->min_righthand = bms_del_member(sjinf->min_righthand, relid);
sjinf->syn_lefthand = bms_del_member(sjinf->syn_lefthand, relid);
@@ -551,8 +562,10 @@ static void
remove_rel_from_restrictinfo(RestrictInfo *rinfo, int relid, int ojrelid)
{
/*
- * The clause_relids probably aren't shared with anything else, but let's
- * copy them just to be sure.
+ * initsplan.c is fairly cavalier about allowing RestrictInfos to share
+ * relid sets with other RestrictInfos (and SpecialJoinInfos for that
+ * matter). To avoid breaking things, make sure this RestrictInfo has its
+ * own clause_relids set before we modify it.
*/
rinfo->clause_relids = bms_copy(rinfo->clause_relids);
rinfo->clause_relids = bms_del_member(rinfo->clause_relids, relid);
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-08 18:49 Tom Lane <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: Tom Lane @ 2024-05-08 18:49 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Richard Guo <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
BTW, now that I've wrapped my head around what's happening here,
I believe that -DREALLOCATE_BITMAPSETS is introducing a bug where
there was none before. The changes that left-join removal makes
won't cause any of these sets to go to empty, so the bms_del_member
calls won't free the sets but just modify them in-place. And the
same change will/should be made in every relevant relid set, so
the fact that the sets may be shared isn't hurting anything.
This is, of course, pretty fragile and I'm totally on board with
making it safer. But there's no live bug in released branches,
and not in HEAD either unless you add -DREALLOCATE_BITMAPSETS.
That accounts for the lack of related field reports, and it means
we don't really need to back-patch anything.
This conclusion also reinforces my previously-vague feeling that
we should not consider making -DREALLOCATE_BITMAPSETS the default in
debug builds, as was proposed upthread. It's really a fundamentally
different behavior, and I strongly suspect that it can mask bugs as
well as introduce them (by hiding sharing in cases that'd be less
benign than this turns out to be). I'd rather not do development on
top of bitmapset infrastructure that acts entirely different from
production bitmapset infrastructure.
regards, tom lane
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-08 22:02 David Rowley <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: David Rowley @ 2024-05-08 22:02 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Richard Guo <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, 9 May 2024 at 06:49, Tom Lane <[email protected]> wrote:
> BTW, now that I've wrapped my head around what's happening here,
> I believe that -DREALLOCATE_BITMAPSETS is introducing a bug where
> there was none before. The changes that left-join removal makes
> won't cause any of these sets to go to empty, so the bms_del_member
> calls won't free the sets but just modify them in-place. And the
> same change will/should be made in every relevant relid set, so
> the fact that the sets may be shared isn't hurting anything.
FWIW, it just feels like we're willing to accept that the
bms_del_member() is not updating all copies of the set in this case as
that particular behaviour is ok for this particular case. I know
you're not proposing this, but I don't think that would warrant
relaxing REALLOCATE_BITMAPSETS to not reallocate Bitmapsets on
bms_del_member() and bms_del_members().
If all we have to do to make -DREALLOCATE_BITMAPSETS builds happy in
make check-world is to add a bms_copy inside the bms_del_member()
calls in remove_rel_from_query(), then I think it's a small price to
pay to allow us to maintain the additional coverage that
REALLOCATE_BITMAPSETS provides. That seems like a small price to pay
when the gains are removing an entire join.
> This conclusion also reinforces my previously-vague feeling that
> we should not consider making -DREALLOCATE_BITMAPSETS the default in
> debug builds, as was proposed upthread. It's really a fundamentally
> different behavior, and I strongly suspect that it can mask bugs as
> well as introduce them (by hiding sharing in cases that'd be less
> benign than this turns out to be). I'd rather not do development on
> top of bitmapset infrastructure that acts entirely different from
> production bitmapset infrastructure.
My primary interest in this feature is using it to catch bugs that
we're unlikely to ever hit in the regression tests. For example, the
planner works when there are <= 63 RTEs but falls over when there are
64 because some bms_add_member() must reallocate more memory to store
the 64th RTI in a Bitmapset. I'd like to have something to make it
more likely we'll find bugs like this before the release instead of
someone having a crash when they run some obscure query shape
containing > 63 RTEs 2 or 4 years after the release.
I'm happy Andrew added this to prion. Thanks for doing that.
David
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-08 22:07 David Rowley <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: David Rowley @ 2024-05-08 22:07 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Richard Guo <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, 9 May 2024 at 06:24, Tom Lane <[email protected]> wrote:
> I like alternative-2.patch a lot better, not least because it
> only adds cycles when join removal actually fires. Basically
> this is putting the onus on the data structure modifier to
> cope with shared bitmapsets, rather than trying to say that
> sharing is disallowed.
>
> Thoughts?
I'm fine with this one as it's the same as what I already mentioned
earlier. I had imagined doing bms_del_member(bms_copy ... but maybe
the compiler is able to optimise away the additional store. Likely, it
does not matter much as pallocing memory likely adds far more overhead
anyway.
David
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-08 22:16 Tom Lane <[email protected]>
parent: David Rowley <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Tom Lane @ 2024-05-08 22:16 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Richard Guo <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
David Rowley <[email protected]> writes:
> On Thu, 9 May 2024 at 06:49, Tom Lane <[email protected]> wrote:
>> BTW, now that I've wrapped my head around what's happening here,
>> I believe that -DREALLOCATE_BITMAPSETS is introducing a bug where
>> there was none before. The changes that left-join removal makes
>> won't cause any of these sets to go to empty, so the bms_del_member
>> calls won't free the sets but just modify them in-place. And the
>> same change will/should be made in every relevant relid set, so
>> the fact that the sets may be shared isn't hurting anything.
> FWIW, it just feels like we're willing to accept that the
> bms_del_member() is not updating all copies of the set in this case as
> that particular behaviour is ok for this particular case. I know
> you're not proposing this,
No, I'm not. I was just trying to explain how come there's not
a visible bug. I quite agree that this is too fragile to leave
as-is going forward. (One thing I'm wondering about is whether
we should back-patch despite the lack of visible bug, just in
case some future back-patch relies on the safer behavior.)
>> This conclusion also reinforces my previously-vague feeling that
>> we should not consider making -DREALLOCATE_BITMAPSETS the default in
>> debug builds, as was proposed upthread.
> My primary interest in this feature is using it to catch bugs that
> we're unlikely to ever hit in the regression tests. For example, the
> planner works when there are <= 63 RTEs but falls over when there are
> 64 because some bms_add_member() must reallocate more memory to store
> the 64th RTI in a Bitmapset. I'd like to have something to make it
> more likely we'll find bugs like this before the release instead of
> someone having a crash when they run some obscure query shape
> containing > 63 RTEs 2 or 4 years after the release.
Again, I think -DREALLOCATE_BITMAPSETS adds a valuable testing weapon.
But if we make that the default in debug builds, then we'll get next
door to zero testing of the behavior without it, and that seems like
a really bad idea given how different the behavior is.
(Speaking of which, I wonder how many buildfarm members build without
--enable-cassert. The answer could well be "zero", and that's likely
not good.)
> I'm happy Andrew added this to prion. Thanks for doing that.
+1
regards, tom lane
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-08 22:40 Tom Lane <[email protected]>
parent: David Rowley <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Tom Lane @ 2024-05-08 22:40 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Richard Guo <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
David Rowley <[email protected]> writes:
> I'm fine with this one as it's the same as what I already mentioned
> earlier. I had imagined doing bms_del_member(bms_copy ... but maybe
> the compiler is able to optimise away the additional store. Likely, it
> does not matter much as pallocing memory likely adds far more overhead
> anyway.
I actually wrote it that way to start with, but undid it after
noticing that the existing code in remove_rel_from_restrictinfo
does it in separate steps, and thinking that that was good for
both separation of concerns and a cleaner git history. I too
can't believe that an extra fetch will be noticeable compared
to the cost of the adjacent bms_xxx operations.
regards, tom lane
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-09 02:43 Richard Guo <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Richard Guo @ 2024-05-09 02:43 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: David Rowley <[email protected]>; Andrew Dunstan <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, May 9, 2024 at 6:40 AM Tom Lane <[email protected]> wrote:
> David Rowley <[email protected]> writes:
> > I'm fine with this one as it's the same as what I already mentioned
> > earlier. I had imagined doing bms_del_member(bms_copy ... but maybe
> > the compiler is able to optimise away the additional store. Likely, it
> > does not matter much as pallocing memory likely adds far more overhead
> > anyway.
>
> I actually wrote it that way to start with, but undid it after
> noticing that the existing code in remove_rel_from_restrictinfo
> does it in separate steps, and thinking that that was good for
> both separation of concerns and a cleaner git history. I too
> can't believe that an extra fetch will be noticeable compared
> to the cost of the adjacent bms_xxx operations.
I also think it seems better to do bms_copy in separate steps, not only
because this keeps consistent with the existing code in
remove_rel_from_restrictinfo, but also because we need to do
bms_del_member twice for each lefthand/righthand relid set.
Speaking of consistency, do you think it would improve the code's
readability if we rearrange the code in remove_rel_from_query so that
the modifications of the same relid set are grouped together, just like
what we do in remove_rel_from_restrictinfo? I mean something like:
sjinf->min_lefthand = bms_copy(sjinf->min_lefthand);
sjinf->min_lefthand = bms_del_member(sjinf->min_lefthand, relid);
sjinf->min_lefthand = bms_del_member(sjinf->min_lefthand, ojrelid);
...
Thanks
Richard
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail.
@ 2024-05-09 15:07 Tom Lane <[email protected]>
parent: Richard Guo <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Tom Lane @ 2024-05-09 15:07 UTC (permalink / raw)
To: Richard Guo <[email protected]>; +Cc: David Rowley <[email protected]>; Andrew Dunstan <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
Richard Guo <[email protected]> writes:
> I also think it seems better to do bms_copy in separate steps, not only
> because this keeps consistent with the existing code in
> remove_rel_from_restrictinfo, but also because we need to do
> bms_del_member twice for each lefthand/righthand relid set.
Yeah. Of course, we don't need a bms_copy() in the second one,
but that'd just add even more asymmetry and chance for confusion.
> Speaking of consistency, do you think it would improve the code's
> readability if we rearrange the code in remove_rel_from_query so that
> the modifications of the same relid set are grouped together, just like
> what we do in remove_rel_from_restrictinfo?
I left it alone, just because it didn't seem worth cluttering "git
blame" here.
regards, tom lane
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: FileFallocate misbehaving on XFS
@ 2025-03-11 01:39 Michael Harris <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Michael Harris @ 2025-03-11 01:39 UTC (permalink / raw)
To: Jakub Wartak <[email protected]>; +Cc: Jean-Christophe Arnu <[email protected]>; Andres Freund <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers
On Tue, 4 Mar 2025 at 23:05, Jakub Wartak <[email protected]> wrote:
> out of curiosity, did Redhat provide any further (deeper) information
> on why too big preallocation (or what heuristics) on XFS can cause
> such issues?
Hi Jakub
So far I don't have any feedback from RH. Unfortunately there is a
long corporate chain of people between me and whomever in RH is
dealing with the ticket! I will update this list if/when I get an
update.
Cheers
Mike
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: FileFallocate misbehaving on XFS
@ 2025-03-27 10:25 Andrea Gelmini <[email protected]>
1 sibling, 0 replies; 18+ messages in thread
From: Andrea Gelmini @ 2025-03-27 10:25 UTC (permalink / raw)
To: Michael Harris <[email protected]>; pgsql-hackers
Il giorno lun 9 dic 2024 alle ore 10:47 Andrea Gelmini <
[email protected]> ha scritto:
>
> Funny, i guess it's the same reason I see randomly complain of WhatsApp
> web interface, on Chrome, since I switched to XFS. It says something like
> "no more space on disk" and logout, with more than 300GB available.
>
To be fair, I found after months (changes fs, and googled about), it's not
XFS related, and it happens with different browsers, also. So, my suspect
was wrong.
Ciao,
Gelma
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: FileFallocate misbehaving on XFS
@ 2025-05-11 18:30 Pierre Barre <[email protected]>
1 sibling, 0 replies; 18+ messages in thread
From: Pierre Barre @ 2025-05-11 18:30 UTC (permalink / raw)
To: [email protected]
Hello,
I was running into the same thing, and the fix for me was mounting xfs with -o inodes64
Best,
Pierre
On Mon, Dec 9, 2024, at 08:34, Michael Harris wrote:
> Hello PG Hackers
>
> Our application has recently migrated to PG16, and we have experienced
> some failed upgrades. The upgrades are performed using pg_upgrade and
> have failed during the phase where the schema is restored into the new
> cluster, with the following error:
>
> pg_restore: error: could not execute query: ERROR: could not extend
> file "pg_tblspc/16401/PG_16_202307071/17643/1249.1" with
> FileFallocate(): No space left on device
> HINT: Check free disk space.
>
> This has happened multiple times on different servers, and in each
> case there was plenty of free space available.
>
> We found this thread describing similar issues:
>
> https://www.postgresql.org/message-id/flat/AS1PR05MB91059AC8B525910A5FCD6E699F9A2%40AS1PR05MB9105.eu...
>
> As is the case in that thread, all of the affected databases are using XFS.
>
> One of my colleagues built postgres from source with
> HAVE_POSIX_FALLOCATE not defined, and using that build he was able to
> complete the pg_upgrade, and then switched to a stock postgres build
> after the upgrade. However, as you might expect, after the upgrade we
> have experienced similar errors during regular operation. We make
> heavy use of COPY, which is mentioned in the above discussion as
> pre-allocating files.
>
> We have seen this on both Rocky Linux 8 (kernel 4.18.0) and Rocky
> Linux 9 (Kernel 5.14.0).
>
> I am wondering if this bug might be related:
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1791323
>
>> When given an offset of 0 and a length, fallocate (man 2 fallocate) reports ENOSPC if the size of the file + the length to be allocated is greater than the available space.
>
> There is a reproduction procedure at the bottom of the above ubuntu
> thread, and using that procedure I get the same results on both kernel
> 4.18.0 and 5.14.0.
> When calling fallocate with offset zero on an existing file, I get
> enospc even if I am only requesting the same amount of space as the
> file already has.
> If I repeat the experiment with ext4 I don't get that behaviour.
>
> On a surface examination of the code paths leading to the
> FileFallocate call, it does not look like it should be trying to
> allocate already allocated space, but I might have missed something
> there.
>
> Is this already being looked into?
>
> Thanks in advance,
>
> Cheers
> Mike
^ permalink raw reply [nested|flat] 18+ messages in thread
end of thread, other threads:[~2025-05-11 18:30 UTC | newest]
Thread overview: 18+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-02-27 01:22 [PATCH v25 4/4] Avoid some calls to RelationGetRelationName Justin Pryzby <[email protected]>
2023-03-13 13:46 [PATCH v4 1/1] pg_upgrade: run all data type checks per connection Daniel Gustafsson <[email protected]>
2023-03-13 13:46 [PATCH v4 1/1] pg_upgrade: run all data type checks per connection Daniel Gustafsson <[email protected]>
2024-05-07 22:47 Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. David Rowley <[email protected]>
2024-05-07 22:55 ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. Tom Lane <[email protected]>
2024-05-07 23:03 ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. David Rowley <[email protected]>
2024-05-07 23:18 ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. Tom Lane <[email protected]>
2024-05-08 18:24 ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. Tom Lane <[email protected]>
2024-05-08 18:49 ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. Tom Lane <[email protected]>
2024-05-08 22:02 ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. David Rowley <[email protected]>
2024-05-08 22:16 ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. Tom Lane <[email protected]>
2024-05-08 22:07 ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. David Rowley <[email protected]>
2024-05-08 22:40 ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. Tom Lane <[email protected]>
2024-05-09 02:43 ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. Richard Guo <[email protected]>
2024-05-09 15:07 ` Re: Revert: Remove useless self-joins *and* -DREALLOCATE_BITMAPSETS make server crash, regress test fail. Tom Lane <[email protected]>
2025-03-11 01:39 Re: FileFallocate misbehaving on XFS Michael Harris <[email protected]>
2025-03-27 10:25 ` Re: FileFallocate misbehaving on XFS Andrea Gelmini <[email protected]>
2025-05-11 18:30 ` Re: FileFallocate misbehaving on XFS Pierre Barre <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox