From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Geoghegan <pg@bowt.ie>
cc: Yoran Heling <contact@yorhel.nl>, pgsql-bugs@lists.postgresql.org
Subject: Re: Dangling operator family after DROP TYPE
In-reply-to: 
 <CAH2-Wz=Jn49axdr=CnR_GDDjggv5jS-P__-OLEmDUxW6aqFDRg@mail.gmail.com>
References: <Z1MVCOh1hprjK5Sf@gmai021> <2830528.1733505356@sss.pgh.pa.us>
 <CAH2-Wz=Jn49axdr=CnR_GDDjggv5jS-P__-OLEmDUxW6aqFDRg@mail.gmail.com>
Comments: In-reply-to Peter Geoghegan <pg@bowt.ie>
	message dated "Fri, 06 Dec 2024 13:36:27 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3057326.1733525482.0@sss.pgh.pa.us>
Content-Transfer-Encoding: 8bit
Date: Fri, 06 Dec 2024 17:51:42 -0500
Message-ID: <3057357.1733525502@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3057357.1733525502%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <3057326.1733525482.1@sss.pgh.pa.us>
Content-Transfer-Encoding: 8bit

Peter Geoghegan <pg@bowt.ie> writes:
> On Fri, Dec 6, 2024 at 12:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Peter, any thoughts about this?

> Nothing much to say about it.

> I would just point out that using the built-in allequalimage function
> is specifically documented as bad practice.

Perhaps, but there are surely plenty of other ways to get into this
situation, when you have support functions whose signatures don't
involve the data type of the indexed column.

Here's a couple of proposed patches.  The first just makes
getObjectDescription robust against dangling
amproclefttype/amprocrighttype links.  (I did the same for pg_amop
entries, though that may be dead code, per comments below.)  I checked
that this allows dropping the busted opfamily.

The second one solves the problem more permanently by adding
dependencies on the types whenever we don't have an indirect
dependency through the operator or function.  Coverage checking shows
that the function case is actually hit in our regression tests (during
creation of contrib GiST opclasses), but the operator case isn't.
I think that the check for operators may be dead code, because AFAICS
from a quick look through opclasscmds.c, assignOperTypes will always
fill lefttype/righttype from the operator's input types and there's
nothing to override that.  But it's at least conceivable that the
index AM's amadjustmembers function would modify the
lefttype/righttype settings.  So I'm inclined to include that code
even if it does nothing today.

I looked at whether we could add a regression test for this, but
all of the cases that presently hit it are contrib extensions.
So there's no way to drop the data type without also dropping the
opfamily (which'd be likewise a member of the extension).  That
probably explains the lack of field reports of this old problem.
We could devise something no doubt, but it doesn't quite seem
worth the trouble and test cycles.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="0001-avoid-failure-with-missing-type.patch";
	charset="us-ascii"
Content-ID: <3057326.1733525482.2@sss.pgh.pa.us>
Content-Description: 0001-avoid-failure-with-missing-type.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/obj=
ectaddress.c
index 7520a98215..be7e4a5dd0 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -3235,6 +3235,12 @@ getObjectDescription(const ObjectAddress *object, b=
ool missing_ok)
 				initStringInfo(&opfam);
 				getOpFamilyDescription(&opfam, amopForm->amopfamily, false);
 =

+				/*
+				 * We use FORMAT_TYPE_ALLOW_INVALID here so as not to fail
+				 * completely if the type links are dangling, which is a form
+				 * of catalog corruption that could occur due to old bugs.
+				 */
+
 				/*------
 				   translator: %d is the operator strategy (a number), the
 				   first two %s's are data type names, the third %s is the
@@ -3242,8 +3248,10 @@ getObjectDescription(const ObjectAddress *object, b=
ool missing_ok)
 				   textual form of the operator with arguments.  */
 				appendStringInfo(&buffer, _("operator %d (%s, %s) of %s: %s"),
 								 amopForm->amopstrategy,
-								 format_type_be(amopForm->amoplefttype),
-								 format_type_be(amopForm->amoprighttype),
+								 format_type_extended(amopForm->amoplefttype,
+													  -1, FORMAT_TYPE_ALLOW_INVALID),
+								 format_type_extended(amopForm->amoprighttype,
+													  -1, FORMAT_TYPE_ALLOW_INVALID),
 								 opfam.data,
 								 format_operator(amopForm->amopopr));
 =

@@ -3292,6 +3300,12 @@ getObjectDescription(const ObjectAddress *object, b=
ool missing_ok)
 				initStringInfo(&opfam);
 				getOpFamilyDescription(&opfam, amprocForm->amprocfamily, false);
 =

+				/*
+				 * We use FORMAT_TYPE_ALLOW_INVALID here so as not to fail
+				 * completely if the type links are dangling, which is a form
+				 * of catalog corruption that could occur due to old bugs.
+				 */
+
 				/*------
 				   translator: %d is the function number, the first two %s's
 				   are data type names, the third %s is the description of the
@@ -3299,8 +3313,10 @@ getObjectDescription(const ObjectAddress *object, b=
ool missing_ok)
 				   function with arguments.  */
 				appendStringInfo(&buffer, _("function %d (%s, %s) of %s: %s"),
 								 amprocForm->amprocnum,
-								 format_type_be(amprocForm->amproclefttype),
-								 format_type_be(amprocForm->amprocrighttype),
+								 format_type_extended(amprocForm->amproclefttype,
+													  -1, FORMAT_TYPE_ALLOW_INVALID),
+								 format_type_extended(amprocForm->amprocrighttype,
+													  -1, FORMAT_TYPE_ALLOW_INVALID),
 								 opfam.data,
 								 format_procedure(amprocForm->amproc));
 =


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="0002-make-missing-dependencies.patch";
	charset="us-ascii"
Content-ID: <3057326.1733525482.3@sss.pgh.pa.us>
Content-Description: 0002-make-missing-dependencies.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/opclasscmds.c b/src/backend/commands/opc=
lasscmds.c
index b8b5c147c5..d024c547cc 100644
--- a/src/backend/commands/opclasscmds.c
+++ b/src/backend/commands/opclasscmds.c
@@ -65,6 +65,7 @@ static void storeOperators(List *opfamilyname, Oid amoid=
, Oid opfamilyoid,
 						   List *operators, bool isAdd);
 static void storeProcedures(List *opfamilyname, Oid amoid, Oid opfamilyoi=
d,
 							List *procedures, bool isAdd);
+static bool typeDepNeeded(Oid typid, OpFamilyMember *member);
 static void dropOperators(List *opfamilyname, Oid amoid, Oid opfamilyoid,
 						  List *operators);
 static void dropProcedures(List *opfamilyname, Oid amoid, Oid opfamilyoid=
,
@@ -1507,6 +1508,29 @@ storeOperators(List *opfamilyname, Oid amoid, Oid o=
pfamilyoid,
 		recordDependencyOn(&myself, &referenced,
 						   op->ref_is_hard ? DEPENDENCY_INTERNAL : DEPENDENCY_AUTO);
 =

+		if (typeDepNeeded(op->lefttype, op))
+		{
+			referenced.classId =3D TypeRelationId;
+			referenced.objectId =3D op->lefttype;
+			referenced.objectSubId =3D 0;
+
+			/* see comments in amapi.h about dependency strength */
+			recordDependencyOn(&myself, &referenced,
+							   op->ref_is_hard ? DEPENDENCY_NORMAL : DEPENDENCY_AUTO);
+		}
+
+		if (op->lefttype !=3D op->righttype &&
+			typeDepNeeded(op->righttype, op))
+		{
+			referenced.classId =3D TypeRelationId;
+			referenced.objectId =3D op->righttype;
+			referenced.objectSubId =3D 0;
+
+			/* see comments in amapi.h about dependency strength */
+			recordDependencyOn(&myself, &referenced,
+							   op->ref_is_hard ? DEPENDENCY_NORMAL : DEPENDENCY_AUTO);
+		}
+
 		/* A search operator also needs a dep on the referenced opfamily */
 		if (OidIsValid(op->sortfamily))
 		{
@@ -1608,6 +1632,29 @@ storeProcedures(List *opfamilyname, Oid amoid, Oid =
opfamilyoid,
 		recordDependencyOn(&myself, &referenced,
 						   proc->ref_is_hard ? DEPENDENCY_INTERNAL : DEPENDENCY_AUTO);
 =

+		if (typeDepNeeded(proc->lefttype, proc))
+		{
+			referenced.classId =3D TypeRelationId;
+			referenced.objectId =3D proc->lefttype;
+			referenced.objectSubId =3D 0;
+
+			/* see comments in amapi.h about dependency strength */
+			recordDependencyOn(&myself, &referenced,
+							   proc->ref_is_hard ? DEPENDENCY_NORMAL : DEPENDENCY_AUTO);
+		}
+
+		if (proc->lefttype !=3D proc->righttype &&
+			typeDepNeeded(proc->righttype, proc))
+		{
+			referenced.classId =3D TypeRelationId;
+			referenced.objectId =3D proc->righttype;
+			referenced.objectSubId =3D 0;
+
+			/* see comments in amapi.h about dependency strength */
+			recordDependencyOn(&myself, &referenced,
+							   proc->ref_is_hard ? DEPENDENCY_NORMAL : DEPENDENCY_AUTO);
+		}
+
 		/* Post create hook of access method procedure */
 		InvokeObjectPostCreateHook(AccessMethodProcedureRelationId,
 								   entryoid, 0);
@@ -1616,6 +1663,57 @@ storeProcedures(List *opfamilyname, Oid amoid, Oid =
opfamilyoid,
 	table_close(rel, RowExclusiveLock);
 }
 =

+/*
+ * Detect whether a pg_amop or pg_amproc entry needs an explicit dependen=
cy
+ * on its lefttype or righttype.
+ *
+ * We make such a dependency unless the entry has an indirect dependency
+ * via its referenced operator or function.  That's nearly always true
+ * for operators, but might well not be true for support functions.
+ */
+static bool
+typeDepNeeded(Oid typid, OpFamilyMember *member)
+{
+	bool		result =3D true;
+
+	/*
+	 * If the type is pinned, we don't need a dependency.  This is a bit of =
a
+	 * layering violation perhaps (recordDependencyOn would ignore the reque=
st
+	 * anyway), but it's a cheap test and will frequently save a syscache
+	 * lookup here.
+	 */
+	if (IsPinnedObject(TypeRelationId, typid))
+		return false;
+
+	/* Nope, so check the input types of the function or operator. */
+	if (member->is_func)
+	{
+		Oid		   *argtypes;
+		int			nargs;
+
+		(void) get_func_signature(member->object, &argtypes, &nargs);
+		for (int i =3D 0; i < nargs; i++)
+		{
+			if (typid =3D=3D argtypes[i])
+			{
+				result =3D false; /* match, no dependency needed */
+				break;
+			}
+		}
+		pfree(argtypes);
+	}
+	else
+	{
+		Oid			lefttype,
+					righttype;
+
+		op_input_types(member->object, &lefttype, &righttype);
+		if (typid =3D=3D lefttype || typid =3D=3D righttype)
+			result =3D false;		/* match, no dependency needed */
+	}
+	return result;
+}
+
 =

 /*
  * Remove operator entries from an opfamily.
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index c51de742ea..a4c0b43aa9 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -76,6 +76,10 @@ typedef enum IndexAMProperty
  * opfamily.  This allows ALTER OPERATOR FAMILY DROP, and causes that to
  * happen automatically if the operator or support func is dropped.  This
  * is the right behavior for inessential ("loose") objects.
+ *
+ * We also make dependencies on lefttype/righttype, of the same strength =
as
+ * the dependency on the operator or support func, unless these dependenc=
ies
+ * are redundant with the dependency on the operator or support func.
  */
 typedef struct OpFamilyMember
 {

------- =_aaaaaaaaaa0--