cannot drop intarray extension

Hi Jian

On Mon, Jun 3, 2024 at 9:14 AM jian he <jian.universality@gmail.com> wrote:

hi.

---- setup
drop table if exist test__int cascade;
create extension intarray;

CREATE TABLE test__int( a int[] );
CREATE INDEX text_idx on test__int using gist (a gist__intbig_ops(siglen =
1));
drop extension intarray cascade;
NOTICE: drop cascades to index text_idx
2024-06-03 11:53:32.629 CST [41165] ERROR: cache lookup failed for
function 17758

Its a bug.

On Mon, Jun 3, 2024 at 12:14 PM jian he <jian.universality@gmail.com> wrote:

hi.

---- setup
drop table if exist test__int cascade;
create extension intarray;

CREATE TABLE test__int( a int[] );
CREATE INDEX text_idx on test__int using gist (a gist__intbig_ops(siglen = 1));
drop extension intarray cascade;
NOTICE: drop cascades to index text_idx
2024-06-03 11:53:32.629 CST [41165] ERROR: cache lookup failed for
function 17758
2024-06-03 11:53:32.629 CST [41165] STATEMENT: drop extension intarray cascade;
ERROR: cache lookup failed for function 17758

------------------------------------------------

extension (ltree, pg_trgm) also have the same problem.

drop table if exists t2 cascade;
CREATE EXTENSION ltree;
CREATE TABLE t2 (t ltree);
create index tstidx on t2 using gist (t gist_ltree_ops(siglen=4));
drop extension ltree cascade;

drop table if exists t3 cascade;
CREATE EXTENSION pg_trgm;
CREATE TABLE t3(t text COLLATE "C");
create index trgm_idx on t3 using gist (t gist_trgm_ops(siglen=1));
drop extension pg_trgm cascade;

------------------------------------------------

extension hstore work as expected, no error.

drop table if exists t1 cascade;
create extension hstore;
CREATE TABLE t1 (h hstore);
create index hidx on t1 using gist(h gist_hstore_ops(siglen=1));
drop extension hstore cascade;

on the master branch. i didn't test on other branches.

On Mon, Jun 3, 2024 at 12:14 PM jian he <jian.universality@gmail.com> wrote:

hi.

---- setup
drop table if exist test__int cascade;
create extension intarray;

CREATE TABLE test__int( a int[] );
CREATE INDEX text_idx on test__int using gist (a gist__intbig_ops(siglen = 1));
drop extension intarray cascade;
NOTICE: drop cascades to index text_idx
2024-06-03 11:53:32.629 CST [41165] ERROR: cache lookup failed for
function 17758
2024-06-03 11:53:32.629 CST [41165] STATEMENT: drop extension intarray cascade;
ERROR: cache lookup failed for function 17758

------------------------------------------------
backtrace info:
index_getprocinfo
#0 index_opclass_options (indrel=0x7faeca727b58, attnum=1,
attoptions=94372901674408, validate=false)
at ../../Desktop/pg_src/src4/postgres/src/backend/access/index/indexam.c:1034
#1 0x000055d4e63a79cb in RelationGetIndexAttOptions
(relation=0x7faeca727b58, copy=false)
at ../../Desktop/pg_src/src4/postgres/src/backend/utils/cache/relcache.c:5872
#2 0x000055d4e639d72d in RelationInitIndexAccessInfo (relation=0x7faeca727b58)
at ../../Desktop/pg_src/src4/postgres/src/backend/utils/cache/relcache.c:1569
#3 0x000055d4e639c5ac in RelationBuildDesc (targetRelId=24582, insertIt=true)
at ../../Desktop/pg_src/src4/postgres/src/backend/utils/cache/relcache.c:1207
#4 0x000055d4e639e9ce in RelationIdGetRelation (relationId=24582)
at ../../Desktop/pg_src/src4/postgres/src/backend/utils/cache/relcache.c:2115
#5 0x000055d4e5a412fd in relation_open (relationId=24582, lockmode=8)
at ../../Desktop/pg_src/src4/postgres/src/backend/access/common/relation.c:58
#6 0x000055d4e5ae6a06 in index_open (relationId=24582, lockmode=8)
at ../../Desktop/pg_src/src4/postgres/src/backend/access/index/indexam.c:137
#7 0x000055d4e5be61b8 in index_drop (indexId=24582, concurrent=false,
concurrent_lock_mode=false)
at ../../Desktop/pg_src/src4/postgres/src/backend/catalog/index.c:2156
------------------------
i guess it's because we first dropped the function g_intbig_options

in this context, the index "text_idx" has a normal dependency with pg_opclass.
but `drop extension intarray cascade;`,
CASCADE means that we drop the pg_opclass and pg_opclass's inner dependency
first, then drop the index.

while drop index (sub functions
RelationGetIndexAttOptions,index_opclass_options, index_getprocinfo)
requires that pg_opclass and its inner dependencies (namely
g_intbig_options, g_int_options) are not dropped first.

in deleteObjectsInList, under certain conditions trying to sort the to
be deleted object list
by just using sort_object_addresses seems to work,
but it looks like a hack.
maybe the proper fix would be in findDependentObjects.

On Fri, Jun 07, 2024 at 11:32:14AM +0800, jian he wrote:

in deleteObjectsInList, under certain conditions trying to sort the to
be deleted object list
by just using sort_object_addresses seems to work,
but it looks like a hack.
maybe the proper fix would be in findDependentObjects.

@@ -1459,6 +1459,7 @@ RemoveRelations(DropStmt *drop)
[...]
-       performMultipleDeletions(objects, drop->behavior, flags);
+       if (list_length(drop->objects) > 1)
+               sortable = false;

I have not studied the patch in details, but this looks
overcomplicated to me. All the callers of performMultipleDeletions
pass down sortable as true, while deleteObjectsInList() uses this
argument to avoid the sorting on nested calls. It seems to me that
this could be simpler.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Fri, Jun 07, 2024 at 11:32:14AM +0800, jian he wrote:

in deleteObjectsInList, under certain conditions trying to sort the to
be deleted object list
by just using sort_object_addresses seems to work,
but it looks like a hack.
maybe the proper fix would be in findDependentObjects.

I have not studied the patch in details, but this looks
overcomplicated to me.

I dunno about overcomplicated, but it's fundamentally the wrong thing:
it won't do much except move the problem from this example to other
example(s). The difficulty is precisely that we cannot simply delete
objects in reverse OID order and expect that to be OK. It appears to
work in simple cases because reverse OID order usually means deleting
newest objects first, and that usually means dropping depender objects
before dependees --- but dependencies added as a result of later ALTER
commands may not be honored correctly. Not to mention that you can
lose if an OID wraparound happened during the sequence of object
creations.

In the case at hand, the reason we're having trouble with
g_intbig_options() is that the sequence of extension scripts
run by CREATE EXTENSION creates the gist__intbig_ops opfamily
first, and then creates g_intbig_options() and attaches it to
the opfamily later (in intarray--1.2--1.3.sql). So g_intbig_options()
has a larger OID than the opclass that the index depends on.
In DROP EXTENSION, the first level of findDependentObjects() finds
all the direct dependencies (members) of the extension, and then
sorts them by OID descending, concluding that g_intbig_options()
can be dropped before the opclass. Subsequent recursive levels
will find the index and recognize that it must be dropped before
the opclass --- but this fails to account for the fact that we'd
better drop the opclass before any of the functions it depends on.
At some point along the line, we will come across the dependency
that says so; but we don't do anything in response, because if
findDependentObjects() sees that the current object is already
in targetObjects then it thinks it's done.

I think when I wrote this code I was assuming that the dependency-
order traversal performed by findDependentObjects() was sufficient
to guarantee producing a safe deletion order, but it's now obvious
that that's not so. At minimum, when findDependentObjects() finds
that a dependency exists on an object that's already in targetObjects,
it'd need to do something about moving that object to after the one
it's working on. But I doubt we can fix it with just that, because
that won't be enough to handle indirect dependencies.

It looks to me that the only real fix will require performing a
topological sort, similar to what pg_dump does, to produce a safe
deletion order that honors all the direct and indirect dependencies
found by findDependentObjects().

An open question is whether we will need dependency-loop-breaking
logic, or whether the hackery done in findDependentObjects() is
sufficient to ensure that we can assume there are no loops in the
dependencies it chooses to output. It might be a better idea to
get rid of that logic and instead have explicit loop-breaking like
the way pg_dump does it.

It's also tempting to wonder if we can share code for this with
pg_dump. The TopoSort function alone is not that big, but if we
have to duplicate the loop-breaking logic it'd get annoying.

Anyway, this is a very long-standing problem and I don't think
we should try to solve it in a rush.

regards, tom lane