From 9b594f81fb3b1ebe6ea79d14d4039778e9e523a2 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 1 Aug 2025 14:08:13 -0400 Subject: [PATCH v1] Mark GiST network_ops opcdefault, and btree_gist's opclasses not. We want to deprecate btree_gist's gist_inet_ops and gist_cidr_ops opclasses, because they sometimes give the wrong answers. (We won't remove those opclasses completely just yet, and even if we did, it wouldn't make this undertaking any less messy.) As a first step on that road, make the replacement opclass the default one. GetDefaultOpClass() enforces that there can be only one default opclass per index AM and input datatype, so we have to remove the DEFAULT markings on gist_inet_ops and gist_cidr_ops. The only way to do this that doesn't cause failures in pg_upgrade is to hack up DefineOpClass() to ignore those markings. Even then, pg_upgrade would do the wrong things with such indexes, so refuse to upgrade them. TODO: user-facing docs, cross-version-upgrade test support. XXX: don't forget catversion bump. --- contrib/btree_gist/btree_gist--1.2.sql | 4 ++ contrib/btree_gist/expected/cidr.out | 2 +- contrib/btree_gist/expected/inet.out | 2 +- contrib/btree_gist/sql/cidr.sql | 2 +- contrib/btree_gist/sql/inet.sql | 2 +- src/backend/commands/opclasscmds.c | 20 +++++- src/bin/pg_upgrade/check.c | 90 ++++++++++++++++++++++++++ src/include/catalog/pg_opclass.dat | 2 +- 8 files changed, 117 insertions(+), 7 deletions(-) diff --git a/contrib/btree_gist/btree_gist--1.2.sql b/contrib/btree_gist/btree_gist--1.2.sql index 1efe7530438..7b3012032c3 100644 --- a/contrib/btree_gist/btree_gist--1.2.sql +++ b/contrib/btree_gist/btree_gist--1.2.sql @@ -1492,6 +1492,10 @@ ALTER OPERATOR FAMILY gist_vbit_ops USING gist ADD -- -- inet/cidr ops -- +-- NOTE: while the CREATE OPERATOR CLASS commands below say DEFAULT, +-- in a v19 or later server DefineOpClass will ignore that and make +-- gist_inet_ops and gist_cidr_ops non-default. +-- -- -- -- define the GiST support methods diff --git a/contrib/btree_gist/expected/cidr.out b/contrib/btree_gist/expected/cidr.out index 6d0995add60..e61df27affc 100644 --- a/contrib/btree_gist/expected/cidr.out +++ b/contrib/btree_gist/expected/cidr.out @@ -32,7 +32,7 @@ SELECT count(*) FROM cidrtmp WHERE a > '121.111.63.82'; 309 (1 row) -CREATE INDEX cidridx ON cidrtmp USING gist ( a ); +CREATE INDEX cidridx ON cidrtmp USING gist ( a gist_cidr_ops ); SET enable_seqscan=off; SELECT count(*) FROM cidrtmp WHERE a < '121.111.63.82'::cidr; count diff --git a/contrib/btree_gist/expected/inet.out b/contrib/btree_gist/expected/inet.out index f15f1435f0a..8cf12e3df8e 100644 --- a/contrib/btree_gist/expected/inet.out +++ b/contrib/btree_gist/expected/inet.out @@ -32,7 +32,7 @@ SELECT count(*) FROM inettmp WHERE a > '89.225.196.191'; 386 (1 row) -CREATE INDEX inetidx ON inettmp USING gist ( a ); +CREATE INDEX inetidx ON inettmp USING gist ( a gist_inet_ops ); SET enable_seqscan=off; SELECT count(*) FROM inettmp WHERE a < '89.225.196.191'::inet; count diff --git a/contrib/btree_gist/sql/cidr.sql b/contrib/btree_gist/sql/cidr.sql index 9bd77185b96..ec1529e3e04 100644 --- a/contrib/btree_gist/sql/cidr.sql +++ b/contrib/btree_gist/sql/cidr.sql @@ -15,7 +15,7 @@ SELECT count(*) FROM cidrtmp WHERE a >= '121.111.63.82'; SELECT count(*) FROM cidrtmp WHERE a > '121.111.63.82'; -CREATE INDEX cidridx ON cidrtmp USING gist ( a ); +CREATE INDEX cidridx ON cidrtmp USING gist ( a gist_cidr_ops ); SET enable_seqscan=off; diff --git a/contrib/btree_gist/sql/inet.sql b/contrib/btree_gist/sql/inet.sql index 249e8085c3b..0bb73c9d715 100644 --- a/contrib/btree_gist/sql/inet.sql +++ b/contrib/btree_gist/sql/inet.sql @@ -16,7 +16,7 @@ SELECT count(*) FROM inettmp WHERE a >= '89.225.196.191'; SELECT count(*) FROM inettmp WHERE a > '89.225.196.191'; -CREATE INDEX inetidx ON inettmp USING gist ( a ); +CREATE INDEX inetidx ON inettmp USING gist ( a gist_inet_ops ); SET enable_seqscan=off; diff --git a/src/backend/commands/opclasscmds.c b/src/backend/commands/opclasscmds.c index a6dd8eab518..1cf9bc12f0c 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,27 @@ 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, avoid failure in the next stanza by silently making the + * new opclass non-default. Without this kluge, we would fail to load + * pre-v19 definitions of contrib/btree_gist. We can remove it sometime + * in the far future when we don't expect any such definitions to exist. + */ + if (isDefault) + { + 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 +677,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 310f53c5577..4f6946f68a4 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,18 @@ check_and_dump_old_cluster(void) if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1800) check_for_not_null_inheritance(&old_cluster); + /* + * Pre-PG 19, the btree_gist extension contained gist_inet_ops and + * gist_cidr_ops opclasses that did not reliably give correct answers. + * Even if we wanted to support migrating indexes using those forward, we + * can't because they were marked opcdefault = true, which will cause + * pg_dump to dump such indexes with no explicit opclass specification, + * which would do the wrong thing now that the in-core inet_ops opclass is + * marked default. So refuse to upgrade if there are any. + */ + 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 +1735,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 are not supported anymore. Replace them with indexes\n" + "that use the built-in GiST network_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', -- 2.43.7