Skip to content

Commit ec7b89c

Browse files
tglsfdctglsfdc
committed
Ensure that pg_amop/amproc entries depend on their lefttype/righttype.
Usually an entry in pg_amop or pg_amproc does not need a dependency on its amoplefttype/amoprighttype/amproclefttype/amprocrighttype types, because there is an indirect dependency via the argument types of its referenced operator or procedure, or via the opclass it belongs to. However, for some support procedures in some index AMs, the argument types of the support procedure might not mention the column data type at all. Also, the amop/amproc entry might be treated as "loose" in the opfamily, in which case it lacks a dependency on any particular opclass; or it might be a cross-type entry having a reference to a datatype that is not its opclass' opcintype. The upshot of all this is that there are cases where a datatype can be dropped while leaving behind amop/amproc entries that mention it, because there is no path in pg_depend showing that those entries depend on that type. Such entries are harmless in normal activity, because they won't get used, but they cause problems for maintenance actions such as dropping the operator family. They also cause pg_dump to produce bogus output. The previous commit put a band-aid on the DROP OPERATOR FAMILY failure, but a real fix is needed. To fix, add pg_depend entries showing that a pg_amop/pg_amproc entry depends on its lefttype/righttype. To avoid bloating pg_depend too much, skip this if the referenced operator or function has that type as an input type. (I did not bother with considering the possible indirect dependency via the opclass' opcintype; at least in the reported case, that wouldn't help anyway.) Probably, the reason this has escaped notice for so long is that add-on datatypes and relevant opclasses/opfamilies are usually packaged as extensions nowadays, so that there's no way to drop a type without dropping the referencing opclasses/opfamilies too. Still, in the absence of pg_depend entries there's nothing that constrains DROP EXTENSION to drop the opfamily entries before the datatype, so it seems possible for a DROP failure to occur anyway. The specific case that was reported doesn't fail in v13, because v13 prefers to attach the support procedure to the opclass not the opfamily. But it's surely possible to construct other edge cases that do fail in v13, so patch that too. Per report from Yoran Heling. Back-patch to all supported branches. Discussion: https://postgr.es/m/Z1MVCOh1hprjK5Sf@gmai021
1 parent 5b44a31 commit ec7b89c

File tree

2 files changed

+102
-0
lines changed

2 files changed

+102
-0
lines changed

‎src/backend/commands/opclasscmds.c

+98
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ static void storeOperators(List *opfamilyname, Oid amoid, Oid opfamilyoid,
6565
List *operators, bool isAdd);
6666
static void storeProcedures(List *opfamilyname, Oid amoid, Oid opfamilyoid,
6767
List *procedures, bool isAdd);
68+
static bool typeDepNeeded(Oid typid, OpFamilyMember *member);
6869
static void dropOperators(List *opfamilyname, Oid amoid, Oid opfamilyoid,
6970
List *operators);
7071
static void dropProcedures(List *opfamilyname, Oid amoid, Oid opfamilyoid,
@@ -1507,6 +1508,29 @@ storeOperators(List *opfamilyname, Oid amoid, Oid opfamilyoid,
15071508
recordDependencyOn(&myself, &referenced,
15081509
op->ref_is_hard ? DEPENDENCY_INTERNAL : DEPENDENCY_AUTO);
15091510

1511+
if (typeDepNeeded(op->lefttype, op))
1512+
{
1513+
referenced.classId = TypeRelationId;
1514+
referenced.objectId = op->lefttype;
1515+
referenced.objectSubId = 0;
1516+
1517+
/* see comments in amapi.h about dependency strength */
1518+
recordDependencyOn(&myself, &referenced,
1519+
op->ref_is_hard ? DEPENDENCY_NORMAL : DEPENDENCY_AUTO);
1520+
}
1521+
1522+
if (op->lefttype != op->righttype &&
1523+
typeDepNeeded(op->righttype, op))
1524+
{
1525+
referenced.classId = TypeRelationId;
1526+
referenced.objectId = op->righttype;
1527+
referenced.objectSubId = 0;
1528+
1529+
/* see comments in amapi.h about dependency strength */
1530+
recordDependencyOn(&myself, &referenced,
1531+
op->ref_is_hard ? DEPENDENCY_NORMAL : DEPENDENCY_AUTO);
1532+
}
1533+
15101534
/* A search operator also needs a dep on the referenced opfamily */
15111535
if (OidIsValid(op->sortfamily))
15121536
{
@@ -1608,6 +1632,29 @@ storeProcedures(List *opfamilyname, Oid amoid, Oid opfamilyoid,
16081632
recordDependencyOn(&myself, &referenced,
16091633
proc->ref_is_hard ? DEPENDENCY_INTERNAL : DEPENDENCY_AUTO);
16101634

1635+
if (typeDepNeeded(proc->lefttype, proc))
1636+
{
1637+
referenced.classId = TypeRelationId;
1638+
referenced.objectId = proc->lefttype;
1639+
referenced.objectSubId = 0;
1640+
1641+
/* see comments in amapi.h about dependency strength */
1642+
recordDependencyOn(&myself, &referenced,
1643+
proc->ref_is_hard ? DEPENDENCY_NORMAL : DEPENDENCY_AUTO);
1644+
}
1645+
1646+
if (proc->lefttype != proc->righttype &&
1647+
typeDepNeeded(proc->righttype, proc))
1648+
{
1649+
referenced.classId = TypeRelationId;
1650+
referenced.objectId = proc->righttype;
1651+
referenced.objectSubId = 0;
1652+
1653+
/* see comments in amapi.h about dependency strength */
1654+
recordDependencyOn(&myself, &referenced,
1655+
proc->ref_is_hard ? DEPENDENCY_NORMAL : DEPENDENCY_AUTO);
1656+
}
1657+
16111658
/* Post create hook of access method procedure */
16121659
InvokeObjectPostCreateHook(AccessMethodProcedureRelationId,
16131660
entryoid, 0);
@@ -1616,6 +1663,57 @@ storeProcedures(List *opfamilyname, Oid amoid, Oid opfamilyoid,
16161663
table_close(rel, RowExclusiveLock);
16171664
}
16181665

1666+
/*
1667+
* Detect whether a pg_amop or pg_amproc entry needs an explicit dependency
1668+
* on its lefttype or righttype.
1669+
*
1670+
* We make such a dependency unless the entry has an indirect dependency
1671+
* via its referenced operator or function. That's nearly always true
1672+
* for operators, but might well not be true for support functions.
1673+
*/
1674+
static bool
1675+
typeDepNeeded(Oid typid, OpFamilyMember *member)
1676+
{
1677+
bool result = true;
1678+
1679+
/*
1680+
* If the type is pinned, we don't need a dependency. This is a bit of a
1681+
* layering violation perhaps (recordDependencyOn would ignore the request
1682+
* anyway), but it's a cheap test and will frequently save a syscache
1683+
* lookup here.
1684+
*/
1685+
if (IsPinnedObject(TypeRelationId, typid))
1686+
return false;
1687+
1688+
/* Nope, so check the input types of the function or operator. */
1689+
if (member->is_func)
1690+
{
1691+
Oid *argtypes;
1692+
int nargs;
1693+
1694+
(void) get_func_signature(member->object, &argtypes, &nargs);
1695+
for (int i = 0; i < nargs; i++)
1696+
{
1697+
if (typid == argtypes[i])
1698+
{
1699+
result = false; /* match, no dependency needed */
1700+
break;
1701+
}
1702+
}
1703+
pfree(argtypes);
1704+
}
1705+
else
1706+
{
1707+
Oid lefttype,
1708+
righttype;
1709+
1710+
op_input_types(member->object, &lefttype, &righttype);
1711+
if (typid == lefttype || typid == righttype)
1712+
result = false; /* match, no dependency needed */
1713+
}
1714+
return result;
1715+
}
1716+
16191717

16201718
/*
16211719
* Remove operator entries from an opfamily.

‎src/include/access/amapi.h

+4
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,10 @@ typedef enum IndexAMProperty
7676
* opfamily. This allows ALTER OPERATOR FAMILY DROP, and causes that to
7777
* happen automatically if the operator or support func is dropped. This
7878
* is the right behavior for inessential ("loose") objects.
79+
*
80+
* We also make dependencies on lefttype/righttype, of the same strength as
81+
* the dependency on the operator or support func, unless these dependencies
82+
* are redundant with the dependency on the operator or support func.
7983
*/
8084
typedef struct OpFamilyMember
8185
{

0 commit comments

Comments
 (0)