public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v4 1/1] pg_upgrade: run all data type checks per connection
28+ messages / 10 participants
[nested] [flat]
* [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; 28+ 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] 28+ 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; 28+ 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] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-10 19:05 Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Alexander Korotkov @ 2024-05-10 19:05 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
On Fri, May 10, 2024 at 8:35 PM Mark Dilger
<[email protected]> wrote:
> > On May 10, 2024, at 5:10 AM, Pavel Borisov <[email protected]> wrote:
> > On Fri, 10 May 2024 at 12:39, Alexander Korotkov <[email protected]> wrote:
> > On Fri, May 10, 2024 at 3:43 AM Tom Lane <[email protected]> wrote:
> > > Alexander Korotkov <[email protected]> writes:
> > > > The revised patchset is attached. I applied cosmetical changes. I'm
> > > > going to push it if no objections.
> > >
> > > Is this really suitable material to be pushing post-feature-freeze?
> > > It doesn't look like it's fixing any new-in-v17 issues.
> >
> > These are code improvements to the 5ae2087202, which answer critics in
> > the thread. 0001 comprises an optimization, but it's rather small and
> > simple. 0002 and 0003 contain refactoring. 0004 contains better
> > error reporting. For me this looks like pretty similar to what others
> > commit post-FF, isn't it?
> > I've re-checked patches v2. Differences from v1 are in improving naming/pgindent's/commit messages.
> > In 0002 order of variables in struct BtreeLastVisibleEntry changed. This doesn't change code behavior.
> >
> > Patch v2-0003 doesn't contain credits and a discussion link. All other patches do.
> >
> > Overall, patches contain small performance optimization (0001), code refactoring and error reporting changes. IMO they could be pushed post-FF.
>
> v2-0001's commit message itself says, "This commit implements skipping keys". I take no position on the correctness or value of the improvement, but it seems out of scope post feature freeze. The patch seems to postpone uniqueness checking until later in the scan than what the prior version did, and that kind of change could require more analysis than we have time for at this point in the release cycle.
Formally this could be classified as algorithmic change and probably
should be postponed to the next release. But that's quite local
optimization, which just postpones a function call within the same
iteration of loop. And the effect is huge. Probably we could allow
this post-FF in the sake of quality release, given it's very local
change with a huge effect.
> v2-0002 does appear to just be refactoring. I don't care for a small portion of that patch, but I doubt it violates the post feature freeze rules. In particular:
>
> + BtreeLastVisibleEntry lVis = {InvalidBlockNumber, InvalidOffsetNumber, -1, NULL};
I don't understand what is the problem with this line and post feature
freeze rules. Please, explain it more.
> v2-0003 may be an improvement in some way, but it compounds some preexisting confusion also. There is already a member of the BtreeCheckState called "target" and a memory context in that struct called "targetcontext". That context is used to allocate pages "state->target", "rightpage", "child" and "page", but not "metapage". Perhaps "targetcontext" is a poor choice of name? "notmetacontext" is a terrible name, but closer to describing the purpose of the memory context. Care to propose something sensible?
>
> Prior to applying v2-0003, the rightpage was stored in state->target, and continued to be in state->target later when entering
>
> /*
> * * Downlink check *
> *
> * Additional check of child items iff this is an internal page and
> * caller holds a ShareLock. This happens for every downlink (item)
> * in target excluding the negative-infinity downlink (again, this is
> * because it has no useful value to compare).
> */
> if (!P_ISLEAF(topaque) && state->readonly)
> bt_child_check(state, skey, offset);
>
> and thereafter. Now, the rightpage of state->target is created, checked, and free'd, and then the old state->target gets processed in the downlink check and thereafter. This is either introducing a bug, or fixing one, but the commit message is totally ambiguous about whether this is a bugfix or a code cleanup or something else? I think this kind of patch should have a super clear commit message about what it thinks it is doing.
The only bt_target_page_check() caller is
bt_check_level_from_leftmost(), which overrides state->target in the
next iteration anyway. I think the patch is just refactoring to
eliminate the confusion pointer by Peter Geoghegan upthread.
0002 and 0003 don't address any bugs, but It would be very nice to
accept them, because it would simplify future backpatching in this
area.
> v2-0004 guards against a real threat, and is reasonable post feature freeze
Ok.
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-11 01:12 Mark Dilger <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 2 replies; 28+ messages in thread
From: Mark Dilger @ 2024-05-11 01:12 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
> On May 10, 2024, at 12:05 PM, Alexander Korotkov <[email protected]> wrote:
>
> The only bt_target_page_check() caller is
> bt_check_level_from_leftmost(), which overrides state->target in the
> next iteration anyway. I think the patch is just refactoring to
> eliminate the confusion pointer by Peter Geoghegan upthread.
I find your argument unconvincing.
After bt_target_page_check() returns at line 919, and before bt_check_level_from_leftmost() overrides state->target in the next iteration, bt_check_level_from_leftmost() conditionally fetches an item from the page referenced by state->target. See line 963.
I'm left with four possibilities:
1) bt_target_page_check() never gets to the code that uses "rightpage" rather than "state->target" in the same iteration where bt_check_level_from_leftmost() conditionally fetches an item from state->target, so the change you're making doesn't matter.
2) The code prior to v2-0003 was wrong, having changed state->target in an inappropriate way, causing the wrong thing to happen at what is now line 963. The patch fixes the bug, because state->target no longer gets overwritten where you are now using "rightpage" for the value.
3) The code used to work, having set up state->target correctly in the place where you are now using "rightpage", but v2-0003 has broken that.
4) It's been broken all along and your patch just changes from wrong to wrong.
If you believe (1) is true, then I'm complaining that you are relying far to much on action at a distance, and that you are not documenting it. Even with documentation of this interrelationship, I'd be unhappy with how brittle the code is. I cannot easily discern that the two don't ever happen in the same iteration, and I'm not at all convinced one way or the other. I tried to set up some Asserts about that, but none of the test cases actually reach the new code, so adding Asserts doesn't help to investigate the question.
If (2) is true, then I'm complaining that the commit message doesn't mention the fact that this is a bug fix. Bug fixes should be clearly documented as such, otherwise future work might assume the commit can be reverted with only stylistic consequences.
If (3) is true, then I'm complaining that the patch is flat busted.
If (4) is true, then maybe we should revert the entire feature, or have a discussion of mitigation efforts that are needed.
Regardless of which of 1..4 you pick, I think it could all do with more regression test coverage.
For reference, I said something similar earlier today in another email to this thread:
This patch introduces a change that stores a new page into variable "rightpage" rather than overwriting "state->target", which the old implementation most certainly did. That means that after returning from bt_target_page_check() into the calling function bt_check_level_from_leftmost() the value in state->target is not what it would have been prior to this patch. Now, that'd be irrelevant if nobody goes on to consult that value, but just 44 lines further down in bt_check_level_from_leftmost() state->target is clearly used. So the behavior at that point is changing between the old and new versions of the code, and I think I'm within reason to ask if it was wrong before the patch, wrong after the patch, or something else? Is this a bug being introduced, being fixed, or ... ?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-11 01:38 Tom Lane <[email protected]>
parent: Mark Dilger <[email protected]>
1 sibling, 0 replies; 28+ messages in thread
From: Tom Lane @ 2024-05-11 01:38 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Pavel Borisov <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
Mark Dilger <[email protected]> writes:
> Regardless of which of 1..4 you pick, I think it could all do with more regression test coverage.
Indeed. If we have no regression tests that reach this code, it's
folly to touch it at all, but most especially so post-feature-freeze.
I think the *first* order of business ought to be to create some
test cases that reach this area. Perhaps they'll be too expensive
to incorporate in our regular regression tests, but we could still
use them to investigate Mark's concerns.
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-12 21:23 Alexander Korotkov <[email protected]>
parent: Mark Dilger <[email protected]>
1 sibling, 1 reply; 28+ messages in thread
From: Alexander Korotkov @ 2024-05-12 21:23 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
On Sat, May 11, 2024 at 4:13 AM Mark Dilger
<[email protected]> wrote:
> > On May 10, 2024, at 12:05 PM, Alexander Korotkov <[email protected]> wrote:
> > The only bt_target_page_check() caller is
> > bt_check_level_from_leftmost(), which overrides state->target in the
> > next iteration anyway. I think the patch is just refactoring to
> > eliminate the confusion pointer by Peter Geoghegan upthread.
>
> I find your argument unconvincing.
>
> After bt_target_page_check() returns at line 919, and before bt_check_level_from_leftmost() overrides state->target in the next iteration, bt_check_level_from_leftmost() conditionally fetches an item from the page referenced by state->target. See line 963.
>
> I'm left with four possibilities:
>
>
> 1) bt_target_page_check() never gets to the code that uses "rightpage" rather than "state->target" in the same iteration where bt_check_level_from_leftmost() conditionally fetches an item from state->target, so the change you're making doesn't matter.
>
> 2) The code prior to v2-0003 was wrong, having changed state->target in an inappropriate way, causing the wrong thing to happen at what is now line 963. The patch fixes the bug, because state->target no longer gets overwritten where you are now using "rightpage" for the value.
>
> 3) The code used to work, having set up state->target correctly in the place where you are now using "rightpage", but v2-0003 has broken that.
>
> 4) It's been broken all along and your patch just changes from wrong to wrong.
>
>
> If you believe (1) is true, then I'm complaining that you are relying far to much on action at a distance, and that you are not documenting it. Even with documentation of this interrelationship, I'd be unhappy with how brittle the code is. I cannot easily discern that the two don't ever happen in the same iteration, and I'm not at all convinced one way or the other. I tried to set up some Asserts about that, but none of the test cases actually reach the new code, so adding Asserts doesn't help to investigate the question.
>
> If (2) is true, then I'm complaining that the commit message doesn't mention the fact that this is a bug fix. Bug fixes should be clearly documented as such, otherwise future work might assume the commit can be reverted with only stylistic consequences.
>
> If (3) is true, then I'm complaining that the patch is flat busted.
>
> If (4) is true, then maybe we should revert the entire feature, or have a discussion of mitigation efforts that are needed.
>
> Regardless of which of 1..4 you pick, I think it could all do with more regression test coverage.
>
>
> For reference, I said something similar earlier today in another email to this thread:
>
> This patch introduces a change that stores a new page into variable "rightpage" rather than overwriting "state->target", which the old implementation most certainly did. That means that after returning from bt_target_page_check() into the calling function bt_check_level_from_leftmost() the value in state->target is not what it would have been prior to this patch. Now, that'd be irrelevant if nobody goes on to consult that value, but just 44 lines further down in bt_check_level_from_leftmost() state->target is clearly used. So the behavior at that point is changing between the old and new versions of the code, and I think I'm within reason to ask if it was wrong before the patch, wrong after the patch, or something else? Is this a bug being introduced, being fixed, or ... ?
Thank you for your analysis. I'm inclined to believe in 2, but not
yet completely sure. It's really pity that our tests don't cover
this. I'm investigating this area.
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-13 01:42 Alexander Korotkov <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 2 replies; 28+ messages in thread
From: Alexander Korotkov @ 2024-05-13 01:42 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
On Mon, May 13, 2024 at 12:23 AM Alexander Korotkov
<[email protected]> wrote:
> On Sat, May 11, 2024 at 4:13 AM Mark Dilger
> <[email protected]> wrote:
> > > On May 10, 2024, at 12:05 PM, Alexander Korotkov <[email protected]> wrote:
> > > The only bt_target_page_check() caller is
> > > bt_check_level_from_leftmost(), which overrides state->target in the
> > > next iteration anyway. I think the patch is just refactoring to
> > > eliminate the confusion pointer by Peter Geoghegan upthread.
> >
> > I find your argument unconvincing.
> >
> > After bt_target_page_check() returns at line 919, and before bt_check_level_from_leftmost() overrides state->target in the next iteration, bt_check_level_from_leftmost() conditionally fetches an item from the page referenced by state->target. See line 963.
> >
> > I'm left with four possibilities:
> >
> >
> > 1) bt_target_page_check() never gets to the code that uses "rightpage" rather than "state->target" in the same iteration where bt_check_level_from_leftmost() conditionally fetches an item from state->target, so the change you're making doesn't matter.
> >
> > 2) The code prior to v2-0003 was wrong, having changed state->target in an inappropriate way, causing the wrong thing to happen at what is now line 963. The patch fixes the bug, because state->target no longer gets overwritten where you are now using "rightpage" for the value.
> >
> > 3) The code used to work, having set up state->target correctly in the place where you are now using "rightpage", but v2-0003 has broken that.
> >
> > 4) It's been broken all along and your patch just changes from wrong to wrong.
> >
> >
> > If you believe (1) is true, then I'm complaining that you are relying far to much on action at a distance, and that you are not documenting it. Even with documentation of this interrelationship, I'd be unhappy with how brittle the code is. I cannot easily discern that the two don't ever happen in the same iteration, and I'm not at all convinced one way or the other. I tried to set up some Asserts about that, but none of the test cases actually reach the new code, so adding Asserts doesn't help to investigate the question.
> >
> > If (2) is true, then I'm complaining that the commit message doesn't mention the fact that this is a bug fix. Bug fixes should be clearly documented as such, otherwise future work might assume the commit can be reverted with only stylistic consequences.
> >
> > If (3) is true, then I'm complaining that the patch is flat busted.
> >
> > If (4) is true, then maybe we should revert the entire feature, or have a discussion of mitigation efforts that are needed.
> >
> > Regardless of which of 1..4 you pick, I think it could all do with more regression test coverage.
> >
> >
> > For reference, I said something similar earlier today in another email to this thread:
> >
> > This patch introduces a change that stores a new page into variable "rightpage" rather than overwriting "state->target", which the old implementation most certainly did. That means that after returning from bt_target_page_check() into the calling function bt_check_level_from_leftmost() the value in state->target is not what it would have been prior to this patch. Now, that'd be irrelevant if nobody goes on to consult that value, but just 44 lines further down in bt_check_level_from_leftmost() state->target is clearly used. So the behavior at that point is changing between the old and new versions of the code, and I think I'm within reason to ask if it was wrong before the patch, wrong after the patch, or something else? Is this a bug being introduced, being fixed, or ... ?
>
> Thank you for your analysis. I'm inclined to believe in 2, but not
> yet completely sure. It's really pity that our tests don't cover
> this. I'm investigating this area.
It seems that I got to the bottom of this. Changing
BtreeCheckState.target for a cross-page unique constraint check is
wrong, but that happens only for leaf pages. After that
BtreeCheckState.target is only used for setting the low key. The low
key is only used for non-leaf pages. So, that didn't lead to any
visible bug. I've revised the commit message to reflect this.
So, the picture for the patches is the following now.
0001 – optimization, but rather simple and giving huge effect
0002 – refactoring
0003 – fix for the bug
0004 – better error reporting
------
Regards,
Alexander Korotkov
Attachments:
[application/octet-stream] v3-0002-amcheck-Refactoring-the-storage-of-the-last-visib.patch (11.0K, ../../CAPpHfdu6bGLxe-+9jfDwoji_T2iD6mjdiAGBr0JT-iX+TWNWOw@mail.gmail.com/2-v3-0002-amcheck-Refactoring-the-storage-of-the-last-visib.patch)
download | inline diff:
From 7014fa433ba5c3c6ea645b13d433a11f4b5a0b15 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Fri, 10 May 2024 03:08:07 +0300
Subject: [PATCH v3 2/4] amcheck: Refactoring the storage of the last visible
entry
This commit introduces a new data structure BtreeLastVisibleEntry comprising
information about the last visible heap entry with the current value of key.
Usage of this data structure allows us to avoid passing all this information
as individual function arguments.
Reported-by: Alexander Korotkov
Discussion: https://www.postgresql.org/message-id/CAPpHfdsVbB9ToriaB1UHuOKwjKxiZmTFQcEF%3DjuzzC_nby31uA%40mail.gmail.com
Author: Pavel Borisov, Alexander Korotkov
---
contrib/amcheck/verify_nbtree.c | 125 +++++++++++++++----------------
src/tools/pgindent/typedefs.list | 1 +
2 files changed, 61 insertions(+), 65 deletions(-)
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index c7be785f88b..b433cb33254 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -145,6 +145,19 @@ typedef struct BtreeLevel
bool istruerootlevel;
} BtreeLevel;
+/*
+ * Information about the last visible entry with current B-tree key. Used
+ * for validation of the unique constraint.
+ */
+typedef struct BtreeLastVisibleEntry
+{
+ BlockNumber blkno; /* Index block */
+ OffsetNumber offset; /* Offset on index block */
+ int postingIndex; /* Number in the posting list (-1 for
+ * non-deduplicated tuples) */
+ ItemPointer tid; /* Heap tid */
+} BtreeLastVisibleEntry;
+
PG_FUNCTION_INFO_V1(bt_index_check);
PG_FUNCTION_INFO_V1(bt_index_parent_check);
@@ -165,17 +178,13 @@ static void bt_recheck_sibling_links(BtreeCheckState *state,
BlockNumber btpo_prev_from_target,
BlockNumber leftcurrent);
static bool heap_entry_is_visible(BtreeCheckState *state, ItemPointer tid);
-static void bt_report_duplicate(BtreeCheckState *state, ItemPointer tid,
- BlockNumber block, OffsetNumber offset,
- int posting, ItemPointer nexttid,
+static void bt_report_duplicate(BtreeCheckState *state, BtreeLastVisibleEntry *lVis,
+ ItemPointer nexttid,
BlockNumber nblock, OffsetNumber noffset,
int nposting);
static void bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup,
BlockNumber targetblock,
- OffsetNumber offset, int *lVis_i,
- ItemPointer *lVis_tid,
- OffsetNumber *lVis_offset,
- BlockNumber *lVis_block);
+ OffsetNumber offset, BtreeLastVisibleEntry *lVis);
static void bt_target_page_check(BtreeCheckState *state);
static BTScanInsert bt_right_page_check_scankey(BtreeCheckState *state,
OffsetNumber *rightfirstoffset);
@@ -997,8 +1006,7 @@ heap_entry_is_visible(BtreeCheckState *state, ItemPointer tid)
*/
static void
bt_report_duplicate(BtreeCheckState *state,
- ItemPointer tid, BlockNumber block, OffsetNumber offset,
- int posting,
+ BtreeLastVisibleEntry *lVis,
ItemPointer nexttid, BlockNumber nblock, OffsetNumber noffset,
int nposting)
{
@@ -1010,18 +1018,18 @@ bt_report_duplicate(BtreeCheckState *state,
*pnposting = "";
htid = psprintf("tid=(%u,%u)",
- ItemPointerGetBlockNumberNoCheck(tid),
- ItemPointerGetOffsetNumberNoCheck(tid));
+ ItemPointerGetBlockNumberNoCheck(lVis->tid),
+ ItemPointerGetOffsetNumberNoCheck(lVis->tid));
nhtid = psprintf("tid=(%u,%u)",
ItemPointerGetBlockNumberNoCheck(nexttid),
ItemPointerGetOffsetNumberNoCheck(nexttid));
- itid = psprintf("tid=(%u,%u)", block, offset);
+ itid = psprintf("tid=(%u,%u)", lVis->blkno, lVis->offset);
- if (nblock != block || noffset != offset)
+ if (nblock != lVis->blkno || noffset != lVis->offset)
nitid = psprintf(" tid=(%u,%u)", nblock, noffset);
- if (posting >= 0)
- pposting = psprintf(" posting %u", posting);
+ if (lVis->postingIndex >= 0)
+ pposting = psprintf(" posting %u", lVis->postingIndex);
if (nposting >= 0)
pnposting = psprintf(" posting %u", nposting);
@@ -1038,9 +1046,7 @@ bt_report_duplicate(BtreeCheckState *state,
/* Check if current nbtree leaf entry complies with UNIQUE constraint */
static void
bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup,
- BlockNumber targetblock, OffsetNumber offset, int *lVis_i,
- ItemPointer *lVis_tid, OffsetNumber *lVis_offset,
- BlockNumber *lVis_block)
+ BlockNumber targetblock, OffsetNumber offset, BtreeLastVisibleEntry *lVis)
{
ItemPointer tid;
bool has_visible_entry = false;
@@ -1049,7 +1055,7 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup,
/*
* Current tuple has posting list. Report duplicate if TID of any posting
- * list entry is visible and lVis_tid is valid.
+ * list entry is visible and lVis->tid is valid.
*/
if (BTreeTupleIsPosting(itup))
{
@@ -1059,11 +1065,10 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup,
if (heap_entry_is_visible(state, tid))
{
has_visible_entry = true;
- if (ItemPointerIsValid(*lVis_tid))
+ if (ItemPointerIsValid(lVis->tid))
{
bt_report_duplicate(state,
- *lVis_tid, *lVis_block,
- *lVis_offset, *lVis_i,
+ lVis,
tid, targetblock,
offset, i);
}
@@ -1073,13 +1078,13 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup,
* between the posting list entries of the first tuple on the
* page after cross-page check.
*/
- if (*lVis_block != targetblock && ItemPointerIsValid(*lVis_tid))
+ if (lVis->blkno != targetblock && ItemPointerIsValid(lVis->tid))
return;
- *lVis_i = i;
- *lVis_tid = tid;
- *lVis_offset = offset;
- *lVis_block = targetblock;
+ lVis->blkno = targetblock;
+ lVis->offset = offset;
+ lVis->postingIndex = i;
+ lVis->tid = tid;
}
}
}
@@ -1087,7 +1092,7 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup,
/*
* Current tuple has no posting list. If TID is visible save info about it
* for the next comparisons in the loop in bt_target_page_check(). Report
- * duplicate if lVis_tid is already valid.
+ * duplicate if lVis->tid is already valid.
*/
else
{
@@ -1095,37 +1100,38 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup,
if (heap_entry_is_visible(state, tid))
{
has_visible_entry = true;
- if (ItemPointerIsValid(*lVis_tid))
+ if (ItemPointerIsValid(lVis->tid))
{
bt_report_duplicate(state,
- *lVis_tid, *lVis_block,
- *lVis_offset, *lVis_i,
+ lVis,
tid, targetblock,
offset, -1);
}
- *lVis_i = -1;
- *lVis_tid = tid;
- *lVis_offset = offset;
- *lVis_block = targetblock;
+
+ lVis->blkno = targetblock;
+ lVis->offset = offset;
+ lVis->tid = tid;
+ lVis->postingIndex = -1;
}
}
- if (!has_visible_entry && *lVis_block != InvalidBlockNumber &&
- *lVis_block != targetblock)
+ if (!has_visible_entry &&
+ lVis->blkno != InvalidBlockNumber &&
+ lVis->blkno != targetblock)
{
char *posting = "";
- if (*lVis_i >= 0)
- posting = psprintf(" posting %u", *lVis_i);
+ if (lVis->postingIndex >= 0)
+ posting = psprintf(" posting %u", lVis->postingIndex);
ereport(DEBUG1,
(errcode(ERRCODE_NO_DATA),
errmsg("index uniqueness can not be checked for index tid=(%u,%u) in index \"%s\"",
targetblock, offset,
RelationGetRelationName(state->rel)),
errdetail("It doesn't have visible heap tids and key is equal to the tid=(%u,%u)%s (points to heap tid=(%u,%u)).",
- *lVis_block, *lVis_offset, posting,
- ItemPointerGetBlockNumberNoCheck(*lVis_tid),
- ItemPointerGetOffsetNumberNoCheck(*lVis_tid)),
+ lVis->blkno, lVis->offset, posting,
+ ItemPointerGetBlockNumberNoCheck(lVis->tid),
+ ItemPointerGetOffsetNumberNoCheck(lVis->tid)),
errhint("VACUUM the table and repeat the check.")));
}
}
@@ -1372,12 +1378,8 @@ bt_target_page_check(BtreeCheckState *state)
OffsetNumber max;
BTPageOpaque topaque;
- /* last visible entry info for checking indexes with unique constraint */
- int lVis_i = -1; /* the position of last visible item for
- * posting tuple. for non-posting tuple (-1) */
- ItemPointer lVis_tid = NULL;
- BlockNumber lVis_block = InvalidBlockNumber;
- OffsetNumber lVis_offset = InvalidOffsetNumber;
+ /* Last visible entry info for checking indexes with unique constraint */
+ BtreeLastVisibleEntry lVis = {InvalidBlockNumber, InvalidOffsetNumber, -1, NULL};
topaque = BTPageGetOpaque(state->target);
max = PageGetMaxOffsetNumber(state->target);
@@ -1784,11 +1786,9 @@ bt_target_page_check(BtreeCheckState *state)
*/
if (state->checkunique && state->indexinfo->ii_Unique &&
P_ISLEAF(topaque) && !skey->anynullkeys &&
- (BTreeTupleIsPosting(itup) || ItemPointerIsValid(lVis_tid)))
+ (BTreeTupleIsPosting(itup) || ItemPointerIsValid(lVis.tid)))
{
- bt_entry_unique_check(state, itup, state->targetblock, offset,
- &lVis_i, &lVis_tid, &lVis_offset,
- &lVis_block);
+ bt_entry_unique_check(state, itup, state->targetblock, offset, &lVis);
unique_checked = true;
}
@@ -1816,17 +1816,16 @@ bt_target_page_check(BtreeCheckState *state)
if (_bt_compare(state->rel, skey, state->target,
OffsetNumberNext(offset)) != 0 || skey->anynullkeys)
{
- lVis_i = -1;
- lVis_tid = NULL;
- lVis_block = InvalidBlockNumber;
- lVis_offset = InvalidOffsetNumber;
+ lVis.blkno = InvalidBlockNumber;
+ lVis.offset = InvalidOffsetNumber;
+ lVis.postingIndex = -1;
+ lVis.tid = NULL;
}
else if (!unique_checked)
{
- bt_entry_unique_check(state, itup, state->targetblock, offset,
- &lVis_i, &lVis_tid, &lVis_offset,
- &lVis_block);
+ bt_entry_unique_check(state, itup, state->targetblock, offset, &lVis);
}
+
skey->scantid = scantid; /* Restore saved scan key state */
}
@@ -1916,9 +1915,7 @@ bt_target_page_check(BtreeCheckState *state)
* postponed.
*/
if (!unique_checked)
- bt_entry_unique_check(state, itup, state->targetblock, offset,
- &lVis_i, &lVis_tid, &lVis_offset,
- &lVis_block);
+ bt_entry_unique_check(state, itup, state->targetblock, offset, &lVis);
elog(DEBUG2, "cross page equal keys");
state->target = palloc_btree_page(state,
@@ -1933,9 +1930,7 @@ bt_target_page_check(BtreeCheckState *state)
rightfirstoffset);
itup = (IndexTuple) PageGetItem(state->target, itemid);
- bt_entry_unique_check(state, itup, rightblock_number, rightfirstoffset,
- &lVis_i, &lVis_tid, &lVis_offset,
- &lVis_block);
+ bt_entry_unique_check(state, itup, rightblock_number, rightfirstoffset, &lVis);
}
}
}
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 34ec87a85eb..8d24418fe3c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -315,6 +315,7 @@ BrinStatsData
BrinTuple
BrinValues
BtreeCheckState
+BtreeLastVisibleEntry
BtreeLevel
Bucket
BufFile
--
2.39.3 (Apple Git-145)
[application/octet-stream] v3-0004-amcheck-Report-an-error-when-the-next-page-to-a-l.patch (2.5K, ../../CAPpHfdu6bGLxe-+9jfDwoji_T2iD6mjdiAGBr0JT-iX+TWNWOw@mail.gmail.com/3-v3-0004-amcheck-Report-an-error-when-the-next-page-to-a-l.patch)
download | inline diff:
From b4645780074cdb0a3abdb7c84435d6abad4e3bec Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Fri, 10 May 2024 03:07:22 +0300
Subject: [PATCH v3 4/4] amcheck: Report an error when the next page to a leaf
is not a leaf
This is a very unlikely condition during checking a B-tree unique constraint,
meaning that the index structure is violated badly, and we shouldn't continue
checking to avoid endless loops, etc. So it's worth immediately throwing an
error.
Reported-by: Peter Geoghegan
Discussion: https://postgr.es/m/CAH2-Wzk%2B2116uOXdOViA27SHcr31WKPgmjsxXLBs_aTxAeThzg%40mail.gmail.com
Author: Pavel Borisov
---
contrib/amcheck/verify_nbtree.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 977f8b6799d..e9bbc18c4a5 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -1849,7 +1849,6 @@ bt_target_page_check(BtreeCheckState *state)
if (offset == max)
{
BTScanInsert rightkey;
- BlockNumber rightblock_number;
/* first offset on a right index page (log only) */
OffsetNumber rightfirstoffset = InvalidOffsetNumber;
@@ -1894,10 +1893,11 @@ bt_target_page_check(BtreeCheckState *state)
* If index has unique constraint make sure that no more than one
* found equal items is visible.
*/
- rightblock_number = topaque->btpo_next;
if (state->checkunique && state->indexinfo->ii_Unique &&
- rightkey && P_ISLEAF(topaque) && rightblock_number != P_NONE)
+ rightkey && P_ISLEAF(topaque) && !P_RIGHTMOST(topaque))
{
+ BlockNumber rightblock_number = topaque->btpo_next;
+
elog(DEBUG2, "check cross page unique condition");
/*
@@ -1924,9 +1924,19 @@ bt_target_page_check(BtreeCheckState *state)
rightblock_number);
topaque = BTPageGetOpaque(rightpage);
- if (P_IGNORE(topaque) || !P_ISLEAF(topaque))
- break;
-
+ if (P_IGNORE(topaque))
+ {
+ if (unlikely(!P_ISLEAF(topaque)))
+ ereport(ERROR,
+ (errcode(ERRCODE_INDEX_CORRUPTED),
+ errmsg("right block of leaf block is non-leaf for index \"%s\"",
+ RelationGetRelationName(state->rel)),
+ errdetail_internal("Block=%u page lsn=%X/%X.",
+ state->targetblock,
+ LSN_FORMAT_ARGS(state->targetlsn))));
+ else
+ break;
+ }
itemid = PageGetItemIdCareful(state, rightblock_number,
rightpage,
rightfirstoffset);
--
2.39.3 (Apple Git-145)
[application/octet-stream] v3-0003-amcheck-Don-t-load-the-right-sibling-page-into-Bt.patch (2.8K, ../../CAPpHfdu6bGLxe-+9jfDwoji_T2iD6mjdiAGBr0JT-iX+TWNWOw@mail.gmail.com/4-v3-0003-amcheck-Don-t-load-the-right-sibling-page-into-Bt.patch)
download | inline diff:
From e9041f10384250472a54bd75a1cf28f89ec15e32 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Mon, 13 May 2024 04:18:56 +0300
Subject: [PATCH v3 3/4] amcheck: Don't load the right sibling page into
BtreeCheckState
5ae2087202 implemented a cross-page unique constraint check by loading
the right sibling to the BtreeCheckState.target variable. This is wrong,
because bt_target_page_check() shouldn't change the target page. Also,
BtreeCheckState.target shouldn't be changed alone without
BtreeCheckState.targetblock.
However, the above didn't cause any visible bugs for the following reasons.
1. We do a cross-page unique constraint check only for leaf index pages.
2. The only way target page get accessed after a cross-page unique constraint
check is loading of the lowkey.
3. The only place lowkey is used is bt_child_highkey_check(), and that applies
only to non-leafs.
The reasons above don't diminish the fact that changing BtreeCheckState.target
for a cross-page unique constraint check is wrong. This commit changes this
check to temporarily store the right sibling to the local variable.
Reported-by: Peter Geoghegan
Discussion: https://postgr.es/m/CAH2-Wzk%2B2116uOXdOViA27SHcr31WKPgmjsxXLBs_aTxAeThzg%40mail.gmail.com
Author: Pavel Borisov
---
contrib/amcheck/verify_nbtree.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index b433cb33254..977f8b6799d 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -1910,6 +1910,8 @@ bt_target_page_check(BtreeCheckState *state)
/* The first key on the next page is the same */
if (_bt_compare(state->rel, rightkey, state->target, max) == 0 && !rightkey->anynullkeys)
{
+ Page rightpage;
+
/*
* Do the bt_entry_unique_check() call if it was
* postponed.
@@ -1918,19 +1920,21 @@ bt_target_page_check(BtreeCheckState *state)
bt_entry_unique_check(state, itup, state->targetblock, offset, &lVis);
elog(DEBUG2, "cross page equal keys");
- state->target = palloc_btree_page(state,
- rightblock_number);
- topaque = BTPageGetOpaque(state->target);
+ rightpage = palloc_btree_page(state,
+ rightblock_number);
+ topaque = BTPageGetOpaque(rightpage);
if (P_IGNORE(topaque) || !P_ISLEAF(topaque))
break;
itemid = PageGetItemIdCareful(state, rightblock_number,
- state->target,
+ rightpage,
rightfirstoffset);
- itup = (IndexTuple) PageGetItem(state->target, itemid);
+ itup = (IndexTuple) PageGetItem(rightpage, itemid);
bt_entry_unique_check(state, itup, rightblock_number, rightfirstoffset, &lVis);
+
+ pfree(rightpage);
}
}
}
--
2.39.3 (Apple Git-145)
[application/octet-stream] v3-0001-amcheck-Optimize-speed-of-checking-for-unique-con.patch (3.8K, ../../CAPpHfdu6bGLxe-+9jfDwoji_T2iD6mjdiAGBr0JT-iX+TWNWOw@mail.gmail.com/5-v3-0001-amcheck-Optimize-speed-of-checking-for-unique-con.patch)
download | inline diff:
From f801783ade88e524b73acfa19f1b06e5b54608df Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Fri, 10 May 2024 03:07:53 +0300
Subject: [PATCH v3 1/4] amcheck: Optimize speed of checking for unique
constraint violation
Currently, when amcheck validates a unique constraint, it visits the heap for
each index tuple. This commit implements skipping keys, which have only one
non-dedeuplicated index tuple (quite common case for unique indexes). That
gives substantial economy on index checking time.
Reported-by: Noah Misch
Discussion: https://postgr.es/m/20240325020323.fd.nmisch%40google.com
Author: Alexander Korotkov, Pavel Borisov
---
contrib/amcheck/verify_nbtree.c | 35 +++++++++++++++++++++++++++++++--
1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 70f65b645a6..c7be785f88b 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -1429,6 +1429,13 @@ bt_target_page_check(BtreeCheckState *state)
bool lowersizelimit;
ItemPointer scantid;
+ /*
+ * True if we already called bt_entry_unique_check() for the current
+ * item. This helps to avoid visiting the heap for keys, which are
+ * anyway presented only once and can't comprise a unique violation.
+ */
+ bool unique_checked = false;
+
CHECK_FOR_INTERRUPTS();
itemid = PageGetItemIdCareful(state, state->targetblock,
@@ -1771,13 +1778,19 @@ bt_target_page_check(BtreeCheckState *state)
/*
* If the index is unique verify entries uniqueness by checking the
- * heap tuples visibility.
+ * heap tuples visibility. Immediately check posting tuples and
+ * tuples with repeated keys. Postpone check for keys, which have the
+ * first appearance.
*/
if (state->checkunique && state->indexinfo->ii_Unique &&
- P_ISLEAF(topaque) && !skey->anynullkeys)
+ P_ISLEAF(topaque) && !skey->anynullkeys &&
+ (BTreeTupleIsPosting(itup) || ItemPointerIsValid(lVis_tid)))
+ {
bt_entry_unique_check(state, itup, state->targetblock, offset,
&lVis_i, &lVis_tid, &lVis_offset,
&lVis_block);
+ unique_checked = true;
+ }
if (state->checkunique && state->indexinfo->ii_Unique &&
P_ISLEAF(topaque) && OffsetNumberNext(offset) <= max)
@@ -1796,6 +1809,9 @@ bt_target_page_check(BtreeCheckState *state)
* data (whole index tuple or last posting in index tuple). Key
* containing null value does not violate unique constraint and
* treated as different to any other key.
+ *
+ * If the next key is the same as the previous one, do the
+ * bt_entry_unique_check() call if it was postponed.
*/
if (_bt_compare(state->rel, skey, state->target,
OffsetNumberNext(offset)) != 0 || skey->anynullkeys)
@@ -1805,6 +1821,12 @@ bt_target_page_check(BtreeCheckState *state)
lVis_block = InvalidBlockNumber;
lVis_offset = InvalidOffsetNumber;
}
+ else if (!unique_checked)
+ {
+ bt_entry_unique_check(state, itup, state->targetblock, offset,
+ &lVis_i, &lVis_tid, &lVis_offset,
+ &lVis_block);
+ }
skey->scantid = scantid; /* Restore saved scan key state */
}
@@ -1889,6 +1911,15 @@ bt_target_page_check(BtreeCheckState *state)
/* The first key on the next page is the same */
if (_bt_compare(state->rel, rightkey, state->target, max) == 0 && !rightkey->anynullkeys)
{
+ /*
+ * Do the bt_entry_unique_check() call if it was
+ * postponed.
+ */
+ if (!unique_checked)
+ bt_entry_unique_check(state, itup, state->targetblock, offset,
+ &lVis_i, &lVis_tid, &lVis_offset,
+ &lVis_block);
+
elog(DEBUG2, "cross page equal keys");
state->target = palloc_btree_page(state,
rightblock_number);
--
2.39.3 (Apple Git-145)
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-13 11:55 Pavel Borisov <[email protected]>
parent: Alexander Korotkov <[email protected]>
1 sibling, 1 reply; 28+ messages in thread
From: Pavel Borisov @ 2024-05-13 11:55 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Mark Dilger <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
Hi, Alexander!
On Mon, 13 May 2024 at 05:42, Alexander Korotkov <[email protected]>
wrote:
> On Mon, May 13, 2024 at 12:23 AM Alexander Korotkov
> <[email protected]> wrote:
> > On Sat, May 11, 2024 at 4:13 AM Mark Dilger
> > <[email protected]> wrote:
> > > > On May 10, 2024, at 12:05 PM, Alexander Korotkov <
> [email protected]> wrote:
> > > > The only bt_target_page_check() caller is
> > > > bt_check_level_from_leftmost(), which overrides state->target in the
> > > > next iteration anyway. I think the patch is just refactoring to
> > > > eliminate the confusion pointer by Peter Geoghegan upthread.
> > >
> > > I find your argument unconvincing.
> > >
> > > After bt_target_page_check() returns at line 919, and before
> bt_check_level_from_leftmost() overrides state->target in the next
> iteration, bt_check_level_from_leftmost() conditionally fetches an item
> from the page referenced by state->target. See line 963.
> > >
> > > I'm left with four possibilities:
> > >
> > >
> > > 1) bt_target_page_check() never gets to the code that uses
> "rightpage" rather than "state->target" in the same iteration where
> bt_check_level_from_leftmost() conditionally fetches an item from
> state->target, so the change you're making doesn't matter.
> > >
> > > 2) The code prior to v2-0003 was wrong, having changed state->target
> in an inappropriate way, causing the wrong thing to happen at what is now
> line 963. The patch fixes the bug, because state->target no longer gets
> overwritten where you are now using "rightpage" for the value.
> > >
> > > 3) The code used to work, having set up state->target correctly in
> the place where you are now using "rightpage", but v2-0003 has broken that.
> > >
> > > 4) It's been broken all along and your patch just changes from wrong
> to wrong.
> > >
> > >
> > > If you believe (1) is true, then I'm complaining that you are relying
> far to much on action at a distance, and that you are not documenting it.
> Even with documentation of this interrelationship, I'd be unhappy with how
> brittle the code is. I cannot easily discern that the two don't ever
> happen in the same iteration, and I'm not at all convinced one way or the
> other. I tried to set up some Asserts about that, but none of the test
> cases actually reach the new code, so adding Asserts doesn't help to
> investigate the question.
> > >
> > > If (2) is true, then I'm complaining that the commit message doesn't
> mention the fact that this is a bug fix. Bug fixes should be clearly
> documented as such, otherwise future work might assume the commit can be
> reverted with only stylistic consequences.
> > >
> > > If (3) is true, then I'm complaining that the patch is flat busted.
> > >
> > > If (4) is true, then maybe we should revert the entire feature, or
> have a discussion of mitigation efforts that are needed.
> > >
> > > Regardless of which of 1..4 you pick, I think it could all do with
> more regression test coverage.
> > >
> > >
> > > For reference, I said something similar earlier today in another email
> to this thread:
> > >
> > > This patch introduces a change that stores a new page into variable
> "rightpage" rather than overwriting "state->target", which the old
> implementation most certainly did. That means that after returning from
> bt_target_page_check() into the calling function
> bt_check_level_from_leftmost() the value in state->target is not what it
> would have been prior to this patch. Now, that'd be irrelevant if nobody
> goes on to consult that value, but just 44 lines further down in
> bt_check_level_from_leftmost() state->target is clearly used. So the
> behavior at that point is changing between the old and new versions of the
> code, and I think I'm within reason to ask if it was wrong before the
> patch, wrong after the patch, or something else? Is this a bug being
> introduced, being fixed, or ... ?
> >
> > Thank you for your analysis. I'm inclined to believe in 2, but not
> > yet completely sure. It's really pity that our tests don't cover
> > this. I'm investigating this area.
>
> It seems that I got to the bottom of this. Changing
> BtreeCheckState.target for a cross-page unique constraint check is
> wrong, but that happens only for leaf pages. After that
> BtreeCheckState.target is only used for setting the low key. The low
> key is only used for non-leaf pages. So, that didn't lead to any
> visible bug. I've revised the commit message to reflect this.
>
I agree with your analysis regarding state->target:
- when the unique check is on, state->target was reassigned only for the
leaf pages (under P_ISLEAF(topaque) in bt_target_page_check).
- in this level (leaf) in bt_check_level_from_leftmost() this value of
state->target was used to get state->lowkey. Then it was reset (in the next
iteration of do loop in in bt_check_level_from_leftmost()
- state->lowkey lives until the end of pages level (leaf) iteration cycle.
Then, low-key is reset (state->lowkey = NULL in the end of
bt_check_level_from_leftmost())
- state->lowkey is used only in bt_child_check/bt_child_highkey_check. Both
are called only from non-leaf pages iteration cycles (under
P_ISLEAF(topaque))
- Also there is a check (rightblock_number != P_NONE) in before getting
rightpage into state->target in bt_target_page_check() that ensures us that
rightpage indeed exists and getting this (unused) lowkey in
bt_check_level_from_leftmost will not invoke any page reading errors.
I'm pretty sure that there was no bug in this, not just the bug was hidden.
Indeed re-assigning state->target in leaf page iteration for cross-page
unique check was not beautiful, and Peter pointed out this. In my opinion
the patch 0003 is a pure code refactoring.
As for the cross-page check regression/TAP testing, this test had problems
since the btree page layout is not fixed (especially it's different on
32-bit arch). I had a variant for testing cross-page check when the test
was yet regression one upthread for both 32/64 bit architectures. I
remember it was decided not to include it due to complications and low
impact for testing the corner case of very rare cross-page duplicates.
(There were also suggestions to drop cross-page duplicates check at all,
which I didn't agree 2 years ago, but still it can make sense)
Separately, I propose to avoid getting state->lowkey for leaf pages at all
as it's unused. PFA is a simple patch for this. (I don't add it to the
current patch set as I believe it has nothing to do with UNIQUE constraint
check, rather it improves the previous btree amcheck code)
Best regards,
Pavel Borisov,
Supabase
Attachments:
[application/octet-stream] XXXX-amcheck-Get-lowkey-only-for-internal-pages-of-btree-.patch (1.5K, ../../CALT9ZEGfH_+5Z057KEGpnKE6FE7gssOMRbSvQOkN3G-bYVcmhA@mail.gmail.com/3-XXXX-amcheck-Get-lowkey-only-for-internal-pages-of-btree-.patch)
download | inline diff:
From 9e3903fd2497c967aa001010b20167363963017a Mon Sep 17 00:00:00 2001
From: Pavel Borisov <[email protected]>
Date: Mon, 13 May 2024 15:19:16 +0400
Subject: [PATCH] amcheck: Get lowkey only for internal pages of btree index
state->lowkey is used only for checking child pages in btree index inside
bt_child_check()/bt_child_highkey_check() All calls of these functions
possible only from internal pages level. So there is no reason of getting
page lowkey for leaf pages just to invalidate it next without actual usage.
---
contrib/amcheck/verify_nbtree.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 70f65b645a..8e18a19deb 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -938,13 +938,13 @@ nextpage:
* falls to the boundary of pages on the target level. See
* bt_child_highkey_check() for details. So, typically we won't end
* up doing anything with low key, but it's simpler for general case
- * high key verification to always have it available.
+ * high key verification to have it available for all non-leaf pages.
*
* The correctness of managing low key in the case of concurrent
* splits wasn't investigated yet. Thankfully we only need low key
* for readonly verification and concurrent splits won't happen.
*/
- if (state->readonly && !P_RIGHTMOST(opaque))
+ if (state->readonly && !P_RIGHTMOST(opaque) && !P_ISLEAF(opaque))
{
IndexTuple itup;
ItemId itemid;
--
2.34.1
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-13 12:19 Pavel Borisov <[email protected]>
parent: Pavel Borisov <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Pavel Borisov @ 2024-05-13 12:19 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Mark Dilger <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
On Mon, 13 May 2024 at 15:55, Pavel Borisov <[email protected]> wrote:
> Hi, Alexander!
>
> On Mon, 13 May 2024 at 05:42, Alexander Korotkov <[email protected]>
> wrote:
>
>> On Mon, May 13, 2024 at 12:23 AM Alexander Korotkov
>> <[email protected]> wrote:
>> > On Sat, May 11, 2024 at 4:13 AM Mark Dilger
>> > <[email protected]> wrote:
>> > > > On May 10, 2024, at 12:05 PM, Alexander Korotkov <
>> [email protected]> wrote:
>> > > > The only bt_target_page_check() caller is
>> > > > bt_check_level_from_leftmost(), which overrides state->target in the
>> > > > next iteration anyway. I think the patch is just refactoring to
>> > > > eliminate the confusion pointer by Peter Geoghegan upthread.
>> > >
>> > > I find your argument unconvincing.
>> > >
>> > > After bt_target_page_check() returns at line 919, and before
>> bt_check_level_from_leftmost() overrides state->target in the next
>> iteration, bt_check_level_from_leftmost() conditionally fetches an item
>> from the page referenced by state->target. See line 963.
>> > >
>> > > I'm left with four possibilities:
>> > >
>> > >
>> > > 1) bt_target_page_check() never gets to the code that uses
>> "rightpage" rather than "state->target" in the same iteration where
>> bt_check_level_from_leftmost() conditionally fetches an item from
>> state->target, so the change you're making doesn't matter.
>> > >
>> > > 2) The code prior to v2-0003 was wrong, having changed state->target
>> in an inappropriate way, causing the wrong thing to happen at what is now
>> line 963. The patch fixes the bug, because state->target no longer gets
>> overwritten where you are now using "rightpage" for the value.
>> > >
>> > > 3) The code used to work, having set up state->target correctly in
>> the place where you are now using "rightpage", but v2-0003 has broken that.
>> > >
>> > > 4) It's been broken all along and your patch just changes from wrong
>> to wrong.
>> > >
>> > >
>> > > If you believe (1) is true, then I'm complaining that you are relying
>> far to much on action at a distance, and that you are not documenting it.
>> Even with documentation of this interrelationship, I'd be unhappy with how
>> brittle the code is. I cannot easily discern that the two don't ever
>> happen in the same iteration, and I'm not at all convinced one way or the
>> other. I tried to set up some Asserts about that, but none of the test
>> cases actually reach the new code, so adding Asserts doesn't help to
>> investigate the question.
>> > >
>> > > If (2) is true, then I'm complaining that the commit message doesn't
>> mention the fact that this is a bug fix. Bug fixes should be clearly
>> documented as such, otherwise future work might assume the commit can be
>> reverted with only stylistic consequences.
>> > >
>> > > If (3) is true, then I'm complaining that the patch is flat busted.
>> > >
>> > > If (4) is true, then maybe we should revert the entire feature, or
>> have a discussion of mitigation efforts that are needed.
>> > >
>> > > Regardless of which of 1..4 you pick, I think it could all do with
>> more regression test coverage.
>> > >
>> > >
>> > > For reference, I said something similar earlier today in another
>> email to this thread:
>> > >
>> > > This patch introduces a change that stores a new page into variable
>> "rightpage" rather than overwriting "state->target", which the old
>> implementation most certainly did. That means that after returning from
>> bt_target_page_check() into the calling function
>> bt_check_level_from_leftmost() the value in state->target is not what it
>> would have been prior to this patch. Now, that'd be irrelevant if nobody
>> goes on to consult that value, but just 44 lines further down in
>> bt_check_level_from_leftmost() state->target is clearly used. So the
>> behavior at that point is changing between the old and new versions of the
>> code, and I think I'm within reason to ask if it was wrong before the
>> patch, wrong after the patch, or something else? Is this a bug being
>> introduced, being fixed, or ... ?
>> >
>> > Thank you for your analysis. I'm inclined to believe in 2, but not
>> > yet completely sure. It's really pity that our tests don't cover
>> > this. I'm investigating this area.
>>
>> It seems that I got to the bottom of this. Changing
>> BtreeCheckState.target for a cross-page unique constraint check is
>> wrong, but that happens only for leaf pages. After that
>> BtreeCheckState.target is only used for setting the low key. The low
>> key is only used for non-leaf pages. So, that didn't lead to any
>> visible bug. I've revised the commit message to reflect this.
>>
>
> I agree with your analysis regarding state->target:
> - when the unique check is on, state->target was reassigned only for the
> leaf pages (under P_ISLEAF(topaque) in bt_target_page_check).
> - in this level (leaf) in bt_check_level_from_leftmost() this value of
> state->target was used to get state->lowkey. Then it was reset (in the next
> iteration of do loop in in bt_check_level_from_leftmost()
> - state->lowkey lives until the end of pages level (leaf) iteration cycle.
> Then, low-key is reset (state->lowkey = NULL in the end of
> bt_check_level_from_leftmost())
> - state->lowkey is used only in bt_child_check/bt_child_highkey_check.
> Both are called only from non-leaf pages iteration cycles (under
> P_ISLEAF(topaque))
> - Also there is a check (rightblock_number != P_NONE) in before getting
> rightpage into state->target in bt_target_page_check() that ensures us that
> rightpage indeed exists and getting this (unused) lowkey in
> bt_check_level_from_leftmost will not invoke any page reading errors.
>
> I'm pretty sure that there was no bug in this, not just the bug was hidden.
>
> Indeed re-assigning state->target in leaf page iteration for cross-page
> unique check was not beautiful, and Peter pointed out this. In my opinion
> the patch 0003 is a pure code refactoring.
>
> As for the cross-page check regression/TAP testing, this test had problems
> since the btree page layout is not fixed (especially it's different on
> 32-bit arch). I had a variant for testing cross-page check when the test
> was yet regression one upthread for both 32/64 bit architectures. I
> remember it was decided not to include it due to complications and low
> impact for testing the corner case of very rare cross-page duplicates.
> (There were also suggestions to drop cross-page duplicates check at all,
> which I didn't agree 2 years ago, but still it can make sense)
>
> Separately, I propose to avoid getting state->lowkey for leaf pages at all
> as it's unused. PFA is a simple patch for this. (I don't add it to the
> current patch set as I believe it has nothing to do with UNIQUE constraint
> check, rather it improves the previous btree amcheck code)
>
A correction of a typo in previous message:
non-leaf pages iteration cycles (under !P_ISLEAF(topaque)) -> non-leaf
pages iteration cycles (under !P_ISLEAF(topaque))
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-13 12:20 Pavel Borisov <[email protected]>
parent: Pavel Borisov <[email protected]>
0 siblings, 0 replies; 28+ messages in thread
From: Pavel Borisov @ 2024-05-13 12:20 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Mark Dilger <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
A correction of a typo in previous message:
non-leaf pages iteration cycles (under P_ISLEAF(topaque)) -> non-leaf pages
iteration cycles (under !P_ISLEAF(topaque))
On Mon, 13 May 2024 at 16:19, Pavel Borisov <[email protected]> wrote:
>
>
> On Mon, 13 May 2024 at 15:55, Pavel Borisov <[email protected]>
> wrote:
>
>> Hi, Alexander!
>>
>> On Mon, 13 May 2024 at 05:42, Alexander Korotkov <[email protected]>
>> wrote:
>>
>>> On Mon, May 13, 2024 at 12:23 AM Alexander Korotkov
>>> <[email protected]> wrote:
>>> > On Sat, May 11, 2024 at 4:13 AM Mark Dilger
>>> > <[email protected]> wrote:
>>> > > > On May 10, 2024, at 12:05 PM, Alexander Korotkov <
>>> [email protected]> wrote:
>>> > > > The only bt_target_page_check() caller is
>>> > > > bt_check_level_from_leftmost(), which overrides state->target in
>>> the
>>> > > > next iteration anyway. I think the patch is just refactoring to
>>> > > > eliminate the confusion pointer by Peter Geoghegan upthread.
>>> > >
>>> > > I find your argument unconvincing.
>>> > >
>>> > > After bt_target_page_check() returns at line 919, and before
>>> bt_check_level_from_leftmost() overrides state->target in the next
>>> iteration, bt_check_level_from_leftmost() conditionally fetches an item
>>> from the page referenced by state->target. See line 963.
>>> > >
>>> > > I'm left with four possibilities:
>>> > >
>>> > >
>>> > > 1) bt_target_page_check() never gets to the code that uses
>>> "rightpage" rather than "state->target" in the same iteration where
>>> bt_check_level_from_leftmost() conditionally fetches an item from
>>> state->target, so the change you're making doesn't matter.
>>> > >
>>> > > 2) The code prior to v2-0003 was wrong, having changed
>>> state->target in an inappropriate way, causing the wrong thing to happen at
>>> what is now line 963. The patch fixes the bug, because state->target no
>>> longer gets overwritten where you are now using "rightpage" for the value.
>>> > >
>>> > > 3) The code used to work, having set up state->target correctly in
>>> the place where you are now using "rightpage", but v2-0003 has broken that.
>>> > >
>>> > > 4) It's been broken all along and your patch just changes from
>>> wrong to wrong.
>>> > >
>>> > >
>>> > > If you believe (1) is true, then I'm complaining that you are
>>> relying far to much on action at a distance, and that you are not
>>> documenting it. Even with documentation of this interrelationship, I'd be
>>> unhappy with how brittle the code is. I cannot easily discern that the two
>>> don't ever happen in the same iteration, and I'm not at all convinced one
>>> way or the other. I tried to set up some Asserts about that, but none of
>>> the test cases actually reach the new code, so adding Asserts doesn't help
>>> to investigate the question.
>>> > >
>>> > > If (2) is true, then I'm complaining that the commit message doesn't
>>> mention the fact that this is a bug fix. Bug fixes should be clearly
>>> documented as such, otherwise future work might assume the commit can be
>>> reverted with only stylistic consequences.
>>> > >
>>> > > If (3) is true, then I'm complaining that the patch is flat busted.
>>> > >
>>> > > If (4) is true, then maybe we should revert the entire feature, or
>>> have a discussion of mitigation efforts that are needed.
>>> > >
>>> > > Regardless of which of 1..4 you pick, I think it could all do with
>>> more regression test coverage.
>>> > >
>>> > >
>>> > > For reference, I said something similar earlier today in another
>>> email to this thread:
>>> > >
>>> > > This patch introduces a change that stores a new page into variable
>>> "rightpage" rather than overwriting "state->target", which the old
>>> implementation most certainly did. That means that after returning from
>>> bt_target_page_check() into the calling function
>>> bt_check_level_from_leftmost() the value in state->target is not what it
>>> would have been prior to this patch. Now, that'd be irrelevant if nobody
>>> goes on to consult that value, but just 44 lines further down in
>>> bt_check_level_from_leftmost() state->target is clearly used. So the
>>> behavior at that point is changing between the old and new versions of the
>>> code, and I think I'm within reason to ask if it was wrong before the
>>> patch, wrong after the patch, or something else? Is this a bug being
>>> introduced, being fixed, or ... ?
>>> >
>>> > Thank you for your analysis. I'm inclined to believe in 2, but not
>>> > yet completely sure. It's really pity that our tests don't cover
>>> > this. I'm investigating this area.
>>>
>>> It seems that I got to the bottom of this. Changing
>>> BtreeCheckState.target for a cross-page unique constraint check is
>>> wrong, but that happens only for leaf pages. After that
>>> BtreeCheckState.target is only used for setting the low key. The low
>>> key is only used for non-leaf pages. So, that didn't lead to any
>>> visible bug. I've revised the commit message to reflect this.
>>>
>>
>> I agree with your analysis regarding state->target:
>> - when the unique check is on, state->target was reassigned only for the
>> leaf pages (under P_ISLEAF(topaque) in bt_target_page_check).
>> - in this level (leaf) in bt_check_level_from_leftmost() this value of
>> state->target was used to get state->lowkey. Then it was reset (in the next
>> iteration of do loop in in bt_check_level_from_leftmost()
>> - state->lowkey lives until the end of pages level (leaf) iteration
>> cycle. Then, low-key is reset (state->lowkey = NULL in the end of
>> bt_check_level_from_leftmost())
>> - state->lowkey is used only in bt_child_check/bt_child_highkey_check.
>> Both are called only from non-leaf pages iteration cycles (under
>> P_ISLEAF(topaque))
>> - Also there is a check (rightblock_number != P_NONE) in before getting
>> rightpage into state->target in bt_target_page_check() that ensures us that
>> rightpage indeed exists and getting this (unused) lowkey in
>> bt_check_level_from_leftmost will not invoke any page reading errors.
>>
>> I'm pretty sure that there was no bug in this, not just the bug was
>> hidden.
>>
>> Indeed re-assigning state->target in leaf page iteration for cross-page
>> unique check was not beautiful, and Peter pointed out this. In my opinion
>> the patch 0003 is a pure code refactoring.
>>
>> As for the cross-page check regression/TAP testing, this test had
>> problems since the btree page layout is not fixed (especially it's
>> different on 32-bit arch). I had a variant for testing cross-page check
>> when the test was yet regression one upthread for both 32/64 bit
>> architectures. I remember it was decided not to include it due to
>> complications and low impact for testing the corner case of very rare
>> cross-page duplicates. (There were also suggestions to drop cross-page
>> duplicates check at all, which I didn't agree 2 years ago, but still it can
>> make sense)
>>
>> Separately, I propose to avoid getting state->lowkey for leaf pages at
>> all as it's unused. PFA is a simple patch for this. (I don't add it to the
>> current patch set as I believe it has nothing to do with UNIQUE constraint
>> check, rather it improves the previous btree amcheck code)
>>
>
> A correction of a typo in previous message:
> non-leaf pages iteration cycles (under !P_ISLEAF(topaque)) -> non-leaf
> pages iteration cycles (under !P_ISLEAF(topaque))
>
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-17 10:11 Alexander Korotkov <[email protected]>
parent: Alexander Korotkov <[email protected]>
1 sibling, 3 replies; 28+ messages in thread
From: Alexander Korotkov @ 2024-05-17 10:11 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
On Mon, May 13, 2024 at 4:42 AM Alexander Korotkov <[email protected]> wrote:
> On Mon, May 13, 2024 at 12:23 AM Alexander Korotkov
> <[email protected]> wrote:
> > On Sat, May 11, 2024 at 4:13 AM Mark Dilger
> > <[email protected]> wrote:
> > > > On May 10, 2024, at 12:05 PM, Alexander Korotkov <[email protected]> wrote:
> > > > The only bt_target_page_check() caller is
> > > > bt_check_level_from_leftmost(), which overrides state->target in the
> > > > next iteration anyway. I think the patch is just refactoring to
> > > > eliminate the confusion pointer by Peter Geoghegan upthread.
> > >
> > > I find your argument unconvincing.
> > >
> > > After bt_target_page_check() returns at line 919, and before bt_check_level_from_leftmost() overrides state->target in the next iteration, bt_check_level_from_leftmost() conditionally fetches an item from the page referenced by state->target. See line 963.
> > >
> > > I'm left with four possibilities:
> > >
> > >
> > > 1) bt_target_page_check() never gets to the code that uses "rightpage" rather than "state->target" in the same iteration where bt_check_level_from_leftmost() conditionally fetches an item from state->target, so the change you're making doesn't matter.
> > >
> > > 2) The code prior to v2-0003 was wrong, having changed state->target in an inappropriate way, causing the wrong thing to happen at what is now line 963. The patch fixes the bug, because state->target no longer gets overwritten where you are now using "rightpage" for the value.
> > >
> > > 3) The code used to work, having set up state->target correctly in the place where you are now using "rightpage", but v2-0003 has broken that.
> > >
> > > 4) It's been broken all along and your patch just changes from wrong to wrong.
> > >
> > >
> > > If you believe (1) is true, then I'm complaining that you are relying far to much on action at a distance, and that you are not documenting it. Even with documentation of this interrelationship, I'd be unhappy with how brittle the code is. I cannot easily discern that the two don't ever happen in the same iteration, and I'm not at all convinced one way or the other. I tried to set up some Asserts about that, but none of the test cases actually reach the new code, so adding Asserts doesn't help to investigate the question.
> > >
> > > If (2) is true, then I'm complaining that the commit message doesn't mention the fact that this is a bug fix. Bug fixes should be clearly documented as such, otherwise future work might assume the commit can be reverted with only stylistic consequences.
> > >
> > > If (3) is true, then I'm complaining that the patch is flat busted.
> > >
> > > If (4) is true, then maybe we should revert the entire feature, or have a discussion of mitigation efforts that are needed.
> > >
> > > Regardless of which of 1..4 you pick, I think it could all do with more regression test coverage.
> > >
> > >
> > > For reference, I said something similar earlier today in another email to this thread:
> > >
> > > This patch introduces a change that stores a new page into variable "rightpage" rather than overwriting "state->target", which the old implementation most certainly did. That means that after returning from bt_target_page_check() into the calling function bt_check_level_from_leftmost() the value in state->target is not what it would have been prior to this patch. Now, that'd be irrelevant if nobody goes on to consult that value, but just 44 lines further down in bt_check_level_from_leftmost() state->target is clearly used. So the behavior at that point is changing between the old and new versions of the code, and I think I'm within reason to ask if it was wrong before the patch, wrong after the patch, or something else? Is this a bug being introduced, being fixed, or ... ?
> >
> > Thank you for your analysis. I'm inclined to believe in 2, but not
> > yet completely sure. It's really pity that our tests don't cover
> > this. I'm investigating this area.
>
> It seems that I got to the bottom of this. Changing
> BtreeCheckState.target for a cross-page unique constraint check is
> wrong, but that happens only for leaf pages. After that
> BtreeCheckState.target is only used for setting the low key. The low
> key is only used for non-leaf pages. So, that didn't lead to any
> visible bug. I've revised the commit message to reflect this.
>
> So, the picture for the patches is the following now.
> 0001 – optimization, but rather simple and giving huge effect
> 0002 – refactoring
> 0003 – fix for the bug
> 0004 – better error reporting
I think the thread contains enough motivation on why 0002, 0003 and
0004 are material for post-FF. They are fixes and refactoring for
new-in-v17 feature. I'm going to push them if no objections.
Regarding 0001, I'd like to ask Tom and Mark if they find convincing
that given that optimization is small, simple and giving huge effect,
it could be pushed post-FF? Otherwise, this could wait for v18.
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-17 11:09 Pavel Borisov <[email protected]>
parent: Alexander Korotkov <[email protected]>
2 siblings, 0 replies; 28+ messages in thread
From: Pavel Borisov @ 2024-05-17 11:09 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Mark Dilger <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
Hi, Alexander!
On Fri, 17 May 2024 at 14:11, Alexander Korotkov <[email protected]>
wrote:
> On Mon, May 13, 2024 at 4:42 AM Alexander Korotkov <[email protected]>
> wrote:
> > On Mon, May 13, 2024 at 12:23 AM Alexander Korotkov
> > <[email protected]> wrote:
> > > On Sat, May 11, 2024 at 4:13 AM Mark Dilger
> > > <[email protected]> wrote:
> > > > > On May 10, 2024, at 12:05 PM, Alexander Korotkov <
> [email protected]> wrote:
> > > > > The only bt_target_page_check() caller is
> > > > > bt_check_level_from_leftmost(), which overrides state->target in
> the
> > > > > next iteration anyway. I think the patch is just refactoring to
> > > > > eliminate the confusion pointer by Peter Geoghegan upthread.
> > > >
> > > > I find your argument unconvincing.
> > > >
> > > > After bt_target_page_check() returns at line 919, and before
> bt_check_level_from_leftmost() overrides state->target in the next
> iteration, bt_check_level_from_leftmost() conditionally fetches an item
> from the page referenced by state->target. See line 963.
> > > >
> > > > I'm left with four possibilities:
> > > >
> > > >
> > > > 1) bt_target_page_check() never gets to the code that uses
> "rightpage" rather than "state->target" in the same iteration where
> bt_check_level_from_leftmost() conditionally fetches an item from
> state->target, so the change you're making doesn't matter.
> > > >
> > > > 2) The code prior to v2-0003 was wrong, having changed
> state->target in an inappropriate way, causing the wrong thing to happen at
> what is now line 963. The patch fixes the bug, because state->target no
> longer gets overwritten where you are now using "rightpage" for the value.
> > > >
> > > > 3) The code used to work, having set up state->target correctly in
> the place where you are now using "rightpage", but v2-0003 has broken that.
> > > >
> > > > 4) It's been broken all along and your patch just changes from
> wrong to wrong.
> > > >
> > > >
> > > > If you believe (1) is true, then I'm complaining that you are
> relying far to much on action at a distance, and that you are not
> documenting it. Even with documentation of this interrelationship, I'd be
> unhappy with how brittle the code is. I cannot easily discern that the two
> don't ever happen in the same iteration, and I'm not at all convinced one
> way or the other. I tried to set up some Asserts about that, but none of
> the test cases actually reach the new code, so adding Asserts doesn't help
> to investigate the question.
> > > >
> > > > If (2) is true, then I'm complaining that the commit message doesn't
> mention the fact that this is a bug fix. Bug fixes should be clearly
> documented as such, otherwise future work might assume the commit can be
> reverted with only stylistic consequences.
> > > >
> > > > If (3) is true, then I'm complaining that the patch is flat busted.
> > > >
> > > > If (4) is true, then maybe we should revert the entire feature, or
> have a discussion of mitigation efforts that are needed.
> > > >
> > > > Regardless of which of 1..4 you pick, I think it could all do with
> more regression test coverage.
> > > >
> > > >
> > > > For reference, I said something similar earlier today in another
> email to this thread:
> > > >
> > > > This patch introduces a change that stores a new page into variable
> "rightpage" rather than overwriting "state->target", which the old
> implementation most certainly did. That means that after returning from
> bt_target_page_check() into the calling function
> bt_check_level_from_leftmost() the value in state->target is not what it
> would have been prior to this patch. Now, that'd be irrelevant if nobody
> goes on to consult that value, but just 44 lines further down in
> bt_check_level_from_leftmost() state->target is clearly used. So the
> behavior at that point is changing between the old and new versions of the
> code, and I think I'm within reason to ask if it was wrong before the
> patch, wrong after the patch, or something else? Is this a bug being
> introduced, being fixed, or ... ?
> > >
> > > Thank you for your analysis. I'm inclined to believe in 2, but not
> > > yet completely sure. It's really pity that our tests don't cover
> > > this. I'm investigating this area.
> >
> > It seems that I got to the bottom of this. Changing
> > BtreeCheckState.target for a cross-page unique constraint check is
> > wrong, but that happens only for leaf pages. After that
> > BtreeCheckState.target is only used for setting the low key. The low
> > key is only used for non-leaf pages. So, that didn't lead to any
> > visible bug. I've revised the commit message to reflect this.
> >
> > So, the picture for the patches is the following now.
> > 0001 – optimization, but rather simple and giving huge effect
> > 0002 – refactoring
> > 0003 – fix for the bug
> > 0004 – better error reporting
>
> I think the thread contains enough motivation on why 0002, 0003 and
> 0004 are material for post-FF. They are fixes and refactoring for
> new-in-v17 feature. I'm going to push them if no objections.
>
> Regarding 0001, I'd like to ask Tom and Mark if they find convincing
> that given that optimization is small, simple and giving huge effect,
> it could be pushed post-FF? Otherwise, this could wait for v18.
>
In my view, patches 0002-0004 are worth pushing.
0001 is ready in my view. But I see no problem pushing it into v18
regarding that this optimization could be not eligible for post-FF. I don't
know the criteria for this just let's be safe about it.
Regards,
Pavel Borisov
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-17 17:41 Mark Dilger <[email protected]>
parent: Alexander Korotkov <[email protected]>
2 siblings, 1 reply; 28+ messages in thread
From: Mark Dilger @ 2024-05-17 17:41 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
> On May 17, 2024, at 3:11 AM, Alexander Korotkov <[email protected]> wrote:
>
> On Mon, May 13, 2024 at 4:42 AM Alexander Korotkov <[email protected]> wrote:
>> On Mon, May 13, 2024 at 12:23 AM Alexander Korotkov
>> <[email protected]> wrote:
>>> On Sat, May 11, 2024 at 4:13 AM Mark Dilger
>>> <[email protected]> wrote:
>>>>> On May 10, 2024, at 12:05 PM, Alexander Korotkov <[email protected]> wrote:
>>>>> The only bt_target_page_check() caller is
>>>>> bt_check_level_from_leftmost(), which overrides state->target in the
>>>>> next iteration anyway. I think the patch is just refactoring to
>>>>> eliminate the confusion pointer by Peter Geoghegan upthread.
>>>>
>>>> I find your argument unconvincing.
>>>>
>>>> After bt_target_page_check() returns at line 919, and before bt_check_level_from_leftmost() overrides state->target in the next iteration, bt_check_level_from_leftmost() conditionally fetches an item from the page referenced by state->target. See line 963.
>>>>
>>>> I'm left with four possibilities:
>>>>
>>>>
>>>> 1) bt_target_page_check() never gets to the code that uses "rightpage" rather than "state->target" in the same iteration where bt_check_level_from_leftmost() conditionally fetches an item from state->target, so the change you're making doesn't matter.
>>>>
>>>> 2) The code prior to v2-0003 was wrong, having changed state->target in an inappropriate way, causing the wrong thing to happen at what is now line 963. The patch fixes the bug, because state->target no longer gets overwritten where you are now using "rightpage" for the value.
>>>>
>>>> 3) The code used to work, having set up state->target correctly in the place where you are now using "rightpage", but v2-0003 has broken that.
>>>>
>>>> 4) It's been broken all along and your patch just changes from wrong to wrong.
>>>>
>>>>
>>>> If you believe (1) is true, then I'm complaining that you are relying far to much on action at a distance, and that you are not documenting it. Even with documentation of this interrelationship, I'd be unhappy with how brittle the code is. I cannot easily discern that the two don't ever happen in the same iteration, and I'm not at all convinced one way or the other. I tried to set up some Asserts about that, but none of the test cases actually reach the new code, so adding Asserts doesn't help to investigate the question.
>>>>
>>>> If (2) is true, then I'm complaining that the commit message doesn't mention the fact that this is a bug fix. Bug fixes should be clearly documented as such, otherwise future work might assume the commit can be reverted with only stylistic consequences.
>>>>
>>>> If (3) is true, then I'm complaining that the patch is flat busted.
>>>>
>>>> If (4) is true, then maybe we should revert the entire feature, or have a discussion of mitigation efforts that are needed.
>>>>
>>>> Regardless of which of 1..4 you pick, I think it could all do with more regression test coverage.
>>>>
>>>>
>>>> For reference, I said something similar earlier today in another email to this thread:
>>>>
>>>> This patch introduces a change that stores a new page into variable "rightpage" rather than overwriting "state->target", which the old implementation most certainly did. That means that after returning from bt_target_page_check() into the calling function bt_check_level_from_leftmost() the value in state->target is not what it would have been prior to this patch. Now, that'd be irrelevant if nobody goes on to consult that value, but just 44 lines further down in bt_check_level_from_leftmost() state->target is clearly used. So the behavior at that point is changing between the old and new versions of the code, and I think I'm within reason to ask if it was wrong before the patch, wrong after the patch, or something else? Is this a bug being introduced, being fixed, or ... ?
>>>
>>> Thank you for your analysis. I'm inclined to believe in 2, but not
>>> yet completely sure. It's really pity that our tests don't cover
>>> this. I'm investigating this area.
>>
>> It seems that I got to the bottom of this. Changing
>> BtreeCheckState.target for a cross-page unique constraint check is
>> wrong, but that happens only for leaf pages. After that
>> BtreeCheckState.target is only used for setting the low key. The low
>> key is only used for non-leaf pages. So, that didn't lead to any
>> visible bug. I've revised the commit message to reflect this.
>>
>> So, the picture for the patches is the following now.
>> 0001 – optimization, but rather simple and giving huge effect
>> 0002 – refactoring
>> 0003 – fix for the bug
>> 0004 – better error reporting
>
> I think the thread contains enough motivation on why 0002, 0003 and
> 0004 are material for post-FF. They are fixes and refactoring for
> new-in-v17 feature. I'm going to push them if no objections.
>
> Regarding 0001, I'd like to ask Tom and Mark if they find convincing
> that given that optimization is small, simple and giving huge effect,
> it could be pushed post-FF? Otherwise, this could wait for v18.
I won't pretend to be part of the Release Management Team. Perhaps Tom wishes to respond.
I wrote a TAP test to check the uniqueness checker. bt_index_check() sometimes fails to detect a corruption. This is true both before and after applying v3-0001. The bt_index_parent_check() seems to always detect the corruption created by the TAP test. Likewise, this is true both before and after applying v3-0001.
The documentation in https://www.postgresql.org/docs/devel/amcheck.html#AMCHECK-FUNCTIONS is ambiguous:
"bt_index_check does not verify invariants that span child/parent relationships, but will verify the presence of all heap tuples as index tuples within the index when heapallindexed is true. When checkunique is true bt_index_check will check that no more than one among duplicate entries in unique index is visible. When a routine, lightweight test for corruption is required in a live production environment, using bt_index_check often provides the best trade-off between thoroughness of verification and limiting the impact on application performance and availability."
The second sentence, "When checkunique is true bt_index_check will check that no more than one among duplicate entries in unique index is visible." is not strictly true, as it won't check if the violation spans a page boundary. That's implied by the surrounding sentences, but I'm not sure a reader can be trusted to know which way to interpret how "checkunique" works. Clarification is needed.
The attached TAP test is not intended for commit. I am only including it here because you might want to use the TAP test as a starting point for creating and testing for new kinds of corruption. Beware the test intentionally includes an infinite loop, which is helpful for a developer examining the code, but not at all appropriate otherwise. It loads all blocks of the index into memory each loop, which could be made more efficient if we wanted this to be part of the core codebase. I just threw it together this morning. It's not polished, documented, checked for portability, or otherwise production quality.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[application/octet-stream] v1-0001-Add-a-WIP-corruption-checker.patch (4.6K, ../../[email protected]/2-v1-0001-Add-a-WIP-corruption-checker.patch)
download | inline diff:
From cc7bf7991ff827c4b41c0cd7888a073b91f66827 Mon Sep 17 00:00:00 2001
From: Mark Dilger <[email protected]>
Date: Fri, 17 May 2024 10:24:11 -0700
Subject: [PATCH v1] Add a WIP corruption checker
To help analyze Alexander Korotkov's v3 series of patches, add a
corruption checker that runs an infinite loop corrupting an index
and seeing if the corruption is detected.
THIS IS NOT FOR COMMIT.
---
contrib/amcheck/t/006_corrupt_idx.pl | 136 +++++++++++++++++++++++++++
1 file changed, 136 insertions(+)
create mode 100644 contrib/amcheck/t/006_corrupt_idx.pl
diff --git a/contrib/amcheck/t/006_corrupt_idx.pl b/contrib/amcheck/t/006_corrupt_idx.pl
new file mode 100644
index 0000000000..d27b0ff9b6
--- /dev/null
+++ b/contrib/amcheck/t/006_corrupt_idx.pl
@@ -0,0 +1,136 @@
+
+# Copyright (c) 2023-2024, PostgreSQL Global Development Group
+
+# This regression test checks the behavior of the btree validation in the
+# presence of breaking sort order changes.
+#
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Fcntl 'SEEK_SET';
+
+my $node = PostgreSQL::Test::Cluster->new('test');
+$node->init;
+$node->append_conf('postgresql.conf', 'autovacuum = off');
+$node->start;
+
+# Create tables and indexes over some values
+$node->safe_psql('postgres',qq(
+CREATE EXTENSION amcheck;
+CREATE TABLE tbl (i TEXT);
+INSERT INTO tbl (SELECT 'MAGIC_' || gs::TEXT FROM generate_series(1,100000) gs ORDER BY RANDOM());
+CREATE UNIQUE INDEX idx ON tbl (i);
+));
+
+my @blocks;
+
+while (1)
+{
+ my ($result, $stdout, $stderr);
+
+ $result = $node->safe_psql(
+ 'postgres', q(
+ SELECT bt_index_parent_check('idx', true, true, true);
+ ));
+ is($result, '', 'run amcheck on non-broken idx');
+
+ my $pgdata = $node->data_dir;
+ my $rel = $node->safe_psql('postgres',
+ qq(SELECT pg_relation_filepath('public.idx')));
+ my $relpath = "$pgdata/$rel";
+ $node->stop;
+
+ my ($blksize, @blocks) = read_blocks($relpath);
+
+ my $ttl = 1000;
+ my $corrupted_blkno = undef;
+ while (!defined($corrupted_blkno) && $ttl--)
+ {
+ my $blkno = int(rand(scalar(@blocks)-1));
+
+ if ($blocks[$blkno] =~ m/.*MAGIC_(\d+)/)
+ {
+ my $magic = $1;
+ my $corrupted_block = $blocks[$blkno];
+
+ my $next_magic = $magic + 1;
+ my $prev_magic = $magic - 1;
+ if ($next_magic >= 1 && $next_magic <= 100000 && $blocks[$blkno+1] =~ m/MAGIC_$next_magic/)
+ {
+ if ($corrupted_block =~ s/MAGIC_$magic/MAGIC_$next_magic/)
+ {
+ write_block($relpath, $blksize, $blkno, $corrupted_block);
+ $corrupted_blkno = $blkno;
+ }
+ }
+ elsif ($prev_magic >= 1 && $prev_magic <= 100000 && $blocks[$blkno+1] =~ m/MAGIC_$prev_magic/)
+ {
+ if ($corrupted_block =~ s/MAGIC_$magic/MAGIC_$prev_magic/)
+ {
+ write_block($relpath, $blksize, $blkno, $corrupted_block);
+ $corrupted_blkno = $blkno;
+ }
+ }
+ }
+ }
+
+ BAIL_OUT("Failed to corrupt anything") unless($ttl > 0);
+
+ # Ok, we've corrupted the file. Restart the node and see if the
+ # corruption checker notices anything.
+ $node->start;
+
+ ($result, $stdout, $stderr) = $node->psql(
+ 'postgres', q(
+ SELECT bt_index_parent_check('idx', true, true, true);
+ ));
+ ok( $stderr =~ /item order invariant violated for index "idx"|index uniqueness is violated for index "idx"|could not find tuple using search from root page in index "idx"|mismatch between parent key and child high key in index "idx"|detected uniqueness violation for index "idx"/);
+
+ # Repair the damage.
+ $node->stop;
+ write_block($relpath, $blksize, $corrupted_blkno, $blocks[$corrupted_blkno]);
+
+ # Restart the database and confirm the index is back to passing
+ $node->start;
+ $result = $node->safe_psql(
+ 'postgres', q(
+ SELECT bt_index_check('idx', true, true);
+ ));
+ is($result, '', 'run amcheck on non-broken idx');
+}
+
+# Not reached
+done_testing();
+
+sub read_blocks
+{
+ my ($relpath) = @_;
+ my $file;
+ open($file, '+<', $relpath)
+ or BAIL_OUT("open failed: $!");
+ binmode $file;
+ my ($dev, $ino, $mode, $nlink, $uid, $gid, $rdev, $size, $atime, $mtime, $ctime, $blksize, $blocks) = stat($file);
+
+ my @result;
+ for (my $blkno = 0; $blkno < $blocks; $blkno++)
+ {
+ sysseek($file, $blkno * $blksize, SEEK_SET);
+ sysread($file, $result[$blkno], $blksize);
+ }
+ close($file);
+ return ($blksize, @result);
+}
+
+sub write_block
+{
+ my ($relpath, $blksize, $blkno, $block) = @_;
+ my $file;
+ open($file, '+<', $relpath)
+ or BAIL_OUT("open failed: $!");
+ binmode $file;
+ sysseek($file, $blkno * $blksize, SEEK_SET);
+ syswrite($file, $block);
+ close($file);
+}
--
2.39.3 (Apple Git-145)
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-17 18:51 Pavel Borisov <[email protected]>
parent: Mark Dilger <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Pavel Borisov @ 2024-05-17 18:51 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
Hi, Mark!
> The documentation in
> https://www.postgresql.org/docs/devel/amcheck.html#AMCHECK-FUNCTIONS is
> ambiguous:
>
> "bt_index_check does not verify invariants that span child/parent
> relationships, but will verify the presence of all heap tuples as index
> tuples within the index when heapallindexed is true. When checkunique is
> true bt_index_check will check that no more than one among duplicate
> entries in unique index is visible. When a routine, lightweight test for
> corruption is required in a live production environment, using
> bt_index_check often provides the best trade-off between thoroughness of
> verification and limiting the impact on application performance and
> availability."
>
> The second sentence, "When checkunique is true bt_index_check will check
> that no more than one among duplicate entries in unique index is visible."
> is not strictly true, as it won't check if the violation spans a page
> boundary.
>
Amcheck with checkunique option does check uniqueness violation between
pages. But it doesn't warranty detection of cross page uniqueness
violations in extremely rare cases when the first equal index entry on the
next page corresponds to tuple that is not visible (e.g. dead). In this, I
followed the Peter's notion [1] that checking across a number of dead equal
entries that could theoretically span even across many pages is an
unneeded code complication and amcheck is not a tool that provides any
warranty when checking an index.
I'm not against docs modification in any way that clarifies its exact usage
and limitations.
Kind regards,
Pavel Borisov
[1]
https://www.postgresql.org/message-id/CAH2-Wz%3DttG__BTZ-r5ccopBRb5evjg%3DzsF_o_3C5h4zRBA_LjQ%40mail...
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-17 19:10 Mark Dilger <[email protected]>
parent: Pavel Borisov <[email protected]>
0 siblings, 2 replies; 28+ messages in thread
From: Mark Dilger @ 2024-05-17 19:10 UTC (permalink / raw)
To: Pavel Borisov <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
> On May 17, 2024, at 11:51 AM, Pavel Borisov <[email protected]> wrote:
>
> Amcheck with checkunique option does check uniqueness violation between pages. But it doesn't warranty detection of cross page uniqueness violations in extremely rare cases when the first equal index entry on the next page corresponds to tuple that is not visible (e.g. dead). In this, I followed the Peter's notion [1] that checking across a number of dead equal entries that could theoretically span even across many pages is an unneeded code complication and amcheck is not a tool that provides any warranty when checking an index.
This confuses me a bit. The regression test creates a table and index but never performs any DELETE nor any UPDATE operations, so none of the index entries should be dead. If I am understanding you correct, I'd be forced to conclude that the uniqueness checking code is broken. Can you take a look?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-17 19:42 Mark Dilger <[email protected]>
parent: Mark Dilger <[email protected]>
1 sibling, 1 reply; 28+ messages in thread
From: Mark Dilger @ 2024-05-17 19:42 UTC (permalink / raw)
To: Pavel Borisov <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
> On May 17, 2024, at 12:10 PM, Mark Dilger <[email protected]> wrote:
>
>> Amcheck with checkunique option does check uniqueness violation between pages. But it doesn't warranty detection of cross page uniqueness violations in extremely rare cases when the first equal index entry on the next page corresponds to tuple that is not visible (e.g. dead). In this, I followed the Peter's notion [1] that checking across a number of dead equal entries that could theoretically span even across many pages is an unneeded code complication and amcheck is not a tool that provides any warranty when checking an index.
>
> This confuses me a bit. The regression test creates a table and index but never performs any DELETE nor any UPDATE operations, so none of the index entries should be dead. If I am understanding you correct, I'd be forced to conclude that the uniqueness checking code is broken. Can you take a look?
On further review, the test was not anticipating the error message "high key invariant violated for index". That wasn't seen in calls to bt_index_parent_check(), but appears as one of the errors from bt_index_check(). I am rerunning the test now....
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-17 19:42 Pavel Borisov <[email protected]>
parent: Mark Dilger <[email protected]>
1 sibling, 1 reply; 28+ messages in thread
From: Pavel Borisov @ 2024-05-17 19:42 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
Hi, Mark!
On Fri, 17 May 2024 at 23:10, Mark Dilger <[email protected]>
wrote:
>
>
> > On May 17, 2024, at 11:51 AM, Pavel Borisov <[email protected]>
> wrote:
> >
> > Amcheck with checkunique option does check uniqueness violation between
> pages. But it doesn't warranty detection of cross page uniqueness
> violations in extremely rare cases when the first equal index entry on the
> next page corresponds to tuple that is not visible (e.g. dead). In this, I
> followed the Peter's notion [1] that checking across a number of dead equal
> entries that could theoretically span even across many pages is an unneeded
> code complication and amcheck is not a tool that provides any warranty when
> checking an index.
>
> This confuses me a bit. The regression test creates a table and index but
> never performs any DELETE nor any UPDATE operations, so none of the index
> entries should be dead. If I am understanding you correct, I'd be forced
> to conclude that the uniqueness checking code is broken. Can you take a
> look?
>
At the first glance it's not clear to me:
- why your test creates cross-page unique constraint violations?
- how do you know they are not detected?
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-17 20:00 Peter Geoghegan <[email protected]>
parent: Mark Dilger <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Peter Geoghegan @ 2024-05-17 20:00 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
On Fri, May 17, 2024 at 3:42 PM Mark Dilger
<[email protected]> wrote:
> On further review, the test was not anticipating the error message "high key invariant violated for index". That wasn't seen in calls to bt_index_parent_check(), but appears as one of the errors from bt_index_check(). I am rerunning the test now....
Many different parts of the B-Tree code will fight against allowing
duplicates of the same value to span multiple leaf pages -- this is
especially true for unique indexes. For example, nbtsplitloc.c has a
variety of strategies that will prevent choosing a split point that
necessitates including a distinguishing heap TID in the new high key.
In other words, nbtsplitloc.c is very aggressive about picking a split
point between (rather than within) groups of duplicates.
Of course it's still *possible* for a unique index to have multiple
leaf pages containing the same individual value. The regression tests
do have coverage for certain relevant code paths (e.g., there is
coverage for code paths only hit when _bt_check_unique has to go to
the page to the right). This is only the case because I went out of my
way to make sure of it, by adding tests that allow a huge number of
version duplicates to accumulate within a unique index. (The "move
right" _bt_check_unique branches had zero test coverage for a year or
two.)
Just how important it is that amcheck covers cases where the version
duplicates span multiple leaf pages is of course debatable -- it's
always better to be more thorough, when practical. But it's certainly
something that needs to be assessed based on the merits.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-17 20:08 Mark Dilger <[email protected]>
parent: Pavel Borisov <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Mark Dilger @ 2024-05-17 20:08 UTC (permalink / raw)
To: Pavel Borisov <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
> On May 17, 2024, at 12:42 PM, Pavel Borisov <[email protected]> wrote:
>
> At the first glance it's not clear to me:
> - why your test creates cross-page unique constraint violations?
To see if they are detected.
> - how do you know they are not detected?
It appears that they are detected. At least, rerunning the test after adjusting the expected output, I no longer see problems.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-17 20:10 Mark Dilger <[email protected]>
parent: Peter Geoghegan <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Mark Dilger @ 2024-05-17 20:10 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
> On May 17, 2024, at 1:00 PM, Peter Geoghegan <[email protected]> wrote:
>
> Many different parts of the B-Tree code will fight against allowing
> duplicates of the same value to span multiple leaf pages -- this is
> especially true for unique indexes.
The quick-and-dirty TAP test I wrote this morning is intended to introduce duplicates across page boundaries, not to test for ones that got there by normal database activity. In other words, the TAP test forcibly corrupts the index by changing a value on one side of a boundary to be equal to the value on the other side of the boundary. Prior to the corrupting action the values were all unique.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-17 20:13 Peter Geoghegan <[email protected]>
parent: Mark Dilger <[email protected]>
0 siblings, 0 replies; 28+ messages in thread
From: Peter Geoghegan @ 2024-05-17 20:13 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
On Fri, May 17, 2024 at 4:10 PM Mark Dilger
<[email protected]> wrote:
> The quick-and-dirty TAP test I wrote this morning is intended to introduce duplicates across page boundaries, not to test for ones that got there by normal database activity. In other words, the TAP test forcibly corrupts the index by changing a value on one side of a boundary to be equal to the value on the other side of the boundary. Prior to the corrupting action the values were all unique.
I understood that. I was just pointing out that an index that looks
even somewhat like that is already quite unnatural.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-05-17 21:08 Pavel Borisov <[email protected]>
parent: Mark Dilger <[email protected]>
0 siblings, 0 replies; 28+ messages in thread
From: Pavel Borisov @ 2024-05-17 21:08 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
Hi, Mark!
> > At the first glance it's not clear to me:
> > - why your test creates cross-page unique constraint violations?
>
> To see if they are detected.
>
> > - how do you know they are not detected?
>
> It appears that they are detected. At least, rerunning the test after
> adjusting the expected output, I no longer see problems.
>
I understand your point. It was unclear how it modified the index so that
only unique constraint check between pages should have failed with other
checks passed.
Anyway, thanks for your testing and efforts! I'm happy that the test now
passes and confirms that amcheck feature works as intended.
Kind regards,
Pavel Borisov
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-07-26 12:10 Alexander Korotkov <[email protected]>
parent: Alexander Korotkov <[email protected]>
2 siblings, 1 reply; 28+ messages in thread
From: Alexander Korotkov @ 2024-07-26 12:10 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
On Fri, May 17, 2024 at 1:11 PM Alexander Korotkov <[email protected]> wrote:
> I think the thread contains enough motivation on why 0002, 0003 and
> 0004 are material for post-FF. They are fixes and refactoring for
> new-in-v17 feature. I'm going to push them if no objections.
>
> Regarding 0001, I'd like to ask Tom and Mark if they find convincing
> that given that optimization is small, simple and giving huge effect,
> it could be pushed post-FF? Otherwise, this could wait for v18.
The revised version of 0001 unique checking optimization is attached.
I'm going to push this to v18 if no objections.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v4-0001-amcheck-Optimize-speed-of-checking-for-unique-con.patch (3.8K, ../../CAPpHfdusEsgnQDNTav=H45hcxPDsVY0H35jT0VZhhkfb=yWeWw@mail.gmail.com/2-v4-0001-amcheck-Optimize-speed-of-checking-for-unique-con.patch)
download | inline diff:
From 324ab164ef7e32dd40e120bc22a79711a82cd77b Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Fri, 10 May 2024 03:07:53 +0300
Subject: [PATCH v4] amcheck: Optimize speed of checking for unique constraint
violation
Currently, when amcheck validates a unique constraint, it visits the heap for
each index tuple. This commit implements skipping keys, which have only one
non-dedeuplicated index tuple (quite common case for unique indexes). That
gives substantial economy on index checking time.
Reported-by: Noah Misch
Discussion: https://postgr.es/m/20240325020323.fd.nmisch%40google.com
Author: Alexander Korotkov, Pavel Borisov
---
contrib/amcheck/verify_nbtree.c | 36 ++++++++++++++++++++++++++++++---
1 file changed, 33 insertions(+), 3 deletions(-)
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 34990c5cea3..7cfb136763f 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -1433,6 +1433,13 @@ bt_target_page_check(BtreeCheckState *state)
bool lowersizelimit;
ItemPointer scantid;
+ /*
+ * True if we already called bt_entry_unique_check() for the current
+ * item. This helps to avoid visiting the heap for keys, which are
+ * anyway presented only once and can't comprise a unique violation.
+ */
+ bool unique_checked = false;
+
CHECK_FOR_INTERRUPTS();
itemid = PageGetItemIdCareful(state, state->targetblock,
@@ -1775,12 +1782,18 @@ bt_target_page_check(BtreeCheckState *state)
/*
* If the index is unique verify entries uniqueness by checking the
- * heap tuples visibility.
+ * heap tuples visibility. Immediately check posting tuples and
+ * tuples with repeated keys. Postpone check for keys, which have the
+ * first appearance.
*/
if (state->checkunique && state->indexinfo->ii_Unique &&
- P_ISLEAF(topaque) && !skey->anynullkeys)
+ P_ISLEAF(topaque) && !skey->anynullkeys &&
+ (BTreeTupleIsPosting(itup) || ItemPointerIsValid(lVis.tid)))
+ {
bt_entry_unique_check(state, itup, state->targetblock, offset,
&lVis);
+ unique_checked = true;
+ }
if (state->checkunique && state->indexinfo->ii_Unique &&
P_ISLEAF(topaque) && OffsetNumberNext(offset) <= max)
@@ -1799,6 +1812,9 @@ bt_target_page_check(BtreeCheckState *state)
* data (whole index tuple or last posting in index tuple). Key
* containing null value does not violate unique constraint and
* treated as different to any other key.
+ *
+ * If the next key is the same as the previous one, do the
+ * bt_entry_unique_check() call if it was postponed.
*/
if (_bt_compare(state->rel, skey, state->target,
OffsetNumberNext(offset)) != 0 || skey->anynullkeys)
@@ -1808,6 +1824,11 @@ bt_target_page_check(BtreeCheckState *state)
lVis.postingIndex = -1;
lVis.tid = NULL;
}
+ else if (!unique_checked)
+ {
+ bt_entry_unique_check(state, itup, state->targetblock, offset,
+ &lVis);
+ }
skey->scantid = scantid; /* Restore saved scan key state */
}
@@ -1890,10 +1911,19 @@ bt_target_page_check(BtreeCheckState *state)
rightkey->scantid = NULL;
/* The first key on the next page is the same */
- if (_bt_compare(state->rel, rightkey, state->target, max) == 0 && !rightkey->anynullkeys)
+ if (_bt_compare(state->rel, rightkey, state->target, max) == 0 &&
+ !rightkey->anynullkeys)
{
Page rightpage;
+ /*
+ * Do the bt_entry_unique_check() call if it was
+ * postponed.
+ */
+ if (!unique_checked)
+ bt_entry_unique_check(state, itup, state->targetblock,
+ offset, &lVis);
+
elog(DEBUG2, "cross page equal keys");
rightpage = palloc_btree_page(state,
rightblock_number);
--
2.39.3 (Apple Git-145)
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-07-26 14:38 Robert Haas <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 28+ messages in thread
From: Robert Haas @ 2024-07-26 14:38 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Mark Dilger <[email protected]>; Pavel Borisov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
On Fri, Jul 26, 2024 at 8:10 AM Alexander Korotkov <[email protected]> wrote:
> The revised version of 0001 unique checking optimization is attached.
> I'm going to push this to v18 if no objections.
I have no reason to specifically object to pushing this into 18, but I
would like to point out that you're posting here about this but failed
to reply to the "64-bit pg_notify page numbers truncated to 32-bit",
an open item that was assigned to you but which, since you didn't
respond, was eventually fixed by commits from Michael Paquier.
I know it's easy to lose track of the open items list and I sometimes
forget to check it myself, but it's rather important to stay on top of
any open items that get assigned to you.
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
@ 2024-07-26 20:53 Alexander Korotkov <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 0 replies; 28+ messages in thread
From: Alexander Korotkov @ 2024-07-26 20:53 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Mark Dilger <[email protected]>; Pavel Borisov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected]
On Fri, Jul 26, 2024 at 5:38 PM Robert Haas <[email protected]> wrote:
> On Fri, Jul 26, 2024 at 8:10 AM Alexander Korotkov <[email protected]> wrote:
> > The revised version of 0001 unique checking optimization is attached.
> > I'm going to push this to v18 if no objections.
>
> I have no reason to specifically object to pushing this into 18, but I
> would like to point out that you're posting here about this but failed
> to reply to the "64-bit pg_notify page numbers truncated to 32-bit",
> an open item that was assigned to you but which, since you didn't
> respond, was eventually fixed by commits from Michael Paquier.
>
> I know it's easy to lose track of the open items list and I sometimes
> forget to check it myself, but it's rather important to stay on top of
> any open items that get assigned to you.
Yes, it's a pity I miss this open item on me. Besides putting ashes
on my head, I think I could pay more attention on other open items.
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: FileFallocate misbehaving on XFS
@ 2025-03-11 01:39 Michael Harris <[email protected]>
0 siblings, 0 replies; 28+ 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] 28+ messages in thread
* Re: FileFallocate misbehaving on XFS
@ 2025-03-27 10:25 Andrea Gelmini <[email protected]>
1 sibling, 0 replies; 28+ 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] 28+ messages in thread
* Re: FileFallocate misbehaving on XFS
@ 2025-05-11 18:30 Pierre Barre <[email protected]>
1 sibling, 0 replies; 28+ 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] 28+ messages in thread
end of thread, other threads:[~2025-05-11 18:30 UTC | newest]
Thread overview: 28+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
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-10 19:05 Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Alexander Korotkov <[email protected]>
2024-05-11 01:12 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Mark Dilger <[email protected]>
2024-05-11 01:38 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Tom Lane <[email protected]>
2024-05-12 21:23 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Alexander Korotkov <[email protected]>
2024-05-13 01:42 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Alexander Korotkov <[email protected]>
2024-05-13 11:55 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Pavel Borisov <[email protected]>
2024-05-13 12:19 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Pavel Borisov <[email protected]>
2024-05-13 12:20 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Pavel Borisov <[email protected]>
2024-05-17 10:11 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Alexander Korotkov <[email protected]>
2024-05-17 11:09 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Pavel Borisov <[email protected]>
2024-05-17 17:41 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Mark Dilger <[email protected]>
2024-05-17 18:51 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Pavel Borisov <[email protected]>
2024-05-17 19:10 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Mark Dilger <[email protected]>
2024-05-17 19:42 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Mark Dilger <[email protected]>
2024-05-17 20:00 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Peter Geoghegan <[email protected]>
2024-05-17 20:10 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Mark Dilger <[email protected]>
2024-05-17 20:13 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Peter Geoghegan <[email protected]>
2024-05-17 19:42 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Pavel Borisov <[email protected]>
2024-05-17 20:08 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Mark Dilger <[email protected]>
2024-05-17 21:08 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Pavel Borisov <[email protected]>
2024-07-26 12:10 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Alexander Korotkov <[email protected]>
2024-07-26 14:38 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Robert Haas <[email protected]>
2024-07-26 20:53 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Alexander Korotkov <[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