Re: Rethinking opclass member checks and dependency strength

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> On Wed, Aug 7, 2019 at 7:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This leads to the thought that maybe we could put some intelligence
>> into an index-AM-specific callback instead.  For example, for btree
>> and hash the appropriate rule is probably that cross-type operators
>> and functions should be tied to the opfamily while single-type
>> members are internally tied to the associated opclass.  For GiST,
>> GIN, and SPGiST it's not clear to me that *any* operator deserves
>> an INTERNAL dependency; only the implementation functions do.
>> 
>> Furthermore, if we had an AM callback that were charged with
>> deciding the right dependency links for all the operators/functions,
>> we could also have it do some validity checking on those things,
>> thus moving some of the checks made by amvalidate into a more
>> useful place.

> +1, sounds like a plan for me.

Here's a preliminary patch along these lines.  It adds an AM callback
that can adjust the dependency types before they're entered into
pg_depend.  There's a lot of stuff that's open for debate and/or
remains to be done:

* Is the parameter list of amcheckmembers() sufficient, or should we
pass more info (if so, what)?  In principle, the AM can always look
up anything else it needs to know from the provided OIDs, but that
would be cumbersome if many AMs need the same info.

* Do we need any more flexibility in the set of ways that the pg_depend
entries can be set up than what I've provided here?

* Are the specific ways that the entries are getting set up appropriate?
Note in particular that I left btree/hash alone, feeling that the default
(historical) behavior was designed for them and is not unreasonable; but
maybe we should switch them to the cross-type-vs-not-cross-type behavior
proposed above.  Also I didn't touch BRIN because I don't know enough
about it to be sure what would be best, and I didn't touch contrib/bloom
because I don't care too much about it.

* I didn't add any actual error checking to the checkmembers functions.
I figure that can be done in a followup patch, and it'll just be tedious
boilerplate anyway.

* I refactored things a little bit in opclasscmds.c, mostly by adding
an is_func boolean to OpFamilyMember and getting rid of parameters
equivalent to that.  This is based on the thought that AMs might prefer
to process the structs based on such a flag rather than by keeping them
in separate lists.  We could go further and merge the operator and
function structs into one list, forcing the is_func flag to be used;
but I'm not sure whether that'd be an improvement.

* I'm not at all impressed with the name, location, or concept of
opfam_internal.h.  I think we should get rid of that header and put
the OpFamilyMember struct somewhere else.  Given that this patch
makes it part of the AM API, it wouldn't be unreasonable to move it
to amapi.h.  But I've not done that here.

I'll add this to the upcoming commitfest.

            regards, tom lane

diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index cc16709..5f664ca 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -134,6 +134,7 @@ blhandler(PG_FUNCTION_ARGS)
     amroutine->amproperty = NULL;
     amroutine->ambuildphasename = NULL;
     amroutine->amvalidate = blvalidate;
+    amroutine->amcheckmembers = NULL;
     amroutine->ambeginscan = blbeginscan;
     amroutine->amrescan = blrescan;
     amroutine->amgettuple = NULL;
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index dd54c68..2815098 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -137,6 +137,7 @@ typedef struct IndexAmRoutine
     amproperty_function amproperty;     /* can be NULL */
     ambuildphasename_function ambuildphasename;   /* can be NULL */
     amvalidate_function amvalidate;
+    amcheckmembers_function amcheckmembers; /* can be NULL */
     ambeginscan_function ambeginscan;
     amrescan_function amrescan;
     amgettuple_function amgettuple;     /* can be NULL */
@@ -496,7 +497,47 @@ amvalidate (Oid opclassoid);
    the access method can reasonably do that.  For example, this might include
    testing that all required support functions are provided.
    The <function>amvalidate</function> function must return false if the opclass is
-   invalid.  Problems should be reported with <function>ereport</function> messages.
+   invalid.  Problems should be reported with <function>ereport</function>
+   messages, typically at <literal>INFO</literal> level.
+  </para>
+
+  <para>
+<programlisting>
+void
+amcheckmembers (Oid opfamilyoid,
+                Oid opclassoid,
+                List *operators,
+                List *functions);
+</programlisting>
+   Validate proposed operator and function members of an operator family,
+   so far as the access method can reasonably do that, and set their
+   dependency types if the default is not satisfactory.  This is called
+   during <command>CREATE OPERATOR CLASS</command> and during
+   <command>ALTER OPERATOR FAMILY ADD</command>; in the latter
+   case <parameter>opclassoid</parameter> is <literal>InvalidOid</literal>.
+   The <type>List</type> arguments are lists
+   of <structname>OpFamilyMember</structname> structs, as defined
+   in <filename>opfam_internal.h</filename>.
+
+   Tests done by this function will typically be a subset of those
+   performed by <function>amvalidate</function>,
+   since <function>amcheckmembers</function> cannot assume that it is
+   seeing a complete set of members.  For example, it would be reasonable
+   to check the signature of a support function, but not to check whether
+   all required support functions are provided.  Any problems can be
+   reported by throwing an error.
+
+   The dependency-related fields of
+   the <structname>OpFamilyMember</structname> structs are initialized by
+   the core code to create hard dependencies on the opclass if this
+   is <command>CREATE OPERATOR CLASS</command>, or soft dependencies on the
+   opfamily if this is <command>ALTER OPERATOR FAMILY ADD</command>.
+   <function>amcheckmembers</function> can adjust these fields if some other
+   behavior is more appropriate.  For example, GIN, GiST, and SP-GiST
+   always set operator members to have soft dependencies on the opfamily,
+   since the connection between an operator and an opclass is relatively
+   weak in these index types; so it is reasonable to allow operator members
+   to be added and removed freely.
   </para>


diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index ae7b729..129b6b5 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -114,6 +114,7 @@ brinhandler(PG_FUNCTION_ARGS)
     amroutine->amproperty = NULL;
     amroutine->ambuildphasename = NULL;
     amroutine->amvalidate = brinvalidate;
+    amroutine->amcheckmembers = NULL;
     amroutine->ambeginscan = brinbeginscan;
     amroutine->amrescan = brinrescan;
     amroutine->amgettuple = NULL;
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index cf9699a..d4f4851 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -66,6 +66,7 @@ ginhandler(PG_FUNCTION_ARGS)
     amroutine->amproperty = NULL;
     amroutine->ambuildphasename = NULL;
     amroutine->amvalidate = ginvalidate;
+    amroutine->amcheckmembers = gincheckmembers;
     amroutine->ambeginscan = ginbeginscan;
     amroutine->amrescan = ginrescan;
     amroutine->amgettuple = NULL;
diff --git a/src/backend/access/gin/ginvalidate.c b/src/backend/access/gin/ginvalidate.c
index 63bd7f2..3a5db93 100644
--- a/src/backend/access/gin/ginvalidate.c
+++ b/src/backend/access/gin/ginvalidate.c
@@ -16,6 +16,7 @@
 #include "access/amvalidate.h"
 #include "access/gin_private.h"
 #include "access/htup_details.h"
+#include "catalog/opfam_internal.h"
 #include "catalog/pg_amop.h"
 #include "catalog/pg_amproc.h"
 #include "catalog/pg_opclass.h"
@@ -268,3 +269,37 @@ ginvalidate(Oid opclassoid)

     return result;
 }
+
+/*
+ * Prechecking function for adding operators/functions to a GIN opfamily.
+ */
+void
+gincheckmembers(Oid opfamilyoid,
+                Oid opclassoid,
+                List *operators,
+                List *functions)
+{
+    ListCell   *lc;
+
+    /*
+     * Operator members of a GIN opfamily should never have hard dependencies,
+     * since their connection to the opfamily depends only on what the support
+     * functions think, and that can be altered.  For consistency, we make all
+     * soft dependencies point to the opfamily, though a soft dependency on
+     * the opclass would work as well in the CREATE OPERATOR CLASS case.
+     */
+    foreach(lc, operators)
+    {
+        OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+        op->ref_is_hard = false;
+        op->ref_is_family = true;
+        op->refobjid = opfamilyoid;
+    }
+
+    /*
+     * We leave the functions' dependencies alone; typically, support
+     * functions would be added only in CREATE OPERATOR CLASS, and the default
+     * hard dependency is appropriate.
+     */
+}
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 0cc8791..e4cf0e8 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -88,6 +88,7 @@ gisthandler(PG_FUNCTION_ARGS)
     amroutine->amproperty = gistproperty;
     amroutine->ambuildphasename = NULL;
     amroutine->amvalidate = gistvalidate;
+    amroutine->amcheckmembers = gistcheckmembers;
     amroutine->ambeginscan = gistbeginscan;
     amroutine->amrescan = gistrescan;
     amroutine->amgettuple = gistgettuple;
diff --git a/src/backend/access/gist/gistvalidate.c b/src/backend/access/gist/gistvalidate.c
index dfc1a87..bf79b4c 100644
--- a/src/backend/access/gist/gistvalidate.c
+++ b/src/backend/access/gist/gistvalidate.c
@@ -16,6 +16,7 @@
 #include "access/amvalidate.h"
 #include "access/gist_private.h"
 #include "access/htup_details.h"
+#include "catalog/opfam_internal.h"
 #include "catalog/pg_amop.h"
 #include "catalog/pg_amproc.h"
 #include "catalog/pg_opclass.h"
@@ -275,3 +276,38 @@ gistvalidate(Oid opclassoid)

     return result;
 }
+
+/*
+ * Prechecking function for adding operators/functions to a GiST opfamily.
+ */
+void
+gistcheckmembers(Oid opfamilyoid,
+                 Oid opclassoid,
+                 List *operators,
+                 List *functions)
+{
+    ListCell   *lc;
+
+    /*
+     * Operator members of a GiST opfamily should never have hard
+     * dependencies, since their connection to the opfamily depends only on
+     * what the support functions think, and that can be altered.  For
+     * consistency, we make all soft dependencies point to the opfamily,
+     * though a soft dependency on the opclass would work as well in the
+     * CREATE OPERATOR CLASS case.
+     */
+    foreach(lc, operators)
+    {
+        OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+        op->ref_is_hard = false;
+        op->ref_is_family = true;
+        op->refobjid = opfamilyoid;
+    }
+
+    /*
+     * We leave the functions' dependencies alone; typically, support
+     * functions would be added only in CREATE OPERATOR CLASS, and the default
+     * hard dependency is appropriate.
+     */
+}
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 5cc30da..e5e471a 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -87,6 +87,7 @@ hashhandler(PG_FUNCTION_ARGS)
     amroutine->amproperty = NULL;
     amroutine->ambuildphasename = NULL;
     amroutine->amvalidate = hashvalidate;
+    amroutine->amcheckmembers = NULL;
     amroutine->ambeginscan = hashbeginscan;
     amroutine->amrescan = hashrescan;
     amroutine->amgettuple = hashgettuple;
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4cfd528..1729709 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -136,6 +136,7 @@ bthandler(PG_FUNCTION_ARGS)
     amroutine->amproperty = btproperty;
     amroutine->ambuildphasename = btbuildphasename;
     amroutine->amvalidate = btvalidate;
+    amroutine->amcheckmembers = NULL;
     amroutine->ambeginscan = btbeginscan;
     amroutine->amrescan = btrescan;
     amroutine->amgettuple = btgettuple;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 45472db..afb416e 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -69,6 +69,7 @@ spghandler(PG_FUNCTION_ARGS)
     amroutine->amproperty = spgproperty;
     amroutine->ambuildphasename = NULL;
     amroutine->amvalidate = spgvalidate;
+    amroutine->amcheckmembers = spgcheckmembers;
     amroutine->ambeginscan = spgbeginscan;
     amroutine->amrescan = spgrescan;
     amroutine->amgettuple = spggettuple;
diff --git a/src/backend/access/spgist/spgvalidate.c b/src/backend/access/spgist/spgvalidate.c
index 4b9fdbd..94e8562 100644
--- a/src/backend/access/spgist/spgvalidate.c
+++ b/src/backend/access/spgist/spgvalidate.c
@@ -16,6 +16,7 @@
 #include "access/amvalidate.h"
 #include "access/htup_details.h"
 #include "access/spgist_private.h"
+#include "catalog/opfam_internal.h"
 #include "catalog/pg_amop.h"
 #include "catalog/pg_amproc.h"
 #include "catalog/pg_opclass.h"
@@ -298,3 +299,38 @@ spgvalidate(Oid opclassoid)

     return result;
 }
+
+/*
+ * Prechecking function for adding operators/functions to an SP-GiST opfamily.
+ */
+void
+spgcheckmembers(Oid opfamilyoid,
+                Oid opclassoid,
+                List *operators,
+                List *functions)
+{
+    ListCell   *lc;
+
+    /*
+     * Operator members of an SP-GiST opfamily should never have hard
+     * dependencies, since their connection to the opfamily depends only on
+     * what the support functions think, and that can be altered.  For
+     * consistency, we make all soft dependencies point to the opfamily,
+     * though a soft dependency on the opclass would work as well in the
+     * CREATE OPERATOR CLASS case.
+     */
+    foreach(lc, operators)
+    {
+        OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+        op->ref_is_hard = false;
+        op->ref_is_family = true;
+        op->refobjid = opfamilyoid;
+    }
+
+    /*
+     * We leave the functions' dependencies alone; typically, support
+     * functions would be added only in CREATE OPERATOR CLASS, and the default
+     * hard dependency is appropriate.
+     */
+}
diff --git a/src/backend/commands/opclasscmds.c b/src/backend/commands/opclasscmds.c
index 6a1ccde..6396905 100644
--- a/src/backend/commands/opclasscmds.c
+++ b/src/backend/commands/opclasscmds.c
@@ -62,12 +62,10 @@ static void AlterOpFamilyDrop(AlterOpFamilyStmt *stmt,
 static void processTypesSpec(List *args, Oid *lefttype, Oid *righttype);
 static void assignOperTypes(OpFamilyMember *member, Oid amoid, Oid typeoid);
 static void assignProcTypes(OpFamilyMember *member, Oid amoid, Oid typeoid);
-static void addFamilyMember(List **list, OpFamilyMember *member, bool isProc);
-static void storeOperators(List *opfamilyname, Oid amoid,
-                           Oid opfamilyoid, Oid opclassoid,
+static void addFamilyMember(List **list, OpFamilyMember *member);
+static void storeOperators(List *opfamilyname, Oid amoid, Oid opfamilyoid,
                            List *operators, bool isAdd);
-static void storeProcedures(List *opfamilyname, Oid amoid,
-                            Oid opfamilyoid, Oid opclassoid,
+static void storeProcedures(List *opfamilyname, Oid amoid, Oid opfamilyoid,
                             List *procedures, bool isAdd);
 static void dropOperators(List *opfamilyname, Oid amoid, Oid opfamilyoid,
                           List *operators);
@@ -516,11 +514,12 @@ DefineOpClass(CreateOpClassStmt *stmt)

                 /* Save the info */
                 member = (OpFamilyMember *) palloc0(sizeof(OpFamilyMember));
+                member->is_func = false;
                 member->object = operOid;
                 member->number = item->number;
                 member->sortfamily = sortfamilyOid;
                 assignOperTypes(member, amoid, typeoid);
-                addFamilyMember(&operators, member, false);
+                addFamilyMember(&operators, member);
                 break;
             case OPCLASS_ITEM_FUNCTION:
                 if (item->number <= 0 || item->number > maxProcNumber)
@@ -540,6 +539,7 @@ DefineOpClass(CreateOpClassStmt *stmt)

                 /* Save the info */
                 member = (OpFamilyMember *) palloc0(sizeof(OpFamilyMember));
+                member->is_func = true;
                 member->object = funcOid;
                 member->number = item->number;

@@ -549,7 +549,7 @@ DefineOpClass(CreateOpClassStmt *stmt)
                                      &member->lefttype, &member->righttype);

                 assignProcTypes(member, amoid, typeoid);
-                addFamilyMember(&procedures, member, true);
+                addFamilyMember(&procedures, member);
                 break;
             case OPCLASS_ITEM_STORAGETYPE:
                 if (OidIsValid(storageoid))
@@ -662,13 +662,45 @@ DefineOpClass(CreateOpClassStmt *stmt)
     heap_freetuple(tup);

     /*
+     * Now that we have the opclass OID, set up default dependency info for
+     * the pg_amop and pg_amproc entries.  Historically, CREATE OPERATOR CLASS
+     * has created hard dependencies on the opclass, so that's what we use.
+     */
+    foreach(l, operators)
+    {
+        OpFamilyMember *op = (OpFamilyMember *) lfirst(l);
+
+        op->ref_is_hard = true;
+        op->ref_is_family = false;
+        op->refobjid = opclassoid;
+    }
+    foreach(l, procedures)
+    {
+        OpFamilyMember *proc = (OpFamilyMember *) lfirst(l);
+
+        proc->ref_is_hard = true;
+        proc->ref_is_family = false;
+        proc->refobjid = opclassoid;
+    }
+
+    /*
+     * Let the index AM editorialize on the dependency choices.  It could also
+     * do further validation on the operators and functions, if it likes.
+     */
+    if (amroutine->amcheckmembers)
+        amroutine->amcheckmembers(opfamilyoid,
+                                  opclassoid,
+                                  operators,
+                                  procedures);
+
+    /*
      * Now add tuples to pg_amop and pg_amproc tying in the operators and
      * functions.  Dependencies on them are inserted, too.
      */
     storeOperators(stmt->opfamilyname, amoid, opfamilyoid,
-                   opclassoid, operators, false);
+                   operators, false);
     storeProcedures(stmt->opfamilyname, amoid, opfamilyoid,
-                    opclassoid, procedures, false);
+                    procedures, false);

     /* let event triggers know what happened */
     EventTriggerCollectCreateOpClass(stmt, opclassoid, operators, procedures);
@@ -837,6 +869,7 @@ static void
 AlterOpFamilyAdd(AlterOpFamilyStmt *stmt, Oid amoid, Oid opfamilyoid,
                  int maxOpNumber, int maxProcNumber, List *items)
 {
+    IndexAmRoutine *amroutine = GetIndexAmRoutineByAmId(amoid, false);
     List       *operators;        /* OpFamilyMember list for operators */
     List       *procedures;        /* OpFamilyMember list for support procs */
     ListCell   *l;
@@ -895,11 +928,17 @@ AlterOpFamilyAdd(AlterOpFamilyStmt *stmt, Oid amoid, Oid opfamilyoid,

                 /* Save the info */
                 member = (OpFamilyMember *) palloc0(sizeof(OpFamilyMember));
+                member->is_func = false;
                 member->object = operOid;
                 member->number = item->number;
                 member->sortfamily = sortfamilyOid;
+                /* We can set up dependency fields immediately */
+                /* Historically, ALTER ADD has created soft dependencies */
+                member->ref_is_hard = false;
+                member->ref_is_family = true;
+                member->refobjid = opfamilyoid;
                 assignOperTypes(member, amoid, InvalidOid);
-                addFamilyMember(&operators, member, false);
+                addFamilyMember(&operators, member);
                 break;
             case OPCLASS_ITEM_FUNCTION:
                 if (item->number <= 0 || item->number > maxProcNumber)
@@ -919,8 +958,14 @@ AlterOpFamilyAdd(AlterOpFamilyStmt *stmt, Oid amoid, Oid opfamilyoid,

                 /* Save the info */
                 member = (OpFamilyMember *) palloc0(sizeof(OpFamilyMember));
+                member->is_func = true;
                 member->object = funcOid;
                 member->number = item->number;
+                /* We can set up dependency fields immediately */
+                /* Historically, ALTER ADD has created soft dependencies */
+                member->ref_is_hard = false;
+                member->ref_is_family = true;
+                member->refobjid = opfamilyoid;

                 /* allow overriding of the function's actual arg types */
                 if (item->class_args)
@@ -928,7 +973,7 @@ AlterOpFamilyAdd(AlterOpFamilyStmt *stmt, Oid amoid, Oid opfamilyoid,
                                      &member->lefttype, &member->righttype);

                 assignProcTypes(member, amoid, InvalidOid);
-                addFamilyMember(&procedures, member, true);
+                addFamilyMember(&procedures, member);
                 break;
             case OPCLASS_ITEM_STORAGETYPE:
                 ereport(ERROR,
@@ -942,13 +987,23 @@ AlterOpFamilyAdd(AlterOpFamilyStmt *stmt, Oid amoid, Oid opfamilyoid,
     }

     /*
+     * Let the index AM editorialize on the dependency choices.  It could also
+     * do further validation on the operators and functions, if it likes.
+     */
+    if (amroutine->amcheckmembers)
+        amroutine->amcheckmembers(opfamilyoid,
+                                  InvalidOid,    /* no specific opclass */
+                                  operators,
+                                  procedures);
+
+    /*
      * Add tuples to pg_amop and pg_amproc tying in the operators and
      * functions.  Dependencies on them are inserted, too.
      */
     storeOperators(stmt->opfamilyname, amoid, opfamilyoid,
-                   InvalidOid, operators, true);
+                   operators, true);
     storeProcedures(stmt->opfamilyname, amoid, opfamilyoid,
-                    InvalidOid, procedures, true);
+                    procedures, true);

     /* make information available to event triggers */
     EventTriggerCollectAlterOpFam(stmt, opfamilyoid,
@@ -991,10 +1046,11 @@ AlterOpFamilyDrop(AlterOpFamilyStmt *stmt, Oid amoid, Oid opfamilyoid,
                 processTypesSpec(item->class_args, &lefttype, &righttype);
                 /* Save the info */
                 member = (OpFamilyMember *) palloc0(sizeof(OpFamilyMember));
+                member->is_func = false;
                 member->number = item->number;
                 member->lefttype = lefttype;
                 member->righttype = righttype;
-                addFamilyMember(&operators, member, false);
+                addFamilyMember(&operators, member);
                 break;
             case OPCLASS_ITEM_FUNCTION:
                 if (item->number <= 0 || item->number > maxProcNumber)
@@ -1006,10 +1062,11 @@ AlterOpFamilyDrop(AlterOpFamilyStmt *stmt, Oid amoid, Oid opfamilyoid,
                 processTypesSpec(item->class_args, &lefttype, &righttype);
                 /* Save the info */
                 member = (OpFamilyMember *) palloc0(sizeof(OpFamilyMember));
+                member->is_func = true;
                 member->number = item->number;
                 member->lefttype = lefttype;
                 member->righttype = righttype;
-                addFamilyMember(&procedures, member, true);
+                addFamilyMember(&procedures, member);
                 break;
             case OPCLASS_ITEM_STORAGETYPE:
                 /* grammar prevents this from appearing */
@@ -1264,7 +1321,7 @@ assignProcTypes(OpFamilyMember *member, Oid amoid, Oid typeoid)
  * duplicated strategy or proc number.
  */
 static void
-addFamilyMember(List **list, OpFamilyMember *member, bool isProc)
+addFamilyMember(List **list, OpFamilyMember *member)
 {
     ListCell   *l;

@@ -1276,7 +1333,7 @@ addFamilyMember(List **list, OpFamilyMember *member, bool isProc)
             old->lefttype == member->lefttype &&
             old->righttype == member->righttype)
         {
-            if (isProc)
+            if (member->is_func)
                 ereport(ERROR,
                         (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                          errmsg("function number %d for (%s,%s) appears more than once",
@@ -1298,13 +1355,10 @@ addFamilyMember(List **list, OpFamilyMember *member, bool isProc)
 /*
  * Dump the operators to pg_amop
  *
- * We also make dependency entries in pg_depend for the opfamily entries.
- * If opclassoid is valid then make an INTERNAL dependency on that opclass,
- * else make an AUTO dependency on the opfamily.
+ * We also make dependency entries in pg_depend for the pg_amop entries.
  */
 static void
-storeOperators(List *opfamilyname, Oid amoid,
-               Oid opfamilyoid, Oid opclassoid,
+storeOperators(List *opfamilyname, Oid amoid, Oid opfamilyoid,
                List *operators, bool isAdd)
 {
     Relation    rel;
@@ -1374,28 +1428,17 @@ storeOperators(List *opfamilyname, Oid amoid,
         referenced.objectId = op->object;
         referenced.objectSubId = 0;

-        if (OidIsValid(opclassoid))
-        {
-            /* if contained in an opclass, use a NORMAL dep on operator */
-            recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
+        /* see comments in opfam_internal.h about dependency strength */
+        recordDependencyOn(&myself, &referenced,
+                           op->ref_is_hard ? DEPENDENCY_NORMAL : DEPENDENCY_AUTO);

-            /* ... and an INTERNAL dep on the opclass */
-            referenced.classId = OperatorClassRelationId;
-            referenced.objectId = opclassoid;
-            referenced.objectSubId = 0;
-            recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
-        }
-        else
-        {
-            /* if "loose" in the opfamily, use a AUTO dep on operator */
-            recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
+        referenced.classId = op->ref_is_family ? OperatorFamilyRelationId :
+            OperatorClassRelationId;
+        referenced.objectId = op->refobjid;
+        referenced.objectSubId = 0;

-            /* ... and an AUTO dep on the opfamily */
-            referenced.classId = OperatorFamilyRelationId;
-            referenced.objectId = opfamilyoid;
-            referenced.objectSubId = 0;
-            recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
-        }
+        recordDependencyOn(&myself, &referenced,
+                           op->ref_is_hard ? DEPENDENCY_INTERNAL : DEPENDENCY_AUTO);

         /* A search operator also needs a dep on the referenced opfamily */
         if (OidIsValid(op->sortfamily))
@@ -1403,8 +1446,11 @@ storeOperators(List *opfamilyname, Oid amoid,
             referenced.classId = OperatorFamilyRelationId;
             referenced.objectId = op->sortfamily;
             referenced.objectSubId = 0;
-            recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
+
+            recordDependencyOn(&myself, &referenced,
+                               op->ref_is_hard ? DEPENDENCY_NORMAL : DEPENDENCY_AUTO);
         }
+
         /* Post create hook of this access method operator */
         InvokeObjectPostCreateHook(AccessMethodOperatorRelationId,
                                    entryoid, 0);
@@ -1416,13 +1462,10 @@ storeOperators(List *opfamilyname, Oid amoid,
 /*
  * Dump the procedures (support routines) to pg_amproc
  *
- * We also make dependency entries in pg_depend for the opfamily entries.
- * If opclassoid is valid then make an INTERNAL dependency on that opclass,
- * else make an AUTO dependency on the opfamily.
+ * We also make dependency entries in pg_depend for the pg_amproc entries.
  */
 static void
-storeProcedures(List *opfamilyname, Oid amoid,
-                Oid opfamilyoid, Oid opclassoid,
+storeProcedures(List *opfamilyname, Oid amoid, Oid opfamilyoid,
                 List *procedures, bool isAdd)
 {
     Relation    rel;
@@ -1486,28 +1529,18 @@ storeProcedures(List *opfamilyname, Oid amoid,
         referenced.objectId = proc->object;
         referenced.objectSubId = 0;

-        if (OidIsValid(opclassoid))
-        {
-            /* if contained in an opclass, use a NORMAL dep on procedure */
-            recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
+        /* see comments in opfam_internal.h about dependency strength */
+        recordDependencyOn(&myself, &referenced,
+                           proc->ref_is_hard ? DEPENDENCY_NORMAL : DEPENDENCY_AUTO);

-            /* ... and an INTERNAL dep on the opclass */
-            referenced.classId = OperatorClassRelationId;
-            referenced.objectId = opclassoid;
-            referenced.objectSubId = 0;
-            recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
-        }
-        else
-        {
-            /* if "loose" in the opfamily, use a AUTO dep on procedure */
-            recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
+        referenced.classId = proc->ref_is_family ? OperatorFamilyRelationId :
+            OperatorClassRelationId;
+        referenced.objectId = proc->refobjid;
+        referenced.objectSubId = 0;
+
+        recordDependencyOn(&myself, &referenced,
+                           proc->ref_is_hard ? DEPENDENCY_INTERNAL : DEPENDENCY_AUTO);

-            /* ... and an AUTO dep on the opfamily */
-            referenced.classId = OperatorFamilyRelationId;
-            referenced.objectId = opfamilyoid;
-            referenced.objectSubId = 0;
-            recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
-        }
         /* Post create hook of access method procedure */
         InvokeObjectPostCreateHook(AccessMethodProcedureRelationId,
                                    entryoid, 0);
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 6e3db06..db38ab0 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -114,6 +114,12 @@ typedef char *(*ambuildphasename_function) (int64 phasenum);
 /* validate definition of an opclass for this AM */
 typedef bool (*amvalidate_function) (Oid opclassoid);

+/* validate operators and support functions to be added to an opclass/family */
+typedef void (*amcheckmembers_function) (Oid opfamilyoid,
+                                         Oid opclassoid,
+                                         List *operators,
+                                         List *functions);
+
 /* prepare for index scan */
 typedef IndexScanDesc (*ambeginscan_function) (Relation indexRelation,
                                                int nkeys,
@@ -218,6 +224,7 @@ typedef struct IndexAmRoutine
     amproperty_function amproperty; /* can be NULL */
     ambuildphasename_function ambuildphasename; /* can be NULL */
     amvalidate_function amvalidate;
+    amcheckmembers_function amcheckmembers; /* can be NULL */
     ambeginscan_function ambeginscan;
     amrescan_function amrescan;
     amgettuple_function amgettuple; /* can be NULL */
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 78fcd82..4ab44d6 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -388,6 +388,10 @@ extern ItemPointer ginVacuumItemPointers(GinVacuumState *gvs,

 /* ginvalidate.c */
 extern bool ginvalidate(Oid opclassoid);
+extern void gincheckmembers(Oid opfamilyoid,
+                            Oid opclassoid,
+                            List *operators,
+                            List *functions);

 /* ginbulk.c */
 typedef struct GinEntryAccumulator
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index fc1a311..11784c2 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -451,6 +451,10 @@ extern bool gistcanreturn(Relation index, int attno);

 /* gistvalidate.c */
 extern bool gistvalidate(Oid opclassoid);
+extern void gistcheckmembers(Oid opfamilyoid,
+                             Oid opclassoid,
+                             List *operators,
+                             List *functions);

 /* gistutil.c */

diff --git a/src/include/access/spgist.h b/src/include/access/spgist.h
index d787ab2..2d1ae05 100644
--- a/src/include/access/spgist.h
+++ b/src/include/access/spgist.h
@@ -223,5 +223,9 @@ extern IndexBulkDeleteResult *spgvacuumcleanup(IndexVacuumInfo *info,

 /* spgvalidate.c */
 extern bool spgvalidate(Oid opclassoid);
+extern void spgcheckmembers(Oid opfamilyoid,
+                            Oid opclassoid,
+                            List *operators,
+                            List *functions);

 #endif                            /* SPGIST_H */
diff --git a/src/include/catalog/opfam_internal.h b/src/include/catalog/opfam_internal.h
index 9f17544..36d3feb 100644
--- a/src/include/catalog/opfam_internal.h
+++ b/src/include/catalog/opfam_internal.h
@@ -14,15 +14,36 @@

 /*
  * We use lists of this struct type to keep track of both operators and
- * procedures while building or adding to an opfamily.
+ * support functions while building or adding to an opclass or opfamily.
+ *
+ * The "ref" fields define how the pg_amop or pg_amproc entry should depend
+ * on the associated objects (that is, which dependency type to use, and
+ * which opclass or opfamily it should depend on).
+ *
+ * If ref_is_hard is true, the entry will have a NORMAL dependency on the
+ * operator or support func, and an INTERNAL dependency on the opclass or
+ * opfamily.  This forces the opclass or opfamily to be dropped if the
+ * operator or support func is dropped, and requires the CASCADE option
+ * to do so.  Nor will ALTER OPERATOR FAMILY DROP be allowed.  This is
+ * the right behavior for objects that are essential to an opclass.
+ *
+ * If ref_is_hard is false, the entry will have an AUTO dependency on the
+ * operator or support func, and also an AUTO dependency on the opclass or
+ * 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.
  */
 typedef struct
 {
-    Oid            object;            /* operator or support proc's OID */
-    int            number;            /* strategy or support proc number */
+    bool        is_func;        /* is this an operator, or support func? */
+    Oid            object;            /* operator or support func's OID */
+    int            number;            /* strategy or support func number */
     Oid            lefttype;        /* lefttype */
     Oid            righttype;        /* righttype */
     Oid            sortfamily;        /* ordering operator's sort opfamily, or 0 */
+    bool        ref_is_hard;    /* hard or soft dependency? */
+    bool        ref_is_family;    /* is dependency on opclass or opfamily? */
+    Oid            refobjid;        /* OID of opclass or opfamily */
 } OpFamilyMember;

 #endif                            /* OPFAM_INTERNAL_H */