Re: Fixing the btree_gist inet mess

On 01/08/2025 21:17, Tom Lane wrote:
> 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.

+1

Bunch of ideas and opinions below, but I'm fine with your plan as is too:

> 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
opclassmay 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.

If we implement upgrade the way you propose, all upgraded databases, 
whether it's with pg_dump or pg_upgrade, will switch to using 
network_ops. The only way to get an index with the old btree_gist 
opclass is to specify it explicitly, on v19. I think we might as well 
remove it completely.

The upgrade as proposed will be a hurdle for anyone using the old 
opclass anyway, so people will get some warning to test their 
application after the upgrade. In the worst case people can stick with 
the old version until any issues have been fixed.

> 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.

I wonder if we could move the old opclass's code to core, and somehow 
detect at runtime e.g. by looking at the root page, whether the index 
was built before the upgrade. Hack initGISTstate() to redirect 
everything to the old AM if it was created before the upgrade. The idea 
is that after upgrade, all indexes would appear to be using the new 
opclass if you look at the catalogs, but if it was pg_upgraded from an 
older version, it would actually use the old functions. If you REINDEX, 
it would get recreated in the new format, and would start using the new 
functions. That would be the best user experience, but not sure it's 
worth the effort and all the special hacks.

> 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.

Instead of having the hack in DefineOpClass(), we could have a similar 
hack in pg_dump's dump_opclass(), only in binary upgrade mode.

It seems silly to keep an unmodified btree_gist--1.2.sql in v19, if it 
actually gets installed in a different way. I feel we should truncate 
the history and only include a new btree_gist--1.9.sql in v19.

Putting all that together, you get a more aggressive plan:

- Remove the old opclass entirely
- Remove all old btree_gist--*.sql scripts, start afresh with 
btree_gist--1.9.sql
- Hack pg_dump, in binary upgrade mode, to dump the btree_gist extension 
as simply "CREATE EXTENSION btree_gist;", instead of dumping the 
individual members like it usually does.


Because btree_gist a contrib extension, we have the luxury that we can 
do special hacks like this, in DefineOpClass() or in pg_upgrade. Out of 
core extensions don't have that luxury. Could we generalize this?

I think the common case for extensions is that you'd want them be 
implicitly upgraded to the latest version when you pg_upgrade. So for 
most extensions, you would actually want pg_upgrade's dump and restore 
to just do "CREATE EXTENSION foo;" instead of dumping the individual 
members. But I'm sure there are exceptions. Could we add information to 
the control file about that? For example, list all the older extension 
versions that new default version is binary-compatible with. In 
pg_upgrade, if the new default version is marked as binary-compatible 
with the old installed version, install the new version on the new 
cluster directly. Have support for extension scripts that are run on 
pg_upgrade.

In the btree_gist case, the new version would be marked as 
binary-compatible with all previous extension versions, but there would 
be a pre-upgrade script that throws an error if there are any indexes 
using the old opclass.

> 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.
Yeah. It would be nice to not drop them so that we have some test 
coverage for upgrading them, though. At least if we do more with them 
than just refuse the upgrade.

- Heikki