From 19eb84d9d9aca2bf24ebc8bd8068d77881def730 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 19 Dec 2025 12:40:03 -0500 Subject: [PATCH v2 3/3] Mark GiST inet_ops as opcdefault, and deal with ensuing fallout. This patch completes the transition to making inet_ops be default for inet/cidr columns, rather than btree_gist's opclasses. Once we do that, though, pg_upgrade has a big problem. A dump from an older version will see btree_gist's opclasses as being default, so it will not mention the opclass explicitly in CREATE INDEX commands, which would cause the restore to create the indexes using inet_ops. Since that's not compatible with what's actually in the files, havoc would ensue. This isn't readily fixable, because the CREATE INDEX command strings are built by the older server's pg_get_indexdef() function; pg_dump hasn't nearly enough knowledge to modify those strings successfully. Even if we cared to put in the work to make that happen in pg_dump, it would be counterproductive because the end goal here is to get people off of these opclasses. Allowing such indexes to persist through pg_upgrade wouldn't advance that goal. Therefore, this patch just adds code to pg_upgrade to detect indexes that would be problematic and refuse to upgrade. There's another issue too: even without any indexes to worry about, pg_dump in binary-upgrade mode will reproduce the "CREATE OPERATOR CLASS ... DEFAULT" commands for btree_gist's opclasses, and those will fail because now we have a built-in opclass that provides a conflicting default. We could ask users to drop the btree_gist extension altogether before upgrading, but that would carry very severe penalties. It would affect perfectly-valid indexes for other data types, and it would drop operators that might be relied on in views or other database objects. Instead, put a hack in DefineOpClass to ignore the DEFAULT clauses for these opclasses when in binary-upgrade mode. This will result in installing a version of btree_gist that isn't quite the version it claims to be, but that can be fixed by issuing ALTER EXTENSION UPDATE afterwards. Since we don't apply that hack when not in binary-upgrade mode, it is now impossible to install any version of btree_gist less than 1.9 into a v19-or-later server. We could drop the btree_gist--1.2.sql script altogether, although I've not included that in this patch. But we must keep the subsequent delta scripts, so that users can bring an old version of btree_gist up to 1.9. Author: Tom Lane Discussion: https://postgr.es/m/2483812.1754072263@sss.pgh.pa.us --- doc/src/sgml/btree-gist.sgml | 47 ++++++++++ src/backend/commands/opclasscmds.c | 24 ++++- src/bin/pg_upgrade/check.c | 93 +++++++++++++++++++ src/include/catalog/pg_opclass.dat | 2 +- .../perl/PostgreSQL/Test/AdjustUpgrade.pm | 23 +++++ 5 files changed, 186 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/btree-gist.sgml b/doc/src/sgml/btree-gist.sgml index a4c1b99be1f..41df635fd5e 100644 --- a/doc/src/sgml/btree-gist.sgml +++ b/doc/src/sgml/btree-gist.sgml @@ -108,6 +108,53 @@ INSERT 0 1 + + <literal>btree_gist</literal> Indexes + on <type>inet</type>/<type>cidr</type> Columns + + + The gist_inet_ops and gist_cidr_ops + operator classes provided by btree_gist have been + shown to be unreliable: index searches may find the wrong rows due to + approximations used in creating the index entries. This is unfixable + without redefining the contents of indexes that use these opclasses. + Therefore, these opclasses are being deprecated in favor of the built-in + GiST inet_ops opclass, which does not share the design + flaw. + + + + As a first step, PostgreSQL version 19 removes + the default-opclass marking from gist_inet_ops + and gist_cidr_ops, instead + marking inet_ops as default for inet + and cidr columns. This will result in transparently + substituting inet_ops for the faulty opclasses in most + contexts. It is still possible to create indexes using the faulty + opclasses, if really necessary, by explicitly specifying which opclass to + use; for example + +CREATE TABLE mytable (addr inet); +CREATE INDEX dubious_index ON mytable USING GIST (addr gist_inet_ops); + + + + + However, pg_upgrade cannot handle this change + due to implementation limitations. If asked to upgrade a pre-v19 + database that contains gist_inet_ops + or gist_cidr_ops + indexes, pg_upgrade will fail and tell you to + replace those indexes before upgrading. This would look approximately + like + +CREATE INDEX good_index ON mytable USING GIST (addr inet_ops); +DROP INDEX bad_index; + + + + + Authors diff --git a/src/backend/commands/opclasscmds.c b/src/backend/commands/opclasscmds.c index 992ae789b00..d0421cf0162 100644 --- a/src/backend/commands/opclasscmds.c +++ b/src/backend/commands/opclasscmds.c @@ -343,6 +343,7 @@ DefineOpClass(CreateOpClassStmt *stmt) optsProcNumber, /* amoptsprocnum value */ maxProcNumber; /* amsupport value */ bool amstorage; /* amstorage flag */ + bool isDefault = stmt->isDefault; List *operators; /* OpFamilyMember list for operators */ List *procedures; /* OpFamilyMember list for support procs */ ListCell *l; @@ -610,12 +611,31 @@ DefineOpClass(CreateOpClassStmt *stmt) errmsg("operator class \"%s\" for access method \"%s\" already exists", opcname, stmt->amname))); + /* + * HACK: if we're trying to create btree_gist's gist_inet_ops or + * gist_cidr_ops during a binary upgrade, avoid failure in the next stanza + * by silently making the new opclass non-default. Without this kluge, we + * would fail to upgrade databases containing pre-1.9 versions of + * contrib/btree_gist. We can remove it sometime in the far future when + * we don't expect any such databases to exist. (The result of this hack + * is that the installed version of btree_gist will approximate btree_gist + * 1.9, how closely depending on whether it's 1.8 or something older. + * ALTER EXTENSION UPDATE can be used to bring it up to real 1.9.) + */ + if (isDefault && IsBinaryUpgrade) + { + if (amoid == GIST_AM_OID && + ((typeoid == INETOID && strcmp(opcname, "gist_inet_ops") == 0) || + (typeoid == CIDROID && strcmp(opcname, "gist_cidr_ops") == 0))) + isDefault = false; + } + /* * If we are creating a default opclass, check there isn't one already. * (Note we do not restrict this test to visible opclasses; this ensures * that typcache.c can find unique solutions to its questions.) */ - if (stmt->isDefault) + if (isDefault) { ScanKeyData skey[1]; SysScanDesc scan; @@ -661,7 +681,7 @@ DefineOpClass(CreateOpClassStmt *stmt) values[Anum_pg_opclass_opcowner - 1] = ObjectIdGetDatum(GetUserId()); values[Anum_pg_opclass_opcfamily - 1] = ObjectIdGetDatum(opfamilyoid); values[Anum_pg_opclass_opcintype - 1] = ObjectIdGetDatum(typeoid); - values[Anum_pg_opclass_opcdefault - 1] = BoolGetDatum(stmt->isDefault); + values[Anum_pg_opclass_opcdefault - 1] = BoolGetDatum(isDefault); values[Anum_pg_opclass_opckeytype - 1] = ObjectIdGetDatum(storageoid); tup = heap_form_tuple(rel->rd_att, values, nulls); diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 1e17d64b3ec..48fecefc626 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -9,6 +9,7 @@ #include "postgres_fe.h" +#include "catalog/pg_am_d.h" #include "catalog/pg_authid_d.h" #include "catalog/pg_class_d.h" #include "fe_utils/string_utils.h" @@ -24,6 +25,7 @@ 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_not_null_inheritance(ClusterInfo *cluster); +static void check_for_gist_inet_ops(ClusterInfo *cluster); static void check_for_pg_role_prefix(ClusterInfo *cluster); static void check_for_new_tablespace_dir(void); static void check_for_user_defined_encoding_conversions(ClusterInfo *cluster); @@ -681,6 +683,21 @@ check_and_dump_old_cluster(void) if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1800) check_for_not_null_inheritance(&old_cluster); + /* + * The btree_gist extension contains gist_inet_ops and gist_cidr_ops + * opclasses that do not reliably give correct answers. We want to + * deprecate and eventually remove those, and as a first step v19 marks + * them not-opcdefault and instead marks the replacement in-core opclass + * "inet_ops" as opcdefault. That creates a problem for pg_upgrade: in + * versions where those opclasses were marked opcdefault, pg_dump will + * dump indexes using them with no explicit opclass specification, so that + * restore would create them using the inet_ops opclass. That would be + * incompatible with what's actually in the on-disk files. So refuse to + * upgrade if there are any such indexes. + */ + if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1800) + check_for_gist_inet_ops(&old_cluster); + /* * Pre-PG 10 allowed tables with 'unknown' type columns and non WAL logged * hash indexes @@ -1721,6 +1738,82 @@ check_for_not_null_inheritance(ClusterInfo *cluster) check_ok(); } +/* + * Callback function for processing results of query for + * check_for_gist_inet_ops()'s UpgradeTask. If the query returned any rows + * (i.e., the check failed), write the details to the report file. + */ +static void +process_gist_inet_ops_check(DbInfo *dbinfo, PGresult *res, void *arg) +{ + UpgradeTaskReport *report = (UpgradeTaskReport *) arg; + int ntups = PQntuples(res); + int i_nspname = PQfnumber(res, "nspname"); + int i_relname = PQfnumber(res, "relname"); + + AssertVariableIsOfType(&process_gist_inet_ops_check, UpgradeTaskProcessCB); + + if (ntups == 0) + return; + + if (report->file == NULL && + (report->file = fopen_priv(report->path, "w")) == NULL) + pg_fatal("could not open file \"%s\": %m", report->path); + + fprintf(report->file, "In database: %s\n", dbinfo->db_name); + + for (int rowno = 0; rowno < ntups; rowno++) + fprintf(report->file, " %s.%s\n", + PQgetvalue(res, rowno, i_nspname), + PQgetvalue(res, rowno, i_relname)); +} + +/* + * Verify that no indexes use gist_inet_ops/gist_cidr_ops, unless the + * opclasses have been changed to not-opcdefault (which would allow + * the old server to dump the index definitions with explicit opclasses). + */ +static void +check_for_gist_inet_ops(ClusterInfo *cluster) +{ + UpgradeTaskReport report; + UpgradeTask *task = upgrade_task_create(); + const char *query = "SELECT nc.nspname, cc.relname " + "FROM pg_catalog.pg_opclass oc, pg_catalog.pg_index i, " + " pg_catalog.pg_class cc, pg_catalog.pg_namespace nc " + "WHERE oc.opcmethod = " CppAsString2(GIST_AM_OID) + " AND oc.opcname IN ('gist_inet_ops', 'gist_cidr_ops')" + " AND oc.opcdefault" + " AND oc.oid = any(i.indclass)" + " AND i.indexrelid = cc.oid AND cc.relnamespace = nc.oid"; + + prep_status("Checking for uses of gist_inet_ops/gist_cidr_ops"); + + report.file = NULL; + snprintf(report.path, sizeof(report.path), "%s/%s", + log_opts.basedir, + "gist_inet_ops.txt"); + + upgrade_task_add_step(task, query, process_gist_inet_ops_check, + true, &report); + upgrade_task_run(task, cluster); + upgrade_task_free(task); + + if (report.file) + { + fclose(report.file); + pg_log(PG_REPORT, "fatal"); + pg_fatal("Your installation contains indexes that use btree_gist's\n" + "gist_inet_ops or gist_cidr_ops opclasses,\n" + "which cannot be binary-upgraded. Replace them with indexes\n" + "that use the built-in GiST inet_ops opclass.\n" + "A list of indexes with the problem is in the file:\n" + " %s", report.path); + } + else + check_ok(); +} + /* * check_for_pg_role_prefix() * diff --git a/src/include/catalog/pg_opclass.dat b/src/include/catalog/pg_opclass.dat index 4a9624802aa..4b2c3a52403 100644 --- a/src/include/catalog/pg_opclass.dat +++ b/src/include/catalog/pg_opclass.dat @@ -57,7 +57,7 @@ { opcmethod => 'hash', opcname => 'inet_ops', opcfamily => 'hash/network_ops', opcintype => 'inet' }, { opcmethod => 'gist', opcname => 'inet_ops', opcfamily => 'gist/network_ops', - opcintype => 'inet', opcdefault => 'f' }, + opcintype => 'inet' }, { opcmethod => 'spgist', opcname => 'inet_ops', opcfamily => 'spgist/network_ops', opcintype => 'inet' }, { oid => '1979', oid_symbol => 'INT2_BTREE_OPS_OID', diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm index 7224c286e1d..1316984ce3d 100644 --- a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm +++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm @@ -112,6 +112,29 @@ sub adjust_database_contents 'drop extension if exists test_ext7'); } + # btree_gist inet/cidr indexes cannot be upgraded to v19 + if ($old_version < 19) + { + if ($dbnames{"contrib_regression_btree_gist"}) + { + _add_st($result, 'contrib_regression_btree_gist', + "drop index if exists public.inettmp_a_a1_idx"); + _add_st($result, 'contrib_regression_btree_gist', + "drop index if exists public.inetidx"); + _add_st($result, 'contrib_regression_btree_gist', + "drop index public.cidridx"); + } + if ($dbnames{"regression_btree_gist"}) + { + _add_st($result, 'regression_btree_gist', + "drop index if exists public.inettmp_a_a1_idx"); + _add_st($result, 'regression_btree_gist', + "drop index if exists public.inetidx"); + _add_st($result, 'regression_btree_gist', + "drop index public.cidridx"); + } + } + # we removed these test-support functions in v18 if ($old_version < 18) { -- 2.43.7