Fixing the btree_gist inet mess - Mailing list pgsql-hackers

As we've known for years[1][2][3], contrib/btree_gist's opclasses
for inet/cidr columns are fundamentally broken: they rely on the
planner's convert_network_to_scalar() function, which was only
ever intended to give approximate results, so you get the wrong
answers in edge cases.  There isn't anything that can be done
about that without breaking on-disk compatibility for such indexes,
so we haven't tried.  What we did do some time ago was to implement
a hopefully-correct, in-core gist network_ops opclass to replace the
btree_gist opclasses.  But people are still using the btree_gist
opclasses, because those are marked default and the in-core opclass
isn't.

It's past time to move this problem along and try to get out of the
business of encouraging use of known-broken code.  I propose that
for v19, we should flip the opcdefault status so that network_ops is
marked default and the btree_gist opclasses are not.  This will be
enough to ensure that network_ops is used unless the user explicitly
specifies to do differently.  I don't think we should go further than
that yet (ie, not actively disable the btree_gist code) for a couple
of reasons: (1) this step is messy enough already, and (2) given the
current situation, the in-core network_ops opclass may be less well
tested than one would like.  So I don't think we have enough evidence
to decide that we can summarily force everyone onto it; broken or not,
there haven't been that many complaints about btree_gist's opclasses.

Having done this, the effects of a plain pg_dump from v18- and restore
into v19+ will be to recreate GiST indexes on inet/cidr columns using
network_ops even if they were previously using btree_gist.  That will
happen because in v18-, those opclasses were marked opcdefault and
pg_dump intentionally omits the explicit opclass specification in that
case.  So that works the way we want.

pg_upgrade is more of a problem, because its invocation of pg_dump
will also omit the explicit opclass specification, resulting in the
new server thinking that the index uses network_ops while the on-disk
data isn't compatible with that.  We can't really change that pg_dump
behavior, because that aspect is managed inside the old server's
pg_get_indexdef() function.  The only solution I can see is for
pg_upgrade to refuse to upgrade indexes that use those opclasses.
We can tell users to replace them with network_ops indexes before
upgrading --- that's possible in 9.4 and later, so it should be
a good enough answer for almost everybody.

The attached draft patch implements these ideas and seems to do
the right things in testing.  It's worth remarking on the way
that I did the "mark the btree_gist opclasses not-default" part:
I hacked up DefineOpClass() to ignore the DEFAULT specification if
the opclass being created has the right name and input data type.
That certainly has a foul odor about it, but the alternatives seem
worse.  We can't simply add a btree_gist update step to remove
the DEFAULT setting, because btree_gist--1.2.sql will already have
failed as a consequence of trying to create a default opclass when
there already is one.  Modifying btree_gist--1.2.sql to remove the
DEFAULT markings might be safe, but it goes against our longstanding
rule that extension scripts don't change once shipped, and I'm not
entirely sure that there aren't bad consequences if we break that
rule.  (I did go as far as to add a comment to it about what will
really happen.)  Moreover, even if we were willing to risk changing
btree_gist--1.2.sql, that's not enough: pg_upgrade would still fail,
because it dumps extensions by content, and what it will see in the
old installation is btree_gist opclasses that are marked default.
So hacking up DefineOpClass() can solve both the
normal-extension-install case and the pg_upgrade case for not a lot
of code, and I'm not seeing another way that's better.

There are a couple of loose ends still to be dealt with.  We need
to say something about this in btree-gist.sgml, but I've not
attempted to write that text yet.  Also, I expect that
cross-version-upgrade testing will spit up on the inet/cidr indexes
created by btree_gist's regression tests.  There's probably
nothing that can be done about the latter except to teach
AdjustUpgrade.pm to drop those indexes from the old installation.

Thoughts?

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/201010112055.o9BKtZf7011251%40wwwmaster.postgresql.org
[2] https://www.postgresql.org/message-id/flat/7891efc1-8378-2cf2-617b-4143848ec895%40proxel.se
[3] https://www.postgresql.org/message-id/flat/19000-2525470d200672ab%40postgresql.org

From 9b594f81fb3b1ebe6ea79d14d4039778e9e523a2 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
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