Thread: Rethinking opclass member checks and dependency strength
Over in [1] we realized that it would be a good idea to remove the <@
operator from contrib/intarray's GiST opclasses. Unfortunately, doing
that isn't a simple matter of generating an extension update script
containing ALTER OPERATOR FAMILY DROP OPERATOR, because that operator
is marked as internally dependent on its opclass which means that
dependency.c will object. We could do some direct hacking on
pg_depend to let the DROP be allowed, but ugh.
I started to wonder why GiST opclass operators are ever considered
as required members of their opclass. The existing rule (cf.
opclasscmds.c) is that everything mentioned in CREATE OPERATOR CLASS
will have an internal dependency on the opclass, but if you add
operators or functions with ALTER OPERATOR FAMILY ADD, those just
have AUTO dependencies on their operator family. So the assumption
is that opclass creators will only put the bare minimum of required
stuff into CREATE OPERATOR CLASS and then add optional stuff with
ALTER ... ADD. But none of our contrib modules do it like that,
and I'd lay long odds against any third party code doing it either.
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.
If we went along this line, then a dump/restore or pg_upgrade
would be enough to change an opclass's dependencies to the new
style, which would get us to a place where intarray's problem
could be fixed with ALTER OPERATOR FAMILY DROP OPERATOR and
nothing else. Such an upgrade script wouldn't work in older
releases, but I think we don't generally care about that.
Thoughts?
regards, tom lane
[1] https://www.postgresql.org/message-id/458.1565114141@sss.pgh.pa.us
On Wed, Aug 7, 2019 at 7:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Over in [1] we realized that it would be a good idea to remove the <@ > operator from contrib/intarray's GiST opclasses. Unfortunately, doing > that isn't a simple matter of generating an extension update script > containing ALTER OPERATOR FAMILY DROP OPERATOR, because that operator > is marked as internally dependent on its opclass which means that > dependency.c will object. We could do some direct hacking on > pg_depend to let the DROP be allowed, but ugh. > > I started to wonder why GiST opclass operators are ever considered > as required members of their opclass. The existing rule (cf. > opclasscmds.c) is that everything mentioned in CREATE OPERATOR CLASS > will have an internal dependency on the opclass, but if you add > operators or functions with ALTER OPERATOR FAMILY ADD, those just > have AUTO dependencies on their operator family. So the assumption > is that opclass creators will only put the bare minimum of required > stuff into CREATE OPERATOR CLASS and then add optional stuff with > ALTER ... ADD. But none of our contrib modules do it like that, > and I'd lay long odds against any third party code doing it either. That's really odd. I don't think any extension SQL scripts does really care about difference between operators defined in CREATE OPERATOR CLASS and operators defined in ALTER OPERATOR FAMILY ADD. But if they would care, then all GiST, GIN, SP-GiST and BRIN opclasses would define all their operators using ALTER OPERATOR FAMILY ADD. > 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. > If we went along this line, then a dump/restore or pg_upgrade > would be enough to change an opclass's dependencies to the new > style, which would get us to a place where intarray's problem > could be fixed with ALTER OPERATOR FAMILY DROP OPERATOR and > nothing else. Such an upgrade script wouldn't work in older > releases, but I think we don't generally care about that. +1 ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Rethinking opclass member checks and dependency strength
From
Darafei "Komяpa" Praliaskouski
Date:
But none of our contrib modules do it like that, and I'd lay long odds against any third party code doing it either.
Thoughts?
PostGIS has some rarely used box operations as part of GiST opclass, like "overabove".
These are source of misunderstanding, as it hinges on the fact that non-square geometry will be coerced into a box on a call, which is not obvious when you call it on something like diagonal linestrings.
It may happen that we will decide to remove them. On such circumstances, I expect that ALTER OPERATOR CLASS DROP OPERATOR will work.
Other thing that I would expect is that DROP FUNCTION ... CASCADE will remove the operator and unregister the operator from operator class without dropping operator class itself.
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 */
On Sun, Aug 18, 2019 at 10:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > 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. Looks sufficient to me. I didn't yet imagine something else useful. > * 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? Flexibility also looks sufficient to me. > * 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 think we need ability to remove GiST fetch proc. Presence of this procedure is used to determine whether GiST index supports index only scan (IOS). We need to be able to remove this proc to drop IOS support. > * 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 also not sure about this. Two lists look OK to me. > * 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. +1 ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> On Sun, Aug 18, 2019 at 10:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * 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 think we need ability to remove GiST fetch proc. Presence of this
> procedure is used to determine whether GiST index supports index only
> scan (IOS). We need to be able to remove this proc to drop IOS
> support.
OK ... so thinking in more general terms, you're arguing that any optional
support function should have a soft not hard dependency. The attached v2
patch implements that rule, and also changes btree and hash to use
the cross-type-vs-not-cross-type behavior I proposed earlier.
This change results in a possibly surprising change in the expected output
for the 002_pg_dump.pl test: an optional support function that had been
created as part of CREATE OPERATOR CLASS will now be dumped as part of
ALTER OPERATOR FAMILY. Maybe that's too surprising? Another approach
that we could use is to give up the premise that soft dependencies are
always on the opfamily. If we kept the dependencies pointing to the
same objects as before (opclass or opfamily) and only twiddled the
dependency strength, then pg_dump's output would not change. Now,
this would possibly result in dropping a still-useful family member
if it were incorrectly tied to an opclass that's dropped --- but that
would have happened before, too. I'm not quite sure if we really want
to editorialize on the user's decisions about which grouping to tie
family members to.
>> * 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.
> +1
Did that in this revision, too.
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..2d16c6f 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,48 @@ 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>amapi.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. Optional support functions are typically
+ also given soft dependencies, so that they can be removed if necessary.
</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..baf07a9 100644
--- a/src/backend/access/gin/ginvalidate.c
+++ b/src/backend/access/gin/ginvalidate.c
@@ -268,3 +268,64 @@ 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;
+ }
+
+ /*
+ * Required support functions should have hard dependencies. Preferably
+ * those are just dependencies on the opclass, but if we're in ALTER
+ * OPERATOR FAMILY, we leave the dependency pointing at the whole
+ * opfamily. (Given that GIN opclasses generally don't share opfamilies,
+ * it seems unlikely to be worth working harder.)
+ */
+ foreach(lc, functions)
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ switch (op->number)
+ {
+ case GIN_EXTRACTVALUE_PROC:
+ case GIN_EXTRACTQUERY_PROC:
+ /* Required support function */
+ op->ref_is_hard = true;
+ break;
+ case GIN_COMPARE_PROC:
+ case GIN_CONSISTENT_PROC:
+ case GIN_COMPARE_PARTIAL_PROC:
+ case GIN_TRICONSISTENT_PROC:
+ /* Optional, so force it to be a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ break;
+ default:
+ /* Probably we should throw error here */
+ break;
+ }
+ }
+}
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..8e2503f 100644
--- a/src/backend/access/gist/gistvalidate.c
+++ b/src/backend/access/gist/gistvalidate.c
@@ -275,3 +275,68 @@ 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;
+ }
+
+ /*
+ * Required support functions should have hard dependencies. Preferably
+ * those are just dependencies on the opclass, but if we're in ALTER
+ * OPERATOR FAMILY, we leave the dependency pointing at the whole
+ * opfamily. (Given that GiST opclasses generally don't share opfamilies,
+ * it seems unlikely to be worth working harder.)
+ */
+ foreach(lc, functions)
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ switch (op->number)
+ {
+ case GIST_CONSISTENT_PROC:
+ case GIST_UNION_PROC:
+ case GIST_PENALTY_PROC:
+ case GIST_PICKSPLIT_PROC:
+ case GIST_EQUAL_PROC:
+ /* Required support function */
+ op->ref_is_hard = true;
+ break;
+ case GIST_COMPRESS_PROC:
+ case GIST_DECOMPRESS_PROC:
+ case GIST_DISTANCE_PROC:
+ case GIST_FETCH_PROC:
+ /* Optional, so force it to be a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ break;
+ default:
+ /* Probably we should throw error here */
+ break;
+ }
+ }
+}
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 5cc30da..3515a7e 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 = hashcheckmembers;
amroutine->ambeginscan = hashbeginscan;
amroutine->amrescan = hashrescan;
amroutine->amgettuple = hashgettuple;
diff --git a/src/backend/access/hash/hashvalidate.c b/src/backend/access/hash/hashvalidate.c
index 9315872..cbab4ec 100644
--- a/src/backend/access/hash/hashvalidate.c
+++ b/src/backend/access/hash/hashvalidate.c
@@ -16,6 +16,8 @@
#include "access/amvalidate.h"
#include "access/hash.h"
#include "access/htup_details.h"
+#include "access/xact.h"
+#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
#include "catalog/pg_amproc.h"
#include "catalog/pg_opclass.h"
@@ -25,6 +27,7 @@
#include "parser/parse_coerce.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
+#include "utils/lsyscache.h"
#include "utils/regproc.h"
#include "utils/syscache.h"
@@ -334,3 +337,96 @@ check_hash_func_signature(Oid funcid, int16 amprocnum, Oid argtype)
ReleaseSysCache(tp);
return result;
}
+
+/*
+ * Prechecking function for adding operators/functions to a hash opfamily.
+ */
+void
+hashcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions)
+{
+ Oid opcintype;
+ ListCell *lc;
+
+ /*
+ * Hash operators and required support functions are always "loose"
+ * members of the opfamily if they are cross-type. If they are not
+ * cross-type, we prefer to tie them to the appropriate opclass ... but if
+ * the user hasn't created one, we can't do that, and must fall back to
+ * using the opfamily dependency. (We mustn't force creation of an
+ * opclass in such a case, as leaving an incomplete opclass laying about
+ * would be bad. Throwing an error is another undesirable alternative.)
+ *
+ * This behavior results in a bit of a dump/reload hazard, in that the
+ * order of restoring objects could affect what dependencies we end up
+ * with. pg_dump's existing behavior will preserve the dependency choices
+ * in most cases, but not if a cross-type operator has been bound tightly
+ * into an opclass. That's a mistake anyway, so silently "fixing" it
+ * isn't awful.
+ *
+ * Optional support functions are always "loose" family members.
+ *
+ * To avoid repeated lookups, we remember the most recently used opclass's
+ * input type.
+ */
+ if (OidIsValid(opclassoid))
+ {
+ /* During CREATE OPERATOR CLASS, need CCI to see the pg_opclass row */
+ CommandCounterIncrement();
+ opcintype = get_opclass_input_type(opclassoid);
+ }
+ else
+ opcintype = InvalidOid;
+
+ /*
+ * We handle operators and support functions almost identically, so rather
+ * than duplicate this code block, just join the lists.
+ */
+ foreach(lc, list_concat_copy(operators, functions))
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ if (op->is_func && op->number != HASHSTANDARD_PROC)
+ {
+ /* Optional support proc, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else if (op->lefttype != op->righttype)
+ {
+ /* Cross-type, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else
+ {
+ /* Not cross-type; is there a suitable opclass? */
+ if (op->lefttype != opcintype)
+ {
+ /* Avoid repeating this expensive lookup, even if it fails */
+ opcintype = op->lefttype;
+ opclassoid = opclass_for_family_datatype(HASH_AM_OID,
+ opfamilyoid,
+ opcintype);
+ }
+ if (OidIsValid(opclassoid))
+ {
+ /* Hard dependency on opclass */
+ op->ref_is_hard = true;
+ op->ref_is_family = false;
+ op->refobjid = opclassoid;
+ }
+ else
+ {
+ /* We're stuck, so make a soft dependency on the opfamily */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ }
+ }
+}
diff --git a/src/backend/access/index/amvalidate.c b/src/backend/access/index/amvalidate.c
index 5a27285..2045b72 100644
--- a/src/backend/access/index/amvalidate.c
+++ b/src/backend/access/index/amvalidate.c
@@ -1,7 +1,8 @@
/*-------------------------------------------------------------------------
*
* amvalidate.c
- * Support routines for index access methods' amvalidate functions.
+ * Support routines for index access methods' amvalidate and
+ * amcheckmembers functions.
*
* Copyright (c) 2016-2019, PostgreSQL Global Development Group
*
@@ -211,21 +212,28 @@ check_amop_signature(Oid opno, Oid restype, Oid lefttype, Oid righttype)
}
/*
- * Is the datatype a legitimate input type for the btree opfamily?
+ * Get the OID of the opclass belonging to an opfamily and accepting
+ * the specified type as input type. Returns InvalidOid if no such opclass.
+ *
+ * If there is more than one such opclass, you get a random one of them.
+ * Since that shouldn't happen, we don't waste cycles checking.
+ *
+ * We could look up the AM's OID from the opfamily, but all existing callers
+ * know that or can get it without an extra lookup, so we make them pass it.
*/
-bool
-opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
+Oid
+opclass_for_family_datatype(Oid amoid, Oid opfamilyoid, Oid datatypeoid)
{
- bool result = false;
+ Oid result = InvalidOid;
CatCList *opclist;
int i;
/*
- * We search through all btree opclasses to see if one matches. This is a
- * bit inefficient but there is no better index available. It also saves
- * making an explicit check that the opfamily belongs to btree.
+ * We search through all the AM's opclasses to see if one matches. This
+ * is a bit inefficient but there is no better index available. It also
+ * saves making an explicit check that the opfamily belongs to the AM.
*/
- opclist = SearchSysCacheList1(CLAAMNAMENSP, ObjectIdGetDatum(BTREE_AM_OID));
+ opclist = SearchSysCacheList1(CLAAMNAMENSP, ObjectIdGetDatum(amoid));
for (i = 0; i < opclist->n_members; i++)
{
@@ -235,7 +243,7 @@ opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
if (classform->opcfamily == opfamilyoid &&
classform->opcintype == datatypeoid)
{
- result = true;
+ result = classform->oid;
break;
}
}
@@ -244,3 +252,14 @@ opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
return result;
}
+
+/*
+ * Is the datatype a legitimate input type for the btree opfamily?
+ */
+bool
+opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
+{
+ return OidIsValid(opclass_for_family_datatype(BTREE_AM_OID,
+ opfamilyoid,
+ datatypeoid));
+}
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4cfd528..14f2505 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 = btcheckmembers;
amroutine->ambeginscan = btbeginscan;
amroutine->amrescan = btrescan;
amroutine->amgettuple = btgettuple;
diff --git a/src/backend/access/nbtree/nbtvalidate.c b/src/backend/access/nbtree/nbtvalidate.c
index 0148ea7..468cd95 100644
--- a/src/backend/access/nbtree/nbtvalidate.c
+++ b/src/backend/access/nbtree/nbtvalidate.c
@@ -16,12 +16,15 @@
#include "access/amvalidate.h"
#include "access/htup_details.h"
#include "access/nbtree.h"
+#include "access/xact.h"
+#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
#include "catalog/pg_amproc.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_type.h"
#include "utils/builtins.h"
+#include "utils/lsyscache.h"
#include "utils/regproc.h"
#include "utils/syscache.h"
@@ -275,3 +278,96 @@ btvalidate(Oid opclassoid)
return result;
}
+
+/*
+ * Prechecking function for adding operators/functions to a btree opfamily.
+ */
+void
+btcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions)
+{
+ Oid opcintype;
+ ListCell *lc;
+
+ /*
+ * Btree operators and comparison support functions are always "loose"
+ * members of the opfamily if they are cross-type. If they are not
+ * cross-type, we prefer to tie them to the appropriate opclass ... but if
+ * the user hasn't created one, we can't do that, and must fall back to
+ * using the opfamily dependency. (We mustn't force creation of an
+ * opclass in such a case, as leaving an incomplete opclass laying about
+ * would be bad. Throwing an error is another undesirable alternative.)
+ *
+ * This behavior results in a bit of a dump/reload hazard, in that the
+ * order of restoring objects could affect what dependencies we end up
+ * with. pg_dump's existing behavior will preserve the dependency choices
+ * in most cases, but not if a cross-type operator has been bound tightly
+ * into an opclass. That's a mistake anyway, so silently "fixing" it
+ * isn't awful.
+ *
+ * Optional support functions are always "loose" family members.
+ *
+ * To avoid repeated lookups, we remember the most recently used opclass's
+ * input type.
+ */
+ if (OidIsValid(opclassoid))
+ {
+ /* During CREATE OPERATOR CLASS, need CCI to see the pg_opclass row */
+ CommandCounterIncrement();
+ opcintype = get_opclass_input_type(opclassoid);
+ }
+ else
+ opcintype = InvalidOid;
+
+ /*
+ * We handle operators and support functions almost identically, so rather
+ * than duplicate this code block, just join the lists.
+ */
+ foreach(lc, list_concat_copy(operators, functions))
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ if (op->is_func && op->number != BTORDER_PROC)
+ {
+ /* Optional support proc, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else if (op->lefttype != op->righttype)
+ {
+ /* Cross-type, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else
+ {
+ /* Not cross-type; is there a suitable opclass? */
+ if (op->lefttype != opcintype)
+ {
+ /* Avoid repeating this expensive lookup, even if it fails */
+ opcintype = op->lefttype;
+ opclassoid = opclass_for_family_datatype(BTREE_AM_OID,
+ opfamilyoid,
+ opcintype);
+ }
+ if (OidIsValid(opclassoid))
+ {
+ /* Hard dependency on opclass */
+ op->ref_is_hard = true;
+ op->ref_is_family = false;
+ op->refobjid = opclassoid;
+ }
+ else
+ {
+ /* We're stuck, so make a soft dependency on the opfamily */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ }
+ }
+}
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..1bdd352 100644
--- a/src/backend/access/spgist/spgvalidate.c
+++ b/src/backend/access/spgist/spgvalidate.c
@@ -298,3 +298,65 @@ 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;
+ }
+
+ /*
+ * Required support functions should have hard dependencies. Preferably
+ * those are just dependencies on the opclass, but if we're in ALTER
+ * OPERATOR FAMILY, we leave the dependency pointing at the whole
+ * opfamily. (Given that SP-GiST opclasses generally don't share
+ * opfamilies, it seems unlikely to be worth working harder.)
+ */
+ foreach(lc, functions)
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ switch (op->number)
+ {
+ case SPGIST_CONFIG_PROC:
+ case SPGIST_CHOOSE_PROC:
+ case SPGIST_PICKSPLIT_PROC:
+ case SPGIST_INNER_CONSISTENT_PROC:
+ case SPGIST_LEAF_CONSISTENT_PROC:
+ /* Required support function */
+ op->ref_is_hard = true;
+ break;
+ case SPGIST_COMPRESS_PROC:
+ /* Optional, so force it to be a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ break;
+ default:
+ /* Probably we should throw error here */
+ break;
+ }
+ }
+}
diff --git a/src/backend/commands/opclasscmds.c b/src/backend/commands/opclasscmds.c
index 6a1ccde..93a0ac2 100644
--- a/src/backend/commands/opclasscmds.c
+++ b/src/backend/commands/opclasscmds.c
@@ -27,7 +27,6 @@
#include "catalog/dependency.h"
#include "catalog/indexing.h"
#include "catalog/objectaccess.h"
-#include "catalog/opfam_internal.h"
#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
#include "catalog/pg_amproc.h"
@@ -62,12 +61,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 +513,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 +538,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 +548,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 +661,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 +868,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 +927,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 +957,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 +972,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 +986,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 +1045,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 +1061,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 +1320,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 +1332,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 +1354,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 +1427,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 amapi.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 +1445,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 +1461,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 +1528,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 amapi.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/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 4712e3a..1fc3fbf 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -523,6 +523,7 @@ my %tests = (
OPERATOR 5 >(bigint,int4),
FUNCTION 1 (int4, int4) btint4cmp(int4,int4),
FUNCTION 2 (int4, int4) btint4sortsupport(internal);',
+ # note: it's correct that btint8sortsupport is included here:
regexp => qr/^
\QALTER OPERATOR FAMILY dump_test.op_family USING btree ADD\E\n\s+
\QOPERATOR 1 <(bigint,integer) ,\E\n\s+
@@ -531,6 +532,7 @@ my %tests = (
\QOPERATOR 4 >=(bigint,integer) ,\E\n\s+
\QOPERATOR 5 >(bigint,integer) ,\E\n\s+
\QFUNCTION 1 (integer, integer) btint4cmp(integer,integer) ,\E\n\s+
+ \QFUNCTION 2 (bigint, bigint) btint8sortsupport(internal) ,\E\n\s+
\QFUNCTION 2 (integer, integer) btint4sortsupport(internal);\E
/xm,
like =>
@@ -1555,6 +1557,7 @@ my %tests = (
OPERATOR 5 >(bigint,bigint),
FUNCTION 1 btint8cmp(bigint,bigint),
FUNCTION 2 btint8sortsupport(internal);',
+ # note: it's correct that btint8sortsupport isn't included here:
regexp => qr/^
\QCREATE OPERATOR CLASS dump_test.op_class\E\n\s+
\QFOR TYPE bigint USING btree FAMILY dump_test.op_family AS\E\n\s+
@@ -1563,8 +1566,7 @@ my %tests = (
\QOPERATOR 3 =(bigint,bigint) ,\E\n\s+
\QOPERATOR 4 >=(bigint,bigint) ,\E\n\s+
\QOPERATOR 5 >(bigint,bigint) ,\E\n\s+
- \QFUNCTION 1 (bigint, bigint) btint8cmp(bigint,bigint) ,\E\n\s+
- \QFUNCTION 2 (bigint, bigint) btint8sortsupport(internal);\E
+ \QFUNCTION 1 (bigint, bigint) btint8cmp(bigint,bigint);\E
/xm,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 6e3db06..2ee34d7 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -54,6 +54,42 @@ typedef enum IndexAMProperty
AMPROP_CAN_INCLUDE
} IndexAMProperty;
+/*
+ * We use lists of this struct type to keep track of both operators and
+ * support functions while building or adding to an opclass or opfamily.
+ * amcheckmembers functions receive lists of these structs, and are allowed
+ * to alter their "ref" fields.
+ *
+ * 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 OpFamilyMember
+{
+ 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;
+
/*
* Callback function signatures --- see indexam.sgml for more info.
@@ -114,6 +150,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 +260,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/amvalidate.h b/src/include/access/amvalidate.h
index 317e1e6..41e62e0 100644
--- a/src/include/access/amvalidate.h
+++ b/src/include/access/amvalidate.h
@@ -1,7 +1,8 @@
/*-------------------------------------------------------------------------
*
* amvalidate.h
- * Support routines for index access methods' amvalidate functions.
+ * Support routines for index access methods' amvalidate and
+ * amcheckmembers functions.
*
* Copyright (c) 2016-2019, PostgreSQL Global Development Group
*
@@ -31,6 +32,8 @@ extern bool check_amproc_signature(Oid funcid, Oid restype, bool exact,
int minargs, int maxargs,...);
extern bool check_amop_signature(Oid opno, Oid restype,
Oid lefttype, Oid righttype);
+extern Oid opclass_for_family_datatype(Oid amoid, Oid opfamilyoid,
+ Oid datatypeoid);
extern bool opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid);
#endif /* AMVALIDATE_H */
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 ed5b643..58c909e 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -469,6 +469,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/hash.h b/src/include/access/hash.h
index 24af778..8287877 100644
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -362,6 +362,10 @@ extern IndexBulkDeleteResult *hashvacuumcleanup(IndexVacuumInfo *info,
IndexBulkDeleteResult *stats);
extern bytea *hashoptions(Datum reloptions, bool validate);
extern bool hashvalidate(Oid opclassoid);
+extern void hashcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions);
/* private routines */
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index 4a80e84..3e5fe97 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -817,6 +817,10 @@ extern void _bt_check_third_page(Relation rel, Relation heap,
* prototypes for functions in nbtvalidate.c
*/
extern bool btvalidate(Oid opclassoid);
+extern void btcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions);
/*
* prototypes for functions in nbtsort.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
deleted file mode 100644
index 9f17544..0000000
--- a/src/include/catalog/opfam_internal.h
+++ /dev/null
@@ -1,28 +0,0 @@
-/*-------------------------------------------------------------------------
- *
- * opfam_internal.h
- *
- * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
- *
- * src/include/catalog/opfam_internal.h
- *
- *-------------------------------------------------------------------------
- */
-#ifndef OPFAM_INTERNAL_H
-#define OPFAM_INTERNAL_H
-
-/*
- * We use lists of this struct type to keep track of both operators and
- * procedures while building or adding to an opfamily.
- */
-typedef struct
-{
- Oid object; /* operator or support proc's OID */
- int number; /* strategy or support proc number */
- Oid lefttype; /* lefttype */
- Oid righttype; /* righttype */
- Oid sortfamily; /* ordering operator's sort opfamily, or 0 */
-} OpFamilyMember;
-
-#endif /* OPFAM_INTERNAL_H */
Hi, The latest version of this patch (from 2019/09/14) no longer applies, although maybe it's some issue with patch format (applying it using patch works fine, git am fails with "Patch format detection failed."). In any case, this means cputube can't apply/test it. On Sat, Sep 14, 2019 at 07:01:33PM -0400, Tom Lane wrote: >Alexander Korotkov <a.korotkov@postgrespro.ru> writes: >> On Sun, Aug 18, 2019 at 10:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> * 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 think we need ability to remove GiST fetch proc. Presence of this >> procedure is used to determine whether GiST index supports index only >> scan (IOS). We need to be able to remove this proc to drop IOS >> support. > >OK ... so thinking in more general terms, you're arguing that any optional >support function should have a soft not hard dependency. The attached v2 >patch implements that rule, and also changes btree and hash to use >the cross-type-vs-not-cross-type behavior I proposed earlier. > >This change results in a possibly surprising change in the expected output >for the 002_pg_dump.pl test: an optional support function that had been >created as part of CREATE OPERATOR CLASS will now be dumped as part of >ALTER OPERATOR FAMILY. Maybe that's too surprising? Another approach >that we could use is to give up the premise that soft dependencies are >always on the opfamily. If we kept the dependencies pointing to the >same objects as before (opclass or opfamily) and only twiddled the >dependency strength, then pg_dump's output would not change. Now, >this would possibly result in dropping a still-useful family member >if it were incorrectly tied to an opclass that's dropped --- but that >would have happened before, too. I'm not quite sure if we really want >to editorialize on the user's decisions about which grouping to tie >family members to. > I agree it's a bit weird to add a dependency on an opfamily and not the opclass. Not just because of the pg_dump weirdness, but doesn't it mean that after a DROP OPERATOR CLASS we might still reject a DROP OPERATOR because of the remaining opfamily dependency? (Haven't tried, so maybe that works fine). >>> * 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. > >> +1 > >Did that in this revision, too. > One minor comment from me is that maybe "amcheckmembers" is a bit misleading. In my mind "check" implies a plain passive check, not something that may actually tweak the dependency type. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> The latest version of this patch (from 2019/09/14) no longer applies,
> although maybe it's some issue with patch format (applying it using
> patch works fine, git am fails with "Patch format detection failed.").
Hm, seems to be just a trivial conflict against the copyright-date-update
patch. Updated version attached.
> On Sat, Sep 14, 2019 at 07:01:33PM -0400, Tom Lane wrote:
>> This change results in a possibly surprising change in the expected output
>> for the 002_pg_dump.pl test: an optional support function that had been
>> created as part of CREATE OPERATOR CLASS will now be dumped as part of
>> ALTER OPERATOR FAMILY. Maybe that's too surprising? Another approach
>> that we could use is to give up the premise that soft dependencies are
>> always on the opfamily. If we kept the dependencies pointing to the
>> same objects as before (opclass or opfamily) and only twiddled the
>> dependency strength, then pg_dump's output would not change. Now,
>> this would possibly result in dropping a still-useful family member
>> if it were incorrectly tied to an opclass that's dropped --- but that
>> would have happened before, too. I'm not quite sure if we really want
>> to editorialize on the user's decisions about which grouping to tie
>> family members to.
> I agree it's a bit weird to add a dependency on an opfamily and not the
> opclass. Not just because of the pg_dump weirdness, but doesn't it mean
> that after a DROP OPERATOR CLASS we might still reject a DROP OPERATOR
> because of the remaining opfamily dependency? (Haven't tried, so maybe
> that works fine).
I poked at the idea of retaining the user's decisions as to whether
a member object is a member of an individual opclass or an opfamily,
but soon realized that there's a big problem with that: we don't have
any ALTER OPERATOR CLASS ADD/DROP syntax, only ALTER OPERATOR FAMILY.
So there's no way to express the concept of "add this at the opclass
level", if you forgot to add it during initial opclass creation.
I suppose that some case could be made for adding such syntax, but
it seems like a significant amount of work, and in the end it seems
like it's better to trust the system to get these assignments right.
Letting the user do it doesn't add much except the opportunity
to shoot oneself in the foot.
> One minor comment from me is that maybe "amcheckmembers" is a bit
> misleading. In my mind "check" implies a plain passive check, not
> something that may actually tweak the dependency type.
Hmm. I'm not wedded to that name, but do you have a better proposal?
The end goal (not realized in this patch, of course) is that these
callbacks would perform fairly thorough checking of opclass members,
missing only the ability to check that all required members are present.
So I don't want to name them something like "amfixdependencies", even
if that's all they're doing right now.
I see your point that "check" suggests a read-only operation, but
I'm not sure about a better verb. I thought of "amvalidatemembers",
but that's not really much better than "check" is it?
regards, tom lane
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index 23d959b..b016be1 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..2d16c6f 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,48 @@ 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>amapi.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. Optional support functions are typically
+ also given soft dependencies, so that they can be removed if necessary.
</para>
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index d89af78..ceea92f 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 910f0bc..b06772d 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 0b62e0a..f44aa4d 100644
--- a/src/backend/access/gin/ginvalidate.c
+++ b/src/backend/access/gin/ginvalidate.c
@@ -267,3 +267,64 @@ 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;
+ }
+
+ /*
+ * Required support functions should have hard dependencies. Preferably
+ * those are just dependencies on the opclass, but if we're in ALTER
+ * OPERATOR FAMILY, we leave the dependency pointing at the whole
+ * opfamily. (Given that GIN opclasses generally don't share opfamilies,
+ * it seems unlikely to be worth working harder.)
+ */
+ foreach(lc, functions)
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ switch (op->number)
+ {
+ case GIN_EXTRACTVALUE_PROC:
+ case GIN_EXTRACTQUERY_PROC:
+ /* Required support function */
+ op->ref_is_hard = true;
+ break;
+ case GIN_COMPARE_PROC:
+ case GIN_CONSISTENT_PROC:
+ case GIN_COMPARE_PARTIAL_PROC:
+ case GIN_TRICONSISTENT_PROC:
+ /* Optional, so force it to be a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ break;
+ default:
+ /* Probably we should throw error here */
+ break;
+ }
+ }
+}
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 5c9ad34..23721c3 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -87,6 +87,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 0c4fb8c..46c31c4 100644
--- a/src/backend/access/gist/gistvalidate.c
+++ b/src/backend/access/gist/gistvalidate.c
@@ -275,3 +275,68 @@ 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;
+ }
+
+ /*
+ * Required support functions should have hard dependencies. Preferably
+ * those are just dependencies on the opclass, but if we're in ALTER
+ * OPERATOR FAMILY, we leave the dependency pointing at the whole
+ * opfamily. (Given that GiST opclasses generally don't share opfamilies,
+ * it seems unlikely to be worth working harder.)
+ */
+ foreach(lc, functions)
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ switch (op->number)
+ {
+ case GIST_CONSISTENT_PROC:
+ case GIST_UNION_PROC:
+ case GIST_PENALTY_PROC:
+ case GIST_PICKSPLIT_PROC:
+ case GIST_EQUAL_PROC:
+ /* Required support function */
+ op->ref_is_hard = true;
+ break;
+ case GIST_COMPRESS_PROC:
+ case GIST_DECOMPRESS_PROC:
+ case GIST_DISTANCE_PROC:
+ case GIST_FETCH_PROC:
+ /* Optional, so force it to be a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ break;
+ default:
+ /* Probably we should throw error here */
+ break;
+ }
+ }
+}
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 4bb6efc..82d8336 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -85,6 +85,7 @@ hashhandler(PG_FUNCTION_ARGS)
amroutine->amproperty = NULL;
amroutine->ambuildphasename = NULL;
amroutine->amvalidate = hashvalidate;
+ amroutine->amcheckmembers = hashcheckmembers;
amroutine->ambeginscan = hashbeginscan;
amroutine->amrescan = hashrescan;
amroutine->amgettuple = hashgettuple;
diff --git a/src/backend/access/hash/hashvalidate.c b/src/backend/access/hash/hashvalidate.c
index 6346e65..5c8626a 100644
--- a/src/backend/access/hash/hashvalidate.c
+++ b/src/backend/access/hash/hashvalidate.c
@@ -16,6 +16,8 @@
#include "access/amvalidate.h"
#include "access/hash.h"
#include "access/htup_details.h"
+#include "access/xact.h"
+#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
#include "catalog/pg_amproc.h"
#include "catalog/pg_opclass.h"
@@ -25,6 +27,7 @@
#include "parser/parse_coerce.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
+#include "utils/lsyscache.h"
#include "utils/regproc.h"
#include "utils/syscache.h"
@@ -334,3 +337,96 @@ check_hash_func_signature(Oid funcid, int16 amprocnum, Oid argtype)
ReleaseSysCache(tp);
return result;
}
+
+/*
+ * Prechecking function for adding operators/functions to a hash opfamily.
+ */
+void
+hashcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions)
+{
+ Oid opcintype;
+ ListCell *lc;
+
+ /*
+ * Hash operators and required support functions are always "loose"
+ * members of the opfamily if they are cross-type. If they are not
+ * cross-type, we prefer to tie them to the appropriate opclass ... but if
+ * the user hasn't created one, we can't do that, and must fall back to
+ * using the opfamily dependency. (We mustn't force creation of an
+ * opclass in such a case, as leaving an incomplete opclass laying about
+ * would be bad. Throwing an error is another undesirable alternative.)
+ *
+ * This behavior results in a bit of a dump/reload hazard, in that the
+ * order of restoring objects could affect what dependencies we end up
+ * with. pg_dump's existing behavior will preserve the dependency choices
+ * in most cases, but not if a cross-type operator has been bound tightly
+ * into an opclass. That's a mistake anyway, so silently "fixing" it
+ * isn't awful.
+ *
+ * Optional support functions are always "loose" family members.
+ *
+ * To avoid repeated lookups, we remember the most recently used opclass's
+ * input type.
+ */
+ if (OidIsValid(opclassoid))
+ {
+ /* During CREATE OPERATOR CLASS, need CCI to see the pg_opclass row */
+ CommandCounterIncrement();
+ opcintype = get_opclass_input_type(opclassoid);
+ }
+ else
+ opcintype = InvalidOid;
+
+ /*
+ * We handle operators and support functions almost identically, so rather
+ * than duplicate this code block, just join the lists.
+ */
+ foreach(lc, list_concat_copy(operators, functions))
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ if (op->is_func && op->number != HASHSTANDARD_PROC)
+ {
+ /* Optional support proc, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else if (op->lefttype != op->righttype)
+ {
+ /* Cross-type, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else
+ {
+ /* Not cross-type; is there a suitable opclass? */
+ if (op->lefttype != opcintype)
+ {
+ /* Avoid repeating this expensive lookup, even if it fails */
+ opcintype = op->lefttype;
+ opclassoid = opclass_for_family_datatype(HASH_AM_OID,
+ opfamilyoid,
+ opcintype);
+ }
+ if (OidIsValid(opclassoid))
+ {
+ /* Hard dependency on opclass */
+ op->ref_is_hard = true;
+ op->ref_is_family = false;
+ op->refobjid = opclassoid;
+ }
+ else
+ {
+ /* We're stuck, so make a soft dependency on the opfamily */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ }
+ }
+}
diff --git a/src/backend/access/index/amvalidate.c b/src/backend/access/index/amvalidate.c
index 3eae6aa..fa1f9d8 100644
--- a/src/backend/access/index/amvalidate.c
+++ b/src/backend/access/index/amvalidate.c
@@ -1,7 +1,8 @@
/*-------------------------------------------------------------------------
*
* amvalidate.c
- * Support routines for index access methods' amvalidate functions.
+ * Support routines for index access methods' amvalidate and
+ * amcheckmembers functions.
*
* Copyright (c) 2016-2020, PostgreSQL Global Development Group
*
@@ -211,21 +212,28 @@ check_amop_signature(Oid opno, Oid restype, Oid lefttype, Oid righttype)
}
/*
- * Is the datatype a legitimate input type for the btree opfamily?
+ * Get the OID of the opclass belonging to an opfamily and accepting
+ * the specified type as input type. Returns InvalidOid if no such opclass.
+ *
+ * If there is more than one such opclass, you get a random one of them.
+ * Since that shouldn't happen, we don't waste cycles checking.
+ *
+ * We could look up the AM's OID from the opfamily, but all existing callers
+ * know that or can get it without an extra lookup, so we make them pass it.
*/
-bool
-opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
+Oid
+opclass_for_family_datatype(Oid amoid, Oid opfamilyoid, Oid datatypeoid)
{
- bool result = false;
+ Oid result = InvalidOid;
CatCList *opclist;
int i;
/*
- * We search through all btree opclasses to see if one matches. This is a
- * bit inefficient but there is no better index available. It also saves
- * making an explicit check that the opfamily belongs to btree.
+ * We search through all the AM's opclasses to see if one matches. This
+ * is a bit inefficient but there is no better index available. It also
+ * saves making an explicit check that the opfamily belongs to the AM.
*/
- opclist = SearchSysCacheList1(CLAAMNAMENSP, ObjectIdGetDatum(BTREE_AM_OID));
+ opclist = SearchSysCacheList1(CLAAMNAMENSP, ObjectIdGetDatum(amoid));
for (i = 0; i < opclist->n_members; i++)
{
@@ -235,7 +243,7 @@ opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
if (classform->opcfamily == opfamilyoid &&
classform->opcintype == datatypeoid)
{
- result = true;
+ result = classform->oid;
break;
}
}
@@ -244,3 +252,14 @@ opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
return result;
}
+
+/*
+ * Is the datatype a legitimate input type for the btree opfamily?
+ */
+bool
+opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
+{
+ return OidIsValid(opclass_for_family_datatype(BTREE_AM_OID,
+ opfamilyoid,
+ datatypeoid));
+}
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 8376a5e..79bdbd5 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -134,6 +134,7 @@ bthandler(PG_FUNCTION_ARGS)
amroutine->amproperty = btproperty;
amroutine->ambuildphasename = btbuildphasename;
amroutine->amvalidate = btvalidate;
+ amroutine->amcheckmembers = btcheckmembers;
amroutine->ambeginscan = btbeginscan;
amroutine->amrescan = btrescan;
amroutine->amgettuple = btgettuple;
diff --git a/src/backend/access/nbtree/nbtvalidate.c b/src/backend/access/nbtree/nbtvalidate.c
index ff634b1..3a0c089 100644
--- a/src/backend/access/nbtree/nbtvalidate.c
+++ b/src/backend/access/nbtree/nbtvalidate.c
@@ -16,12 +16,15 @@
#include "access/amvalidate.h"
#include "access/htup_details.h"
#include "access/nbtree.h"
+#include "access/xact.h"
+#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
#include "catalog/pg_amproc.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_type.h"
#include "utils/builtins.h"
+#include "utils/lsyscache.h"
#include "utils/regproc.h"
#include "utils/syscache.h"
@@ -275,3 +278,96 @@ btvalidate(Oid opclassoid)
return result;
}
+
+/*
+ * Prechecking function for adding operators/functions to a btree opfamily.
+ */
+void
+btcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions)
+{
+ Oid opcintype;
+ ListCell *lc;
+
+ /*
+ * Btree operators and comparison support functions are always "loose"
+ * members of the opfamily if they are cross-type. If they are not
+ * cross-type, we prefer to tie them to the appropriate opclass ... but if
+ * the user hasn't created one, we can't do that, and must fall back to
+ * using the opfamily dependency. (We mustn't force creation of an
+ * opclass in such a case, as leaving an incomplete opclass laying about
+ * would be bad. Throwing an error is another undesirable alternative.)
+ *
+ * This behavior results in a bit of a dump/reload hazard, in that the
+ * order of restoring objects could affect what dependencies we end up
+ * with. pg_dump's existing behavior will preserve the dependency choices
+ * in most cases, but not if a cross-type operator has been bound tightly
+ * into an opclass. That's a mistake anyway, so silently "fixing" it
+ * isn't awful.
+ *
+ * Optional support functions are always "loose" family members.
+ *
+ * To avoid repeated lookups, we remember the most recently used opclass's
+ * input type.
+ */
+ if (OidIsValid(opclassoid))
+ {
+ /* During CREATE OPERATOR CLASS, need CCI to see the pg_opclass row */
+ CommandCounterIncrement();
+ opcintype = get_opclass_input_type(opclassoid);
+ }
+ else
+ opcintype = InvalidOid;
+
+ /*
+ * We handle operators and support functions almost identically, so rather
+ * than duplicate this code block, just join the lists.
+ */
+ foreach(lc, list_concat_copy(operators, functions))
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ if (op->is_func && op->number != BTORDER_PROC)
+ {
+ /* Optional support proc, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else if (op->lefttype != op->righttype)
+ {
+ /* Cross-type, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else
+ {
+ /* Not cross-type; is there a suitable opclass? */
+ if (op->lefttype != opcintype)
+ {
+ /* Avoid repeating this expensive lookup, even if it fails */
+ opcintype = op->lefttype;
+ opclassoid = opclass_for_family_datatype(BTREE_AM_OID,
+ opfamilyoid,
+ opcintype);
+ }
+ if (OidIsValid(opclassoid))
+ {
+ /* Hard dependency on opclass */
+ op->ref_is_hard = true;
+ op->ref_is_family = false;
+ op->refobjid = opclassoid;
+ }
+ else
+ {
+ /* We're stuck, so make a soft dependency on the opfamily */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ }
+ }
+}
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index d715908..ad2fa8c 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 e316d6e..9a9e301 100644
--- a/src/backend/access/spgist/spgvalidate.c
+++ b/src/backend/access/spgist/spgvalidate.c
@@ -298,3 +298,65 @@ 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;
+ }
+
+ /*
+ * Required support functions should have hard dependencies. Preferably
+ * those are just dependencies on the opclass, but if we're in ALTER
+ * OPERATOR FAMILY, we leave the dependency pointing at the whole
+ * opfamily. (Given that SP-GiST opclasses generally don't share
+ * opfamilies, it seems unlikely to be worth working harder.)
+ */
+ foreach(lc, functions)
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ switch (op->number)
+ {
+ case SPGIST_CONFIG_PROC:
+ case SPGIST_CHOOSE_PROC:
+ case SPGIST_PICKSPLIT_PROC:
+ case SPGIST_INNER_CONSISTENT_PROC:
+ case SPGIST_LEAF_CONSISTENT_PROC:
+ /* Required support function */
+ op->ref_is_hard = true;
+ break;
+ case SPGIST_COMPRESS_PROC:
+ /* Optional, so force it to be a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ break;
+ default:
+ /* Probably we should throw error here */
+ break;
+ }
+ }
+}
diff --git a/src/backend/commands/opclasscmds.c b/src/backend/commands/opclasscmds.c
index e2c6de4..9706208 100644
--- a/src/backend/commands/opclasscmds.c
+++ b/src/backend/commands/opclasscmds.c
@@ -27,7 +27,6 @@
#include "catalog/dependency.h"
#include "catalog/indexing.h"
#include "catalog/objectaccess.h"
-#include "catalog/opfam_internal.h"
#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
#include "catalog/pg_amproc.h"
@@ -61,12 +60,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);
@@ -515,11 +512,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)
@@ -539,6 +537,7 @@ DefineOpClass(CreateOpClassStmt *stmt)
/* Save the info */
member = (OpFamilyMember *) palloc0(sizeof(OpFamilyMember));
+ member->is_func = true;
member->object = funcOid;
member->number = item->number;
@@ -548,7 +547,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))
@@ -661,13 +660,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);
@@ -836,6 +867,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;
@@ -894,11 +926,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)
@@ -918,8 +956,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)
@@ -927,7 +971,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,
@@ -941,13 +985,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,
@@ -990,10 +1044,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)
@@ -1005,10 +1060,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 */
@@ -1263,7 +1319,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;
@@ -1275,7 +1331,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",
@@ -1297,13 +1353,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;
@@ -1373,28 +1426,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 amapi.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))
@@ -1402,8 +1444,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);
@@ -1415,13 +1460,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;
@@ -1485,28 +1527,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 amapi.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/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 4a9764c..3082a7b 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -523,6 +523,7 @@ my %tests = (
OPERATOR 5 >(bigint,int4),
FUNCTION 1 (int4, int4) btint4cmp(int4,int4),
FUNCTION 2 (int4, int4) btint4sortsupport(internal);',
+ # note: it's correct that btint8sortsupport is included here:
regexp => qr/^
\QALTER OPERATOR FAMILY dump_test.op_family USING btree ADD\E\n\s+
\QOPERATOR 1 <(bigint,integer) ,\E\n\s+
@@ -531,6 +532,7 @@ my %tests = (
\QOPERATOR 4 >=(bigint,integer) ,\E\n\s+
\QOPERATOR 5 >(bigint,integer) ,\E\n\s+
\QFUNCTION 1 (integer, integer) btint4cmp(integer,integer) ,\E\n\s+
+ \QFUNCTION 2 (bigint, bigint) btint8sortsupport(internal) ,\E\n\s+
\QFUNCTION 2 (integer, integer) btint4sortsupport(internal);\E
/xm,
like =>
@@ -1555,6 +1557,7 @@ my %tests = (
OPERATOR 5 >(bigint,bigint),
FUNCTION 1 btint8cmp(bigint,bigint),
FUNCTION 2 btint8sortsupport(internal);',
+ # note: it's correct that btint8sortsupport isn't included here:
regexp => qr/^
\QCREATE OPERATOR CLASS dump_test.op_class\E\n\s+
\QFOR TYPE bigint USING btree FAMILY dump_test.op_family AS\E\n\s+
@@ -1563,8 +1566,7 @@ my %tests = (
\QOPERATOR 3 =(bigint,bigint) ,\E\n\s+
\QOPERATOR 4 >=(bigint,bigint) ,\E\n\s+
\QOPERATOR 5 >(bigint,bigint) ,\E\n\s+
- \QFUNCTION 1 (bigint, bigint) btint8cmp(bigint,bigint) ,\E\n\s+
- \QFUNCTION 2 (bigint, bigint) btint8sortsupport(internal);\E
+ \QFUNCTION 1 (bigint, bigint) btint8cmp(bigint,bigint);\E
/xm,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index d2a49e8..b142aee 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -54,6 +54,42 @@ typedef enum IndexAMProperty
AMPROP_CAN_INCLUDE
} IndexAMProperty;
+/*
+ * We use lists of this struct type to keep track of both operators and
+ * support functions while building or adding to an opclass or opfamily.
+ * amcheckmembers functions receive lists of these structs, and are allowed
+ * to alter their "ref" fields.
+ *
+ * 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 OpFamilyMember
+{
+ 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;
+
/*
* Callback function signatures --- see indexam.sgml for more info.
@@ -114,6 +150,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 +260,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/amvalidate.h b/src/include/access/amvalidate.h
index c6c60e0..707a705 100644
--- a/src/include/access/amvalidate.h
+++ b/src/include/access/amvalidate.h
@@ -1,7 +1,8 @@
/*-------------------------------------------------------------------------
*
* amvalidate.h
- * Support routines for index access methods' amvalidate functions.
+ * Support routines for index access methods' amvalidate and
+ * amcheckmembers functions.
*
* Copyright (c) 2016-2020, PostgreSQL Global Development Group
*
@@ -31,6 +32,8 @@ extern bool check_amproc_signature(Oid funcid, Oid restype, bool exact,
int minargs, int maxargs,...);
extern bool check_amop_signature(Oid opno, Oid restype,
Oid lefttype, Oid righttype);
+extern Oid opclass_for_family_datatype(Oid amoid, Oid opfamilyoid,
+ Oid datatypeoid);
extern bool opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid);
#endif /* AMVALIDATE_H */
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index a136f7f..352b8db 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -393,6 +393,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 18f2b0d..86cf332 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -462,6 +462,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/hash.h b/src/include/access/hash.h
index 9fc0696..173cbf6 100644
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -378,6 +378,10 @@ extern IndexBulkDeleteResult *hashvacuumcleanup(IndexVacuumInfo *info,
IndexBulkDeleteResult *stats);
extern bytea *hashoptions(Datum reloptions, bool validate);
extern bool hashvalidate(Oid opclassoid);
+extern void hashcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions);
/* private routines */
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index f90ee3a..11afb04 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -835,6 +835,10 @@ extern void _bt_check_third_page(Relation rel, Relation heap,
* prototypes for functions in nbtvalidate.c
*/
extern bool btvalidate(Oid opclassoid);
+extern void btcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions);
/*
* prototypes for functions in nbtsort.c
diff --git a/src/include/access/spgist.h b/src/include/access/spgist.h
index f48080b..c95c67c 100644
--- a/src/include/access/spgist.h
+++ b/src/include/access/spgist.h
@@ -219,5 +219,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
deleted file mode 100644
index d63bd9f..0000000
--- a/src/include/catalog/opfam_internal.h
+++ /dev/null
@@ -1,28 +0,0 @@
-/*-------------------------------------------------------------------------
- *
- * opfam_internal.h
- *
- * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
- *
- * src/include/catalog/opfam_internal.h
- *
- *-------------------------------------------------------------------------
- */
-#ifndef OPFAM_INTERNAL_H
-#define OPFAM_INTERNAL_H
-
-/*
- * We use lists of this struct type to keep track of both operators and
- * procedures while building or adding to an opfamily.
- */
-typedef struct
-{
- Oid object; /* operator or support proc's OID */
- int number; /* strategy or support proc number */
- Oid lefttype; /* lefttype */
- Oid righttype; /* righttype */
- Oid sortfamily; /* ordering operator's sort opfamily, or 0 */
-} OpFamilyMember;
-
-#endif /* OPFAM_INTERNAL_H */
On Sun, Jan 05, 2020 at 12:33:10PM -0500, Tom Lane wrote: >Tomas Vondra <tomas.vondra@2ndquadrant.com> writes: >> The latest version of this patch (from 2019/09/14) no longer applies, >> although maybe it's some issue with patch format (applying it using >> patch works fine, git am fails with "Patch format detection failed."). > >Hm, seems to be just a trivial conflict against the copyright-date-update >patch. Updated version attached. > Interesting. I still get $ git am ~/am-check-members-callback-3.patch Patch format detection failed. I'm on git 2.21.1, not sure if that matters. Cputube is happy, though. Meh. >> On Sat, Sep 14, 2019 at 07:01:33PM -0400, Tom Lane wrote: >>> This change results in a possibly surprising change in the expected output >>> for the 002_pg_dump.pl test: an optional support function that had been >>> created as part of CREATE OPERATOR CLASS will now be dumped as part of >>> ALTER OPERATOR FAMILY. Maybe that's too surprising? Another approach >>> that we could use is to give up the premise that soft dependencies are >>> always on the opfamily. If we kept the dependencies pointing to the >>> same objects as before (opclass or opfamily) and only twiddled the >>> dependency strength, then pg_dump's output would not change. Now, >>> this would possibly result in dropping a still-useful family member >>> if it were incorrectly tied to an opclass that's dropped --- but that >>> would have happened before, too. I'm not quite sure if we really want >>> to editorialize on the user's decisions about which grouping to tie >>> family members to. > >> I agree it's a bit weird to add a dependency on an opfamily and not the >> opclass. Not just because of the pg_dump weirdness, but doesn't it mean >> that after a DROP OPERATOR CLASS we might still reject a DROP OPERATOR >> because of the remaining opfamily dependency? (Haven't tried, so maybe >> that works fine). > >I poked at the idea of retaining the user's decisions as to whether >a member object is a member of an individual opclass or an opfamily, >but soon realized that there's a big problem with that: we don't have >any ALTER OPERATOR CLASS ADD/DROP syntax, only ALTER OPERATOR FAMILY. >So there's no way to express the concept of "add this at the opclass >level", if you forgot to add it during initial opclass creation. > >I suppose that some case could be made for adding such syntax, but >it seems like a significant amount of work, and in the end it seems >like it's better to trust the system to get these assignments right. >Letting the user do it doesn't add much except the opportunity >to shoot oneself in the foot. > OK. So we shall keep the v2 behavior, with opfamily dependencies and modified pg_dump output? Fine with me - I still think it's a bit weird, but I'm willing to commit myself to add the missing syntax. And I doubt anyone will notice, probably ... >> One minor comment from me is that maybe "amcheckmembers" is a bit >> misleading. In my mind "check" implies a plain passive check, not >> something that may actually tweak the dependency type. > >Hmm. I'm not wedded to that name, but do you have a better proposal? >The end goal (not realized in this patch, of course) is that these >callbacks would perform fairly thorough checking of opclass members, >missing only the ability to check that all required members are present. >So I don't want to name them something like "amfixdependencies", even >if that's all they're doing right now. > OK. > >I see your point that "check" suggests a read-only operation, but >I'm not sure about a better verb. I thought of "amvalidatemembers", >but that's not really much better than "check" is it? > I don't :-( regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> On Sun, Jan 05, 2020 at 12:33:10PM -0500, Tom Lane wrote:
>> I see your point that "check" suggests a read-only operation, but
>> I'm not sure about a better verb. I thought of "amvalidatemembers",
>> but that's not really much better than "check" is it?
> I don't :-(
Still haven't got a better naming idea, but in the meantime here's
a rebase to fix a conflict with 612a1ab76.
regards, tom lane
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index 0104d02..1098ab7 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -138,6 +138,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 37f8d87..d76d17d 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -141,6 +141,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 */
@@ -500,7 +501,48 @@ 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>amapi.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. Optional support functions are typically
+ also given soft dependencies, so that they can be removed if necessary.
</para>
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 2e8f67e..d1e1176 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -118,6 +118,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 a7e55ca..e61e629 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -70,6 +70,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 0b62e0a..f44aa4d 100644
--- a/src/backend/access/gin/ginvalidate.c
+++ b/src/backend/access/gin/ginvalidate.c
@@ -267,3 +267,64 @@ 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;
+ }
+
+ /*
+ * Required support functions should have hard dependencies. Preferably
+ * those are just dependencies on the opclass, but if we're in ALTER
+ * OPERATOR FAMILY, we leave the dependency pointing at the whole
+ * opfamily. (Given that GIN opclasses generally don't share opfamilies,
+ * it seems unlikely to be worth working harder.)
+ */
+ foreach(lc, functions)
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ switch (op->number)
+ {
+ case GIN_EXTRACTVALUE_PROC:
+ case GIN_EXTRACTQUERY_PROC:
+ /* Required support function */
+ op->ref_is_hard = true;
+ break;
+ case GIN_COMPARE_PROC:
+ case GIN_CONSISTENT_PROC:
+ case GIN_COMPARE_PARTIAL_PROC:
+ case GIN_TRICONSISTENT_PROC:
+ /* Optional, so force it to be a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ break;
+ default:
+ /* Probably we should throw error here */
+ break;
+ }
+ }
+}
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 90c46e8..8f28d99 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -91,6 +91,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 0c4fb8c..46c31c4 100644
--- a/src/backend/access/gist/gistvalidate.c
+++ b/src/backend/access/gist/gistvalidate.c
@@ -275,3 +275,68 @@ 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;
+ }
+
+ /*
+ * Required support functions should have hard dependencies. Preferably
+ * those are just dependencies on the opclass, but if we're in ALTER
+ * OPERATOR FAMILY, we leave the dependency pointing at the whole
+ * opfamily. (Given that GiST opclasses generally don't share opfamilies,
+ * it seems unlikely to be worth working harder.)
+ */
+ foreach(lc, functions)
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ switch (op->number)
+ {
+ case GIST_CONSISTENT_PROC:
+ case GIST_UNION_PROC:
+ case GIST_PENALTY_PROC:
+ case GIST_PICKSPLIT_PROC:
+ case GIST_EQUAL_PROC:
+ /* Required support function */
+ op->ref_is_hard = true;
+ break;
+ case GIST_COMPRESS_PROC:
+ case GIST_DECOMPRESS_PROC:
+ case GIST_DISTANCE_PROC:
+ case GIST_FETCH_PROC:
+ /* Optional, so force it to be a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ break;
+ default:
+ /* Probably we should throw error here */
+ break;
+ }
+ }
+}
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 4871b7f..a769196 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -88,6 +88,7 @@ hashhandler(PG_FUNCTION_ARGS)
amroutine->amproperty = NULL;
amroutine->ambuildphasename = NULL;
amroutine->amvalidate = hashvalidate;
+ amroutine->amcheckmembers = hashcheckmembers;
amroutine->ambeginscan = hashbeginscan;
amroutine->amrescan = hashrescan;
amroutine->amgettuple = hashgettuple;
diff --git a/src/backend/access/hash/hashvalidate.c b/src/backend/access/hash/hashvalidate.c
index 6346e65..5c8626a 100644
--- a/src/backend/access/hash/hashvalidate.c
+++ b/src/backend/access/hash/hashvalidate.c
@@ -16,6 +16,8 @@
#include "access/amvalidate.h"
#include "access/hash.h"
#include "access/htup_details.h"
+#include "access/xact.h"
+#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
#include "catalog/pg_amproc.h"
#include "catalog/pg_opclass.h"
@@ -25,6 +27,7 @@
#include "parser/parse_coerce.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
+#include "utils/lsyscache.h"
#include "utils/regproc.h"
#include "utils/syscache.h"
@@ -334,3 +337,96 @@ check_hash_func_signature(Oid funcid, int16 amprocnum, Oid argtype)
ReleaseSysCache(tp);
return result;
}
+
+/*
+ * Prechecking function for adding operators/functions to a hash opfamily.
+ */
+void
+hashcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions)
+{
+ Oid opcintype;
+ ListCell *lc;
+
+ /*
+ * Hash operators and required support functions are always "loose"
+ * members of the opfamily if they are cross-type. If they are not
+ * cross-type, we prefer to tie them to the appropriate opclass ... but if
+ * the user hasn't created one, we can't do that, and must fall back to
+ * using the opfamily dependency. (We mustn't force creation of an
+ * opclass in such a case, as leaving an incomplete opclass laying about
+ * would be bad. Throwing an error is another undesirable alternative.)
+ *
+ * This behavior results in a bit of a dump/reload hazard, in that the
+ * order of restoring objects could affect what dependencies we end up
+ * with. pg_dump's existing behavior will preserve the dependency choices
+ * in most cases, but not if a cross-type operator has been bound tightly
+ * into an opclass. That's a mistake anyway, so silently "fixing" it
+ * isn't awful.
+ *
+ * Optional support functions are always "loose" family members.
+ *
+ * To avoid repeated lookups, we remember the most recently used opclass's
+ * input type.
+ */
+ if (OidIsValid(opclassoid))
+ {
+ /* During CREATE OPERATOR CLASS, need CCI to see the pg_opclass row */
+ CommandCounterIncrement();
+ opcintype = get_opclass_input_type(opclassoid);
+ }
+ else
+ opcintype = InvalidOid;
+
+ /*
+ * We handle operators and support functions almost identically, so rather
+ * than duplicate this code block, just join the lists.
+ */
+ foreach(lc, list_concat_copy(operators, functions))
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ if (op->is_func && op->number != HASHSTANDARD_PROC)
+ {
+ /* Optional support proc, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else if (op->lefttype != op->righttype)
+ {
+ /* Cross-type, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else
+ {
+ /* Not cross-type; is there a suitable opclass? */
+ if (op->lefttype != opcintype)
+ {
+ /* Avoid repeating this expensive lookup, even if it fails */
+ opcintype = op->lefttype;
+ opclassoid = opclass_for_family_datatype(HASH_AM_OID,
+ opfamilyoid,
+ opcintype);
+ }
+ if (OidIsValid(opclassoid))
+ {
+ /* Hard dependency on opclass */
+ op->ref_is_hard = true;
+ op->ref_is_family = false;
+ op->refobjid = opclassoid;
+ }
+ else
+ {
+ /* We're stuck, so make a soft dependency on the opfamily */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ }
+ }
+}
diff --git a/src/backend/access/index/amvalidate.c b/src/backend/access/index/amvalidate.c
index 3eae6aa..fa1f9d8 100644
--- a/src/backend/access/index/amvalidate.c
+++ b/src/backend/access/index/amvalidate.c
@@ -1,7 +1,8 @@
/*-------------------------------------------------------------------------
*
* amvalidate.c
- * Support routines for index access methods' amvalidate functions.
+ * Support routines for index access methods' amvalidate and
+ * amcheckmembers functions.
*
* Copyright (c) 2016-2020, PostgreSQL Global Development Group
*
@@ -211,21 +212,28 @@ check_amop_signature(Oid opno, Oid restype, Oid lefttype, Oid righttype)
}
/*
- * Is the datatype a legitimate input type for the btree opfamily?
+ * Get the OID of the opclass belonging to an opfamily and accepting
+ * the specified type as input type. Returns InvalidOid if no such opclass.
+ *
+ * If there is more than one such opclass, you get a random one of them.
+ * Since that shouldn't happen, we don't waste cycles checking.
+ *
+ * We could look up the AM's OID from the opfamily, but all existing callers
+ * know that or can get it without an extra lookup, so we make them pass it.
*/
-bool
-opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
+Oid
+opclass_for_family_datatype(Oid amoid, Oid opfamilyoid, Oid datatypeoid)
{
- bool result = false;
+ Oid result = InvalidOid;
CatCList *opclist;
int i;
/*
- * We search through all btree opclasses to see if one matches. This is a
- * bit inefficient but there is no better index available. It also saves
- * making an explicit check that the opfamily belongs to btree.
+ * We search through all the AM's opclasses to see if one matches. This
+ * is a bit inefficient but there is no better index available. It also
+ * saves making an explicit check that the opfamily belongs to the AM.
*/
- opclist = SearchSysCacheList1(CLAAMNAMENSP, ObjectIdGetDatum(BTREE_AM_OID));
+ opclist = SearchSysCacheList1(CLAAMNAMENSP, ObjectIdGetDatum(amoid));
for (i = 0; i < opclist->n_members; i++)
{
@@ -235,7 +243,7 @@ opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
if (classform->opcfamily == opfamilyoid &&
classform->opcintype == datatypeoid)
{
- result = true;
+ result = classform->oid;
break;
}
}
@@ -244,3 +252,14 @@ opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
return result;
}
+
+/*
+ * Is the datatype a legitimate input type for the btree opfamily?
+ */
+bool
+opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
+{
+ return OidIsValid(opclass_for_family_datatype(BTREE_AM_OID,
+ opfamilyoid,
+ datatypeoid));
+}
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4bb1629..ae8e0d9 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -141,6 +141,7 @@ bthandler(PG_FUNCTION_ARGS)
amroutine->amproperty = btproperty;
amroutine->ambuildphasename = btbuildphasename;
amroutine->amvalidate = btvalidate;
+ amroutine->amcheckmembers = btcheckmembers;
amroutine->ambeginscan = btbeginscan;
amroutine->amrescan = btrescan;
amroutine->amgettuple = btgettuple;
diff --git a/src/backend/access/nbtree/nbtvalidate.c b/src/backend/access/nbtree/nbtvalidate.c
index 627f744..c18dadd 100644
--- a/src/backend/access/nbtree/nbtvalidate.c
+++ b/src/backend/access/nbtree/nbtvalidate.c
@@ -16,12 +16,15 @@
#include "access/amvalidate.h"
#include "access/htup_details.h"
#include "access/nbtree.h"
+#include "access/xact.h"
+#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
#include "catalog/pg_amproc.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_type.h"
#include "utils/builtins.h"
+#include "utils/lsyscache.h"
#include "utils/regproc.h"
#include "utils/syscache.h"
@@ -279,3 +282,96 @@ btvalidate(Oid opclassoid)
return result;
}
+
+/*
+ * Prechecking function for adding operators/functions to a btree opfamily.
+ */
+void
+btcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions)
+{
+ Oid opcintype;
+ ListCell *lc;
+
+ /*
+ * Btree operators and comparison support functions are always "loose"
+ * members of the opfamily if they are cross-type. If they are not
+ * cross-type, we prefer to tie them to the appropriate opclass ... but if
+ * the user hasn't created one, we can't do that, and must fall back to
+ * using the opfamily dependency. (We mustn't force creation of an
+ * opclass in such a case, as leaving an incomplete opclass laying about
+ * would be bad. Throwing an error is another undesirable alternative.)
+ *
+ * This behavior results in a bit of a dump/reload hazard, in that the
+ * order of restoring objects could affect what dependencies we end up
+ * with. pg_dump's existing behavior will preserve the dependency choices
+ * in most cases, but not if a cross-type operator has been bound tightly
+ * into an opclass. That's a mistake anyway, so silently "fixing" it
+ * isn't awful.
+ *
+ * Optional support functions are always "loose" family members.
+ *
+ * To avoid repeated lookups, we remember the most recently used opclass's
+ * input type.
+ */
+ if (OidIsValid(opclassoid))
+ {
+ /* During CREATE OPERATOR CLASS, need CCI to see the pg_opclass row */
+ CommandCounterIncrement();
+ opcintype = get_opclass_input_type(opclassoid);
+ }
+ else
+ opcintype = InvalidOid;
+
+ /*
+ * We handle operators and support functions almost identically, so rather
+ * than duplicate this code block, just join the lists.
+ */
+ foreach(lc, list_concat_copy(operators, functions))
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ if (op->is_func && op->number != BTORDER_PROC)
+ {
+ /* Optional support proc, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else if (op->lefttype != op->righttype)
+ {
+ /* Cross-type, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else
+ {
+ /* Not cross-type; is there a suitable opclass? */
+ if (op->lefttype != opcintype)
+ {
+ /* Avoid repeating this expensive lookup, even if it fails */
+ opcintype = op->lefttype;
+ opclassoid = opclass_for_family_datatype(BTREE_AM_OID,
+ opfamilyoid,
+ opcintype);
+ }
+ if (OidIsValid(opclassoid))
+ {
+ /* Hard dependency on opclass */
+ op->ref_is_hard = true;
+ op->ref_is_family = false;
+ op->refobjid = opclassoid;
+ }
+ else
+ {
+ /* We're stuck, so make a soft dependency on the opfamily */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ }
+ }
+}
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 4924ae1..800b96c 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -73,6 +73,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 e316d6e..9a9e301 100644
--- a/src/backend/access/spgist/spgvalidate.c
+++ b/src/backend/access/spgist/spgvalidate.c
@@ -298,3 +298,65 @@ 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;
+ }
+
+ /*
+ * Required support functions should have hard dependencies. Preferably
+ * those are just dependencies on the opclass, but if we're in ALTER
+ * OPERATOR FAMILY, we leave the dependency pointing at the whole
+ * opfamily. (Given that SP-GiST opclasses generally don't share
+ * opfamilies, it seems unlikely to be worth working harder.)
+ */
+ foreach(lc, functions)
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ switch (op->number)
+ {
+ case SPGIST_CONFIG_PROC:
+ case SPGIST_CHOOSE_PROC:
+ case SPGIST_PICKSPLIT_PROC:
+ case SPGIST_INNER_CONSISTENT_PROC:
+ case SPGIST_LEAF_CONSISTENT_PROC:
+ /* Required support function */
+ op->ref_is_hard = true;
+ break;
+ case SPGIST_COMPRESS_PROC:
+ /* Optional, so force it to be a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ break;
+ default:
+ /* Probably we should throw error here */
+ break;
+ }
+ }
+}
diff --git a/src/backend/commands/opclasscmds.c b/src/backend/commands/opclasscmds.c
index 743511b..618b822 100644
--- a/src/backend/commands/opclasscmds.c
+++ b/src/backend/commands/opclasscmds.c
@@ -27,7 +27,6 @@
#include "catalog/dependency.h"
#include "catalog/indexing.h"
#include "catalog/objectaccess.h"
-#include "catalog/opfam_internal.h"
#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
#include "catalog/pg_amproc.h"
@@ -61,12 +60,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);
@@ -515,11 +512,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)
@@ -539,6 +537,7 @@ DefineOpClass(CreateOpClassStmt *stmt)
/* Save the info */
member = (OpFamilyMember *) palloc0(sizeof(OpFamilyMember));
+ member->is_func = true;
member->object = funcOid;
member->number = item->number;
@@ -548,7 +547,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))
@@ -661,13 +660,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);
@@ -836,6 +867,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;
@@ -894,11 +926,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)
@@ -918,8 +956,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)
@@ -927,7 +971,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,
@@ -941,13 +985,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,
@@ -990,10 +1044,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)
@@ -1005,10 +1060,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 */
@@ -1287,7 +1343,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;
@@ -1299,7 +1355,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",
@@ -1321,13 +1377,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;
@@ -1397,28 +1450,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 amapi.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))
@@ -1426,8 +1468,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);
@@ -1439,13 +1484,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;
@@ -1509,28 +1551,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 amapi.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/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 1b90cbd..25fb77f 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -524,6 +524,8 @@ my %tests = (
FUNCTION 1 (int4, int4) btint4cmp(int4,int4),
FUNCTION 2 (int4, int4) btint4sortsupport(internal),
FUNCTION 4 (int4, int4) btequalimage(oid);',
+ # note: it's correct that btint8sortsupport and bigint btequalimage
+ # are included here:
regexp => qr/^
\QALTER OPERATOR FAMILY dump_test.op_family USING btree ADD\E\n\s+
\QOPERATOR 1 <(bigint,integer) ,\E\n\s+
@@ -532,7 +534,9 @@ my %tests = (
\QOPERATOR 4 >=(bigint,integer) ,\E\n\s+
\QOPERATOR 5 >(bigint,integer) ,\E\n\s+
\QFUNCTION 1 (integer, integer) btint4cmp(integer,integer) ,\E\n\s+
+ \QFUNCTION 2 (bigint, bigint) btint8sortsupport(internal) ,\E\n\s+
\QFUNCTION 2 (integer, integer) btint4sortsupport(internal) ,\E\n\s+
+ \QFUNCTION 4 (bigint, bigint) btequalimage(oid) ,\E\n\s+
\QFUNCTION 4 (integer, integer) btequalimage(oid);\E
/xm,
like =>
@@ -1558,6 +1562,8 @@ my %tests = (
FUNCTION 1 btint8cmp(bigint,bigint),
FUNCTION 2 btint8sortsupport(internal),
FUNCTION 4 btequalimage(oid);',
+ # note: it's correct that btint8sortsupport and btequalimage
+ # are NOT included here (they're optional support functions):
regexp => qr/^
\QCREATE OPERATOR CLASS dump_test.op_class\E\n\s+
\QFOR TYPE bigint USING btree FAMILY dump_test.op_family AS\E\n\s+
@@ -1566,9 +1572,7 @@ my %tests = (
\QOPERATOR 3 =(bigint,bigint) ,\E\n\s+
\QOPERATOR 4 >=(bigint,bigint) ,\E\n\s+
\QOPERATOR 5 >(bigint,bigint) ,\E\n\s+
- \QFUNCTION 1 (bigint, bigint) btint8cmp(bigint,bigint) ,\E\n\s+
- \QFUNCTION 2 (bigint, bigint) btint8sortsupport(internal) ,\E\n\s+
- \QFUNCTION 4 (bigint, bigint) btequalimage(oid);\E
+ \QFUNCTION 1 (bigint, bigint) btint8cmp(bigint,bigint);\E
/xm,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 3b3e22f..2b3ad86 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -54,6 +54,42 @@ typedef enum IndexAMProperty
AMPROP_CAN_INCLUDE
} IndexAMProperty;
+/*
+ * We use lists of this struct type to keep track of both operators and
+ * support functions while building or adding to an opclass or opfamily.
+ * amcheckmembers functions receive lists of these structs, and are allowed
+ * to alter their "ref" fields.
+ *
+ * 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 OpFamilyMember
+{
+ 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;
+
/*
* Callback function signatures --- see indexam.sgml for more info.
@@ -114,6 +150,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,
@@ -222,6 +264,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/amvalidate.h b/src/include/access/amvalidate.h
index c6c60e0..707a705 100644
--- a/src/include/access/amvalidate.h
+++ b/src/include/access/amvalidate.h
@@ -1,7 +1,8 @@
/*-------------------------------------------------------------------------
*
* amvalidate.h
- * Support routines for index access methods' amvalidate functions.
+ * Support routines for index access methods' amvalidate and
+ * amcheckmembers functions.
*
* Copyright (c) 2016-2020, PostgreSQL Global Development Group
*
@@ -31,6 +32,8 @@ extern bool check_amproc_signature(Oid funcid, Oid restype, bool exact,
int minargs, int maxargs,...);
extern bool check_amop_signature(Oid opno, Oid restype,
Oid lefttype, Oid righttype);
+extern Oid opclass_for_family_datatype(Oid amoid, Oid opfamilyoid,
+ Oid datatypeoid);
extern bool opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid);
#endif /* AMVALIDATE_H */
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 71eeac2..ec8678a 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -407,6 +407,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 18f2b0d..86cf332 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -462,6 +462,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/hash.h b/src/include/access/hash.h
index 2707e19..75e8e06 100644
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -378,6 +378,10 @@ extern IndexBulkDeleteResult *hashvacuumcleanup(IndexVacuumInfo *info,
IndexBulkDeleteResult *stats);
extern bytea *hashoptions(Datum reloptions, bool validate);
extern bool hashvalidate(Oid opclassoid);
+extern void hashcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions);
/* private routines */
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index bfe49f4..cd09c2e 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -1145,6 +1145,10 @@ extern bool _bt_allequalimage(Relation rel, bool debugmessage);
* prototypes for functions in nbtvalidate.c
*/
extern bool btvalidate(Oid opclassoid);
+extern void btcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions);
/*
* prototypes for functions in nbtsort.c
diff --git a/src/include/access/spgist.h b/src/include/access/spgist.h
index f48080b..c95c67c 100644
--- a/src/include/access/spgist.h
+++ b/src/include/access/spgist.h
@@ -219,5 +219,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
deleted file mode 100644
index d63bd9f..0000000
--- a/src/include/catalog/opfam_internal.h
+++ /dev/null
@@ -1,28 +0,0 @@
-/*-------------------------------------------------------------------------
- *
- * opfam_internal.h
- *
- * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
- *
- * src/include/catalog/opfam_internal.h
- *
- *-------------------------------------------------------------------------
- */
-#ifndef OPFAM_INTERNAL_H
-#define OPFAM_INTERNAL_H
-
-/*
- * We use lists of this struct type to keep track of both operators and
- * procedures while building or adding to an opfamily.
- */
-typedef struct
-{
- Oid object; /* operator or support proc's OID */
- int number; /* strategy or support proc number */
- Oid lefttype; /* lefttype */
- Oid righttype; /* righttype */
- Oid sortfamily; /* ordering operator's sort opfamily, or 0 */
-} OpFamilyMember;
-
-#endif /* OPFAM_INTERNAL_H */
I wrote:
> Still haven't got a better naming idea, but in the meantime here's
> a rebase to fix a conflict with 612a1ab76.
I see from the cfbot that this needs another rebase, so here 'tis.
No changes in the patch itself.
regards, tom lane
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index d3bf866..663ff7e 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -139,6 +139,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 37f8d87..d76d17d 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -141,6 +141,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 */
@@ -500,7 +501,48 @@ 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>amapi.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. Optional support functions are typically
+ also given soft dependencies, so that they can be removed if necessary.
</para>
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 7db3ae5..b911029 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -120,6 +120,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 a400f1f..26aa152 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -71,6 +71,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 1e3046f..739006a 100644
--- a/src/backend/access/gin/ginvalidate.c
+++ b/src/backend/access/gin/ginvalidate.c
@@ -271,3 +271,64 @@ 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;
+ }
+
+ /*
+ * Required support functions should have hard dependencies. Preferably
+ * those are just dependencies on the opclass, but if we're in ALTER
+ * OPERATOR FAMILY, we leave the dependency pointing at the whole
+ * opfamily. (Given that GIN opclasses generally don't share opfamilies,
+ * it seems unlikely to be worth working harder.)
+ */
+ foreach(lc, functions)
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ switch (op->number)
+ {
+ case GIN_EXTRACTVALUE_PROC:
+ case GIN_EXTRACTQUERY_PROC:
+ /* Required support function */
+ op->ref_is_hard = true;
+ break;
+ case GIN_COMPARE_PROC:
+ case GIN_CONSISTENT_PROC:
+ case GIN_COMPARE_PARTIAL_PROC:
+ case GIN_TRICONSISTENT_PROC:
+ /* Optional, so force it to be a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ break;
+ default:
+ /* Probably we should throw error here */
+ break;
+ }
+ }
+}
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 9eee538..90826c2 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -92,6 +92,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 a285736..2eac5ab 100644
--- a/src/backend/access/gist/gistvalidate.c
+++ b/src/backend/access/gist/gistvalidate.c
@@ -279,3 +279,68 @@ 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;
+ }
+
+ /*
+ * Required support functions should have hard dependencies. Preferably
+ * those are just dependencies on the opclass, but if we're in ALTER
+ * OPERATOR FAMILY, we leave the dependency pointing at the whole
+ * opfamily. (Given that GiST opclasses generally don't share opfamilies,
+ * it seems unlikely to be worth working harder.)
+ */
+ foreach(lc, functions)
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ switch (op->number)
+ {
+ case GIST_CONSISTENT_PROC:
+ case GIST_UNION_PROC:
+ case GIST_PENALTY_PROC:
+ case GIST_PICKSPLIT_PROC:
+ case GIST_EQUAL_PROC:
+ /* Required support function */
+ op->ref_is_hard = true;
+ break;
+ case GIST_COMPRESS_PROC:
+ case GIST_DECOMPRESS_PROC:
+ case GIST_DISTANCE_PROC:
+ case GIST_FETCH_PROC:
+ /* Optional, so force it to be a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ break;
+ default:
+ /* Probably we should throw error here */
+ break;
+ }
+ }
+}
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 3ec6d52..4eaba8b 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -89,6 +89,7 @@ hashhandler(PG_FUNCTION_ARGS)
amroutine->amproperty = NULL;
amroutine->ambuildphasename = NULL;
amroutine->amvalidate = hashvalidate;
+ amroutine->amcheckmembers = hashcheckmembers;
amroutine->ambeginscan = hashbeginscan;
amroutine->amrescan = hashrescan;
amroutine->amgettuple = hashgettuple;
diff --git a/src/backend/access/hash/hashvalidate.c b/src/backend/access/hash/hashvalidate.c
index 7b08ed5..b2ee7ee 100644
--- a/src/backend/access/hash/hashvalidate.c
+++ b/src/backend/access/hash/hashvalidate.c
@@ -16,6 +16,8 @@
#include "access/amvalidate.h"
#include "access/hash.h"
#include "access/htup_details.h"
+#include "access/xact.h"
+#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
#include "catalog/pg_amproc.h"
#include "catalog/pg_opclass.h"
@@ -25,6 +27,7 @@
#include "parser/parse_coerce.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
+#include "utils/lsyscache.h"
#include "utils/regproc.h"
#include "utils/syscache.h"
@@ -338,3 +341,96 @@ check_hash_func_signature(Oid funcid, int16 amprocnum, Oid argtype)
ReleaseSysCache(tp);
return result;
}
+
+/*
+ * Prechecking function for adding operators/functions to a hash opfamily.
+ */
+void
+hashcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions)
+{
+ Oid opcintype;
+ ListCell *lc;
+
+ /*
+ * Hash operators and required support functions are always "loose"
+ * members of the opfamily if they are cross-type. If they are not
+ * cross-type, we prefer to tie them to the appropriate opclass ... but if
+ * the user hasn't created one, we can't do that, and must fall back to
+ * using the opfamily dependency. (We mustn't force creation of an
+ * opclass in such a case, as leaving an incomplete opclass laying about
+ * would be bad. Throwing an error is another undesirable alternative.)
+ *
+ * This behavior results in a bit of a dump/reload hazard, in that the
+ * order of restoring objects could affect what dependencies we end up
+ * with. pg_dump's existing behavior will preserve the dependency choices
+ * in most cases, but not if a cross-type operator has been bound tightly
+ * into an opclass. That's a mistake anyway, so silently "fixing" it
+ * isn't awful.
+ *
+ * Optional support functions are always "loose" family members.
+ *
+ * To avoid repeated lookups, we remember the most recently used opclass's
+ * input type.
+ */
+ if (OidIsValid(opclassoid))
+ {
+ /* During CREATE OPERATOR CLASS, need CCI to see the pg_opclass row */
+ CommandCounterIncrement();
+ opcintype = get_opclass_input_type(opclassoid);
+ }
+ else
+ opcintype = InvalidOid;
+
+ /*
+ * We handle operators and support functions almost identically, so rather
+ * than duplicate this code block, just join the lists.
+ */
+ foreach(lc, list_concat_copy(operators, functions))
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ if (op->is_func && op->number != HASHSTANDARD_PROC)
+ {
+ /* Optional support proc, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else if (op->lefttype != op->righttype)
+ {
+ /* Cross-type, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else
+ {
+ /* Not cross-type; is there a suitable opclass? */
+ if (op->lefttype != opcintype)
+ {
+ /* Avoid repeating this expensive lookup, even if it fails */
+ opcintype = op->lefttype;
+ opclassoid = opclass_for_family_datatype(HASH_AM_OID,
+ opfamilyoid,
+ opcintype);
+ }
+ if (OidIsValid(opclassoid))
+ {
+ /* Hard dependency on opclass */
+ op->ref_is_hard = true;
+ op->ref_is_family = false;
+ op->refobjid = opclassoid;
+ }
+ else
+ {
+ /* We're stuck, so make a soft dependency on the opfamily */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ }
+ }
+}
diff --git a/src/backend/access/index/amvalidate.c b/src/backend/access/index/amvalidate.c
index 24d4975..0c5f3aa 100644
--- a/src/backend/access/index/amvalidate.c
+++ b/src/backend/access/index/amvalidate.c
@@ -1,7 +1,8 @@
/*-------------------------------------------------------------------------
*
* amvalidate.c
- * Support routines for index access methods' amvalidate functions.
+ * Support routines for index access methods' amvalidate and
+ * amcheckmembers functions.
*
* Copyright (c) 2016-2020, PostgreSQL Global Development Group
*
@@ -222,21 +223,28 @@ check_amop_signature(Oid opno, Oid restype, Oid lefttype, Oid righttype)
}
/*
- * Is the datatype a legitimate input type for the btree opfamily?
+ * Get the OID of the opclass belonging to an opfamily and accepting
+ * the specified type as input type. Returns InvalidOid if no such opclass.
+ *
+ * If there is more than one such opclass, you get a random one of them.
+ * Since that shouldn't happen, we don't waste cycles checking.
+ *
+ * We could look up the AM's OID from the opfamily, but all existing callers
+ * know that or can get it without an extra lookup, so we make them pass it.
*/
-bool
-opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
+Oid
+opclass_for_family_datatype(Oid amoid, Oid opfamilyoid, Oid datatypeoid)
{
- bool result = false;
+ Oid result = InvalidOid;
CatCList *opclist;
int i;
/*
- * We search through all btree opclasses to see if one matches. This is a
- * bit inefficient but there is no better index available. It also saves
- * making an explicit check that the opfamily belongs to btree.
+ * We search through all the AM's opclasses to see if one matches. This
+ * is a bit inefficient but there is no better index available. It also
+ * saves making an explicit check that the opfamily belongs to the AM.
*/
- opclist = SearchSysCacheList1(CLAAMNAMENSP, ObjectIdGetDatum(BTREE_AM_OID));
+ opclist = SearchSysCacheList1(CLAAMNAMENSP, ObjectIdGetDatum(amoid));
for (i = 0; i < opclist->n_members; i++)
{
@@ -246,7 +254,7 @@ opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
if (classform->opcfamily == opfamilyoid &&
classform->opcintype == datatypeoid)
{
- result = true;
+ result = classform->oid;
break;
}
}
@@ -255,3 +263,14 @@ opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
return result;
}
+
+/*
+ * Is the datatype a legitimate input type for the btree opfamily?
+ */
+bool
+opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid)
+{
+ return OidIsValid(opclass_for_family_datatype(BTREE_AM_OID,
+ opfamilyoid,
+ datatypeoid));
+}
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 3629478..52b2386 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -142,6 +142,7 @@ bthandler(PG_FUNCTION_ARGS)
amroutine->amproperty = btproperty;
amroutine->ambuildphasename = btbuildphasename;
amroutine->amvalidate = btvalidate;
+ amroutine->amcheckmembers = btcheckmembers;
amroutine->ambeginscan = btbeginscan;
amroutine->amrescan = btrescan;
amroutine->amgettuple = btgettuple;
diff --git a/src/backend/access/nbtree/nbtvalidate.c b/src/backend/access/nbtree/nbtvalidate.c
index 02905f7..1dba4e4 100644
--- a/src/backend/access/nbtree/nbtvalidate.c
+++ b/src/backend/access/nbtree/nbtvalidate.c
@@ -16,12 +16,15 @@
#include "access/amvalidate.h"
#include "access/htup_details.h"
#include "access/nbtree.h"
+#include "access/xact.h"
+#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
#include "catalog/pg_amproc.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_type.h"
#include "utils/builtins.h"
+#include "utils/lsyscache.h"
#include "utils/regproc.h"
#include "utils/syscache.h"
@@ -282,3 +285,96 @@ btvalidate(Oid opclassoid)
return result;
}
+
+/*
+ * Prechecking function for adding operators/functions to a btree opfamily.
+ */
+void
+btcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions)
+{
+ Oid opcintype;
+ ListCell *lc;
+
+ /*
+ * Btree operators and comparison support functions are always "loose"
+ * members of the opfamily if they are cross-type. If they are not
+ * cross-type, we prefer to tie them to the appropriate opclass ... but if
+ * the user hasn't created one, we can't do that, and must fall back to
+ * using the opfamily dependency. (We mustn't force creation of an
+ * opclass in such a case, as leaving an incomplete opclass laying about
+ * would be bad. Throwing an error is another undesirable alternative.)
+ *
+ * This behavior results in a bit of a dump/reload hazard, in that the
+ * order of restoring objects could affect what dependencies we end up
+ * with. pg_dump's existing behavior will preserve the dependency choices
+ * in most cases, but not if a cross-type operator has been bound tightly
+ * into an opclass. That's a mistake anyway, so silently "fixing" it
+ * isn't awful.
+ *
+ * Optional support functions are always "loose" family members.
+ *
+ * To avoid repeated lookups, we remember the most recently used opclass's
+ * input type.
+ */
+ if (OidIsValid(opclassoid))
+ {
+ /* During CREATE OPERATOR CLASS, need CCI to see the pg_opclass row */
+ CommandCounterIncrement();
+ opcintype = get_opclass_input_type(opclassoid);
+ }
+ else
+ opcintype = InvalidOid;
+
+ /*
+ * We handle operators and support functions almost identically, so rather
+ * than duplicate this code block, just join the lists.
+ */
+ foreach(lc, list_concat_copy(operators, functions))
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ if (op->is_func && op->number != BTORDER_PROC)
+ {
+ /* Optional support proc, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else if (op->lefttype != op->righttype)
+ {
+ /* Cross-type, so always a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ else
+ {
+ /* Not cross-type; is there a suitable opclass? */
+ if (op->lefttype != opcintype)
+ {
+ /* Avoid repeating this expensive lookup, even if it fails */
+ opcintype = op->lefttype;
+ opclassoid = opclass_for_family_datatype(BTREE_AM_OID,
+ opfamilyoid,
+ opcintype);
+ }
+ if (OidIsValid(opclassoid))
+ {
+ /* Hard dependency on opclass */
+ op->ref_is_hard = true;
+ op->ref_is_family = false;
+ op->refobjid = opclassoid;
+ }
+ else
+ {
+ /* We're stuck, so make a soft dependency on the opfamily */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ }
+ }
+ }
+}
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 0efe05e..ea3be52 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -74,6 +74,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 3c433e9..a9aff99 100644
--- a/src/backend/access/spgist/spgvalidate.c
+++ b/src/backend/access/spgist/spgvalidate.c
@@ -303,3 +303,65 @@ 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;
+ }
+
+ /*
+ * Required support functions should have hard dependencies. Preferably
+ * those are just dependencies on the opclass, but if we're in ALTER
+ * OPERATOR FAMILY, we leave the dependency pointing at the whole
+ * opfamily. (Given that SP-GiST opclasses generally don't share
+ * opfamilies, it seems unlikely to be worth working harder.)
+ */
+ foreach(lc, functions)
+ {
+ OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
+
+ switch (op->number)
+ {
+ case SPGIST_CONFIG_PROC:
+ case SPGIST_CHOOSE_PROC:
+ case SPGIST_PICKSPLIT_PROC:
+ case SPGIST_INNER_CONSISTENT_PROC:
+ case SPGIST_LEAF_CONSISTENT_PROC:
+ /* Required support function */
+ op->ref_is_hard = true;
+ break;
+ case SPGIST_COMPRESS_PROC:
+ /* Optional, so force it to be a soft family dependency */
+ op->ref_is_hard = false;
+ op->ref_is_family = true;
+ op->refobjid = opfamilyoid;
+ break;
+ default:
+ /* Probably we should throw error here */
+ break;
+ }
+ }
+}
diff --git a/src/backend/commands/opclasscmds.c b/src/backend/commands/opclasscmds.c
index 7322cbc..e2ad0ed 100644
--- a/src/backend/commands/opclasscmds.c
+++ b/src/backend/commands/opclasscmds.c
@@ -27,7 +27,6 @@
#include "catalog/dependency.h"
#include "catalog/indexing.h"
#include "catalog/objectaccess.h"
-#include "catalog/opfam_internal.h"
#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
#include "catalog/pg_amproc.h"
@@ -62,12 +61,10 @@ 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,
int opclassOptsProcNum);
-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);
@@ -518,11 +515,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)
@@ -541,6 +539,7 @@ DefineOpClass(CreateOpClassStmt *stmt)
#endif
/* Save the info */
member = (OpFamilyMember *) palloc0(sizeof(OpFamilyMember));
+ member->is_func = true;
member->object = funcOid;
member->number = item->number;
@@ -550,7 +549,7 @@ DefineOpClass(CreateOpClassStmt *stmt)
&member->lefttype, &member->righttype);
assignProcTypes(member, amoid, typeoid, optsProcNumber);
- addFamilyMember(&procedures, member, true);
+ addFamilyMember(&procedures, member);
break;
case OPCLASS_ITEM_STORAGETYPE:
if (OidIsValid(storageoid))
@@ -663,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);
@@ -842,6 +873,7 @@ AlterOpFamilyAdd(AlterOpFamilyStmt *stmt, Oid amoid, Oid opfamilyoid,
int maxOpNumber, int maxProcNumber, int optsProcNumber,
List *items)
{
+ IndexAmRoutine *amroutine = GetIndexAmRoutineByAmId(amoid, false);
List *operators; /* OpFamilyMember list for operators */
List *procedures; /* OpFamilyMember list for support procs */
ListCell *l;
@@ -900,11 +932,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)
@@ -924,8 +962,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)
@@ -933,7 +977,7 @@ AlterOpFamilyAdd(AlterOpFamilyStmt *stmt, Oid amoid, Oid opfamilyoid,
&member->lefttype, &member->righttype);
assignProcTypes(member, amoid, InvalidOid, optsProcNumber);
- addFamilyMember(&procedures, member, true);
+ addFamilyMember(&procedures, member);
break;
case OPCLASS_ITEM_STORAGETYPE:
ereport(ERROR,
@@ -947,13 +991,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,
@@ -996,10 +1050,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)
@@ -1011,10 +1066,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 */
@@ -1323,7 +1379,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;
@@ -1335,7 +1391,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",
@@ -1357,13 +1413,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;
@@ -1433,28 +1486,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 amapi.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))
@@ -1462,8 +1504,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);
@@ -1475,13 +1520,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;
@@ -1545,28 +1587,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 amapi.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/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 1b90cbd..25fb77f 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -524,6 +524,8 @@ my %tests = (
FUNCTION 1 (int4, int4) btint4cmp(int4,int4),
FUNCTION 2 (int4, int4) btint4sortsupport(internal),
FUNCTION 4 (int4, int4) btequalimage(oid);',
+ # note: it's correct that btint8sortsupport and bigint btequalimage
+ # are included here:
regexp => qr/^
\QALTER OPERATOR FAMILY dump_test.op_family USING btree ADD\E\n\s+
\QOPERATOR 1 <(bigint,integer) ,\E\n\s+
@@ -532,7 +534,9 @@ my %tests = (
\QOPERATOR 4 >=(bigint,integer) ,\E\n\s+
\QOPERATOR 5 >(bigint,integer) ,\E\n\s+
\QFUNCTION 1 (integer, integer) btint4cmp(integer,integer) ,\E\n\s+
+ \QFUNCTION 2 (bigint, bigint) btint8sortsupport(internal) ,\E\n\s+
\QFUNCTION 2 (integer, integer) btint4sortsupport(internal) ,\E\n\s+
+ \QFUNCTION 4 (bigint, bigint) btequalimage(oid) ,\E\n\s+
\QFUNCTION 4 (integer, integer) btequalimage(oid);\E
/xm,
like =>
@@ -1558,6 +1562,8 @@ my %tests = (
FUNCTION 1 btint8cmp(bigint,bigint),
FUNCTION 2 btint8sortsupport(internal),
FUNCTION 4 btequalimage(oid);',
+ # note: it's correct that btint8sortsupport and btequalimage
+ # are NOT included here (they're optional support functions):
regexp => qr/^
\QCREATE OPERATOR CLASS dump_test.op_class\E\n\s+
\QFOR TYPE bigint USING btree FAMILY dump_test.op_family AS\E\n\s+
@@ -1566,9 +1572,7 @@ my %tests = (
\QOPERATOR 3 =(bigint,bigint) ,\E\n\s+
\QOPERATOR 4 >=(bigint,bigint) ,\E\n\s+
\QOPERATOR 5 >(bigint,bigint) ,\E\n\s+
- \QFUNCTION 1 (bigint, bigint) btint8cmp(bigint,bigint) ,\E\n\s+
- \QFUNCTION 2 (bigint, bigint) btint8sortsupport(internal) ,\E\n\s+
- \QFUNCTION 4 (bigint, bigint) btequalimage(oid);\E
+ \QFUNCTION 1 (bigint, bigint) btint8cmp(bigint,bigint);\E
/xm,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 4325faa..6538fc8 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -54,6 +54,42 @@ typedef enum IndexAMProperty
AMPROP_CAN_INCLUDE
} IndexAMProperty;
+/*
+ * We use lists of this struct type to keep track of both operators and
+ * support functions while building or adding to an opclass or opfamily.
+ * amcheckmembers functions receive lists of these structs, and are allowed
+ * to alter their "ref" fields.
+ *
+ * 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 OpFamilyMember
+{
+ 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;
+
/*
* Callback function signatures --- see indexam.sgml for more info.
@@ -114,6 +150,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,
@@ -224,6 +266,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/amvalidate.h b/src/include/access/amvalidate.h
index f3a0e52..670991d 100644
--- a/src/include/access/amvalidate.h
+++ b/src/include/access/amvalidate.h
@@ -1,7 +1,8 @@
/*-------------------------------------------------------------------------
*
* amvalidate.h
- * Support routines for index access methods' amvalidate functions.
+ * Support routines for index access methods' amvalidate and
+ * amcheckmembers functions.
*
* Copyright (c) 2016-2020, PostgreSQL Global Development Group
*
@@ -32,6 +33,8 @@ extern bool check_amproc_signature(Oid funcid, Oid restype, bool exact,
extern bool check_amoptsproc_signature(Oid funcid);
extern bool check_amop_signature(Oid opno, Oid restype,
Oid lefttype, Oid righttype);
+extern Oid opclass_for_family_datatype(Oid amoid, Oid opfamilyoid,
+ Oid datatypeoid);
extern bool opfamily_can_sort_type(Oid opfamilyoid, Oid datatypeoid);
#endif /* AMVALIDATE_H */
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 71eeac2..ec8678a 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -407,6 +407,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 18f2b0d..86cf332 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -462,6 +462,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/hash.h b/src/include/access/hash.h
index 8cda938..4824b1d 100644
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -379,6 +379,10 @@ extern IndexBulkDeleteResult *hashvacuumcleanup(IndexVacuumInfo *info,
IndexBulkDeleteResult *stats);
extern bytea *hashoptions(Datum reloptions, bool validate);
extern bool hashvalidate(Oid opclassoid);
+extern void hashcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions);
/* private routines */
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index 5f67fc0..a855fc0 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -1146,6 +1146,10 @@ extern bool _bt_allequalimage(Relation rel, bool debugmessage);
* prototypes for functions in nbtvalidate.c
*/
extern bool btvalidate(Oid opclassoid);
+extern void btcheckmembers(Oid opfamilyoid,
+ Oid opclassoid,
+ List *operators,
+ List *functions);
/*
* prototypes for functions in nbtsort.c
diff --git a/src/include/access/spgist.h b/src/include/access/spgist.h
index 852d1e2..7ca6168 100644
--- a/src/include/access/spgist.h
+++ b/src/include/access/spgist.h
@@ -220,5 +220,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
deleted file mode 100644
index d63bd9f..0000000
--- a/src/include/catalog/opfam_internal.h
+++ /dev/null
@@ -1,28 +0,0 @@
-/*-------------------------------------------------------------------------
- *
- * opfam_internal.h
- *
- * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
- *
- * src/include/catalog/opfam_internal.h
- *
- *-------------------------------------------------------------------------
- */
-#ifndef OPFAM_INTERNAL_H
-#define OPFAM_INTERNAL_H
-
-/*
- * We use lists of this struct type to keep track of both operators and
- * procedures while building or adding to an opfamily.
- */
-typedef struct
-{
- Oid object; /* operator or support proc's OID */
- int number; /* strategy or support proc number */
- Oid lefttype; /* lefttype */
- Oid righttype; /* righttype */
- Oid sortfamily; /* ordering operator's sort opfamily, or 0 */
-} OpFamilyMember;
-
-#endif /* OPFAM_INTERNAL_H */
On 2019-Aug-18, Tom Lane wrote: > * 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 created that file so that it'd be possible to interpret the struct when dealing with DDL commands in event triggers (commit b488c580aef4). The struct was previously in a .c file, and we didn't have an appropriate .h file to put it in. I think amapi.h is a great place for it. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Aug-18, Tom Lane wrote:
>> * 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 created that file so that it'd be possible to interpret the struct
> when dealing with DDL commands in event triggers (commit b488c580aef4).
> The struct was previously in a .c file, and we didn't have an
> appropriate .h file to put it in. I think amapi.h is a great place for
> it.
Yeah, later versions of the patch put it there.
regards, tom lane
On 31.03.2020 23:45, Tom Lane wrote: > I wrote: >> Still haven't got a better naming idea, but in the meantime here's >> a rebase to fix a conflict with 612a1ab76. Maybe "amadjustmembers" will work? I've looked through the patch and noticed this comment: + default: + /* Probably we should throw error here */ + break; I suggest adding an ERROR or maybe Assert, so that future developers wouldn't forget about setting dependencies. Other than that, the patch looks good to me. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: tested, failed Spec compliant: not tested Documentation: not tested I've gone through the patch and applied on the master branch, other than a few hunks, and whether as suggested upthread,the default case for "switch (op->number)" should throw an error or not, I found that bloom regression is crashing. ------------- test bloom ... FAILED (test process exited with exit code 2) 20 ms +server closed the connection unexpectedly + This probably means the server terminated abnormally + before or while processing the request. +connection to server was lost ------------- The new status of this patch is: Waiting on Author
Hamid Akhtar <hamid.akhtar@gmail.com> writes:
> I've gone through the patch and applied on the master branch, other than a few hunks, and whether as suggested
upthread,the default case for "switch (op->number)" should throw an error or not, I found that bloom regression is
crashing.
> -------------
> test bloom ... FAILED (test process exited with exit code 2) 20 ms
Hmm ... I think you must have done something wrong. For me,
am-check-members-callback-5.patch still applies cleanly (just a few
small offsets), and it passes that test as well as the rest of
check-world. The cfbot agrees [1].
Maybe you didn't "make clean" before rebuilding?
regards, tom lane
[1] https://travis-ci.org/github/postgresql-cfbot/postgresql/builds/712599990
On Tue, Jul 28, 2020 at 8:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Hamid Akhtar <hamid.akhtar@gmail.com> writes:
> I've gone through the patch and applied on the master branch, other than a few hunks, and whether as suggested upthread, the default case for "switch (op->number)" should throw an error or not, I found that bloom regression is crashing.
> -------------
> test bloom ... FAILED (test process exited with exit code 2) 20 ms
Hmm ... I think you must have done something wrong. For me,
am-check-members-callback-5.patch still applies cleanly (just a few
small offsets), and it passes that test as well as the rest of
check-world. The cfbot agrees [1].
Maybe you didn't "make clean" before rebuilding?
regards, tom lane
[1] https://travis-ci.org/github/postgresql-cfbot/postgresql/builds/712599990
I was pretty sure I did make clean before testing the patch, but perhaps I didn't as re-running it causes all tests to pass.
Sorry for the false alarm. All good with the patch.
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950 EMAIL: mailto:hamid.akhtar@highgo.ca
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950 EMAIL: mailto:hamid.akhtar@highgo.ca
SKYPE: engineeredvirus
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation: not tested Looks good to me. CORRECTION: In my previous review I had mistakenly mentioned that it was causing a server crash. Tests run perfectly fine without anyerrors. The new status of this patch is: Ready for Committer
Anastasia Lubennikova <a.lubennikova@postgrespro.ru> writes:
> On 31.03.2020 23:45, Tom Lane wrote:
>> Still haven't got a better naming idea, but in the meantime here's
>> a rebase to fix a conflict with 612a1ab76.
> Maybe "amadjustmembers" will work?
Not having any better idea, I adopted that one.
> I've looked through the patch and noticed this comment:
> + /* Probably we should throw error here */
> I suggest adding an ERROR or maybe Assert, so that future developers
> wouldn't
> forget about setting dependencies. Other than that, the patch looks good
> to me.
I'd figured that adding error checks could be left for a second pass,
but there's no strong reason not to insert these particular checks
now ... and indeed, doing so showed me that the patch hadn't been
updated to cover the recent addition of opclass options procs :-(.
So I fixed that and pushed it.
regards, tom lane