Fix DROP OPERATOR to reset oprcom/oprnegate links to the dropped operator.

This avoids leaving dangling links in pg_operator; which while fairly
harmless are also unsightly.

While we're at it, simplify OperatorUpd, which went through
heap_modify_tuple for no very good reason considering it had already made
a tuple copy it could just scribble on.

Roma Sokolov, reviewed by Tomas Vondra, additional hacking by Robert Haas
and myself.

diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 3cd18995565e8c881aef4d65fc2541ef7257d14b..5b5cd3fc01bd77b0e933e758171bc0fed470793a 100644 (file)
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -54,8 +54,6 @@ static Oid OperatorShellMake(const char *operatorName,
                  Oid leftTypeId,
                  Oid rightTypeId);
 
-static void OperatorUpd(Oid baseId, Oid commId, Oid negId);
-
 static Oid get_other_operator(List *otherOp,
                   Oid otherLeftTypeId, Oid otherRightTypeId,
                   const char *operatorName, Oid operatorNamespace,
@@ -566,7 +564,7 @@ OperatorCreate(const char *operatorName,
        commutatorId = operatorObjectId;
 
    if (OidIsValid(commutatorId) || OidIsValid(negatorId))
-       OperatorUpd(operatorObjectId, commutatorId, negatorId);
+       OperatorUpd(operatorObjectId, commutatorId, negatorId, false);
 
    return address;
 }
@@ -639,125 +637,125 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
  * OperatorUpd
  *
  * For a given operator, look up its negator and commutator operators.
- * If they are defined, but their negator and commutator fields
- * (respectively) are empty, then use the new operator for neg or comm.
- * This solves a problem for users who need to insert two new operators
- * which are the negator or commutator of each other.
+ * When isDelete is false, update their negator and commutator fields to
+ * point back to the given operator; when isDelete is true, update those
+ * fields to no longer point back to the given operator.
+ *
+ * The !isDelete case solves a problem for users who need to insert two new
+ * operators that are the negator or commutator of each other, while the
+ * isDelete case is needed so as not to leave dangling OID links behind
+ * after dropping an operator.
  */
-static void
-OperatorUpd(Oid baseId, Oid commId, Oid negId)
+void
+OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete)
 {
-   int         i;
    Relation    pg_operator_desc;
    HeapTuple   tup;
-   bool        nulls[Natts_pg_operator];
-   bool        replaces[Natts_pg_operator];
-   Datum       values[Natts_pg_operator];
-
-   for (i = 0; i < Natts_pg_operator; ++i)
-   {
-       values[i] = (Datum) 0;
-       replaces[i] = false;
-       nulls[i] = false;
-   }
 
    /*
-    * check and update the commutator & negator, if necessary
-    *
-    * We need a CommandCounterIncrement here in case of a self-commutator
-    * operator: we'll need to update the tuple that we just inserted.
+    * If we're making an operator into its own commutator, then we need a
+    * command-counter increment here, since we've just inserted the tuple
+    * we're about to update.  But when we're dropping an operator, we can
+    * skip this because we're at the beginning of the command.
     */
-   CommandCounterIncrement();
+   if (!isDelete)
+       CommandCounterIncrement();
 
+   /* Open the relation. */
    pg_operator_desc = heap_open(OperatorRelationId, RowExclusiveLock);
 
-   tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(commId));
+   /* Get a writable copy of the commutator's tuple. */
+   if (OidIsValid(commId))
+       tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(commId));
+   else
+       tup = NULL;
 
-   /*
-    * if the commutator and negator are the same operator, do one update. XXX
-    * this is probably useless code --- I doubt it ever makes sense for
-    * commutator and negator to be the same thing...
-    */
-   if (commId == negId)
+   /* Update the commutator's tuple if need be. */
+   if (HeapTupleIsValid(tup))
    {
-       if (HeapTupleIsValid(tup))
+       Form_pg_operator t = (Form_pg_operator) GETSTRUCT(tup);
+       bool        update_commutator = false;
+
+       /*
+        * Out of due caution, we only change the commutator's oprcom field if
+        * it has the exact value we expected: InvalidOid when creating an
+        * operator, or baseId when dropping one.
+        */
+       if (isDelete && t->oprcom == baseId)
        {
-           Form_pg_operator t = (Form_pg_operator) GETSTRUCT(tup);
-
-           if (!OidIsValid(t->oprcom) || !OidIsValid(t->oprnegate))
-           {
-               if (!OidIsValid(t->oprnegate))
-               {
-                   values[Anum_pg_operator_oprnegate - 1] = ObjectIdGetDatum(baseId);
-                   replaces[Anum_pg_operator_oprnegate - 1] = true;
-               }
-
-               if (!OidIsValid(t->oprcom))
-               {
-                   values[Anum_pg_operator_oprcom - 1] = ObjectIdGetDatum(baseId);
-                   replaces[Anum_pg_operator_oprcom - 1] = true;
-               }
-
-               tup = heap_modify_tuple(tup,
-                                       RelationGetDescr(pg_operator_desc),
-                                       values,
-                                       nulls,
-                                       replaces);
-
-               simple_heap_update(pg_operator_desc, &tup->t_self, tup);
-
-               CatalogUpdateIndexes(pg_operator_desc, tup);
-           }
+           t->oprcom = InvalidOid;
+           update_commutator = true;
+       }
+       else if (!isDelete && !OidIsValid(t->oprcom))
+       {
+           t->oprcom = baseId;
+           update_commutator = true;
        }
 
-       heap_close(pg_operator_desc, RowExclusiveLock);
-
-       return;
-   }
-
-   /* if commutator and negator are different, do two updates */
-
-   if (HeapTupleIsValid(tup) &&
-       !(OidIsValid(((Form_pg_operator) GETSTRUCT(tup))->oprcom)))
-   {
-       values[Anum_pg_operator_oprcom - 1] = ObjectIdGetDatum(baseId);
-       replaces[Anum_pg_operator_oprcom - 1] = true;
-
-       tup = heap_modify_tuple(tup,
-                               RelationGetDescr(pg_operator_desc),
-                               values,
-                               nulls,
-                               replaces);
-
-       simple_heap_update(pg_operator_desc, &tup->t_self, tup);
-
-       CatalogUpdateIndexes(pg_operator_desc, tup);
-
-       values[Anum_pg_operator_oprcom - 1] = (Datum) NULL;
-       replaces[Anum_pg_operator_oprcom - 1] = false;
+       /* If any columns were found to need modification, update tuple. */
+       if (update_commutator)
+       {
+           simple_heap_update(pg_operator_desc, &tup->t_self, tup);
+           CatalogUpdateIndexes(pg_operator_desc, tup);
+
+           /*
+            * Do CCI to make the updated tuple visible.  We must do this in
+            * case the commutator is also the negator.  (Which would be a
+            * logic error on the operator definer's part, but that's not a
+            * good reason to fail here.)  We would need a CCI anyway in the
+            * deletion case for a self-commutator with no negator.
+            */
+           CommandCounterIncrement();
+       }
    }
 
-   /* check and update the negator, if necessary */
-
-   tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(negId));
+   /*
+    * Similarly find and update the negator, if any.
+    */
+   if (OidIsValid(negId))
+       tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(negId));
+   else
+       tup = NULL;
 
-   if (HeapTupleIsValid(tup) &&
-       !(OidIsValid(((Form_pg_operator) GETSTRUCT(tup))->oprnegate)))
+   if (HeapTupleIsValid(tup))
    {
-       values[Anum_pg_operator_oprnegate - 1] = ObjectIdGetDatum(baseId);
-       replaces[Anum_pg_operator_oprnegate - 1] = true;
+       Form_pg_operator t = (Form_pg_operator) GETSTRUCT(tup);
+       bool        update_negator = false;
 
-       tup = heap_modify_tuple(tup,
-                               RelationGetDescr(pg_operator_desc),
-                               values,
-                               nulls,
-                               replaces);
-
-       simple_heap_update(pg_operator_desc, &tup->t_self, tup);
+       /*
+        * Out of due caution, we only change the negator's oprnegate field if
+        * it has the exact value we expected: InvalidOid when creating an
+        * operator, or baseId when dropping one.
+        */
+       if (isDelete && t->oprnegate == baseId)
+       {
+           t->oprnegate = InvalidOid;
+           update_negator = true;
+       }
+       else if (!isDelete && !OidIsValid(t->oprnegate))
+       {
+           t->oprnegate = baseId;
+           update_negator = true;
+       }
 
-       CatalogUpdateIndexes(pg_operator_desc, tup);
+       /* If any columns were found to need modification, update tuple. */
+       if (update_negator)
+       {
+           simple_heap_update(pg_operator_desc, &tup->t_self, tup);
+           CatalogUpdateIndexes(pg_operator_desc, tup);
+
+           /*
+            * In the deletion case, do CCI to make the updated tuple visible.
+            * We must do this in case the operator is its own negator. (Which
+            * would be a logic error on the operator definer's part, but
+            * that's not a good reason to fail here.)
+            */
+           if (isDelete)
+               CommandCounterIncrement();
+       }
    }
 
+   /* Close relation and release catalog lock. */
    heap_close(pg_operator_desc, RowExclusiveLock);
 }
 

diff --git a/src/backend/commands/operatorcmds.c b/src/backend/commands/operatorcmds.c
index 664e5d7de74514837729610a9780eb72b7c9a4e1..d32ba2d61f626299bedb73bcb2fac62b3f3baf01 100644 (file)
--- a/src/backend/commands/operatorcmds.c
+++ b/src/backend/commands/operatorcmds.c
@@ -341,12 +341,32 @@ RemoveOperatorById(Oid operOid)
 {
    Relation    relation;
    HeapTuple   tup;
+   Form_pg_operator op;
 
    relation = heap_open(OperatorRelationId, RowExclusiveLock);
 
    tup = SearchSysCache1(OPEROID, ObjectIdGetDatum(operOid));
    if (!HeapTupleIsValid(tup)) /* should not happen */
        elog(ERROR, "cache lookup failed for operator %u", operOid);
+   op = (Form_pg_operator) GETSTRUCT(tup);
+
+   /*
+    * Reset links from commutator and negator, if any.  In case of a
+    * self-commutator or self-negator, this means we have to re-fetch the
+    * updated tuple.  (We could optimize away updates on the tuple we're
+    * about to drop, but it doesn't seem worth convoluting the logic for.)
+    */
+   if (OidIsValid(op->oprcom) || OidIsValid(op->oprnegate))
+   {
+       OperatorUpd(operOid, op->oprcom, op->oprnegate, true);
+       if (operOid == op->oprcom || operOid == op->oprnegate)
+       {
+           ReleaseSysCache(tup);
+           tup = SearchSysCache1(OPEROID, ObjectIdGetDatum(operOid));
+           if (!HeapTupleIsValid(tup)) /* should not happen */
+               elog(ERROR, "cache lookup failed for operator %u", operOid);
+       }
+   }
 
    simple_heap_delete(relation, &tup->t_self);
 

diff --git a/src/include/catalog/pg_operator_fn.h b/src/include/catalog/pg_operator_fn.h
index 5315e19feb3d5132b40b90a11ef2fb7158eb4590..58814e21111294f41759c0e8edf9a5118c001ead 100644 (file)
--- a/src/include/catalog/pg_operator_fn.h
+++ b/src/include/catalog/pg_operator_fn.h
@@ -31,4 +31,6 @@ extern ObjectAddress OperatorCreate(const char *operatorName,
 
 extern ObjectAddress makeOperatorDependencies(HeapTuple tuple, bool isUpdate);
 
+extern void OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete);
+
 #endif   /* PG_OPERATOR_FN_H */

diff --git a/src/test/regress/expected/drop_operator.out b/src/test/regress/expected/drop_operator.out
new file mode 100644 (file)
index 0000000..cc8f5e7
--- /dev/null
+++ b/src/test/regress/expected/drop_operator.out
@@ -0,0 +1,61 @@
+CREATE OPERATOR === (
+        PROCEDURE = int8eq,
+        LEFTARG = bigint,
+        RIGHTARG = bigint,
+        COMMUTATOR = ===
+);
+CREATE OPERATOR !== (
+        PROCEDURE = int8ne,
+        LEFTARG = bigint,
+        RIGHTARG = bigint,
+        NEGATOR = ===,
+        COMMUTATOR = !==
+);
+DROP OPERATOR !==(bigint, bigint);
+SELECT  ctid, oprcom
+FROM    pg_catalog.pg_operator fk
+WHERE   oprcom != 0 AND
+        NOT EXISTS(SELECT 1 FROM pg_catalog.pg_operator pk WHERE pk.oid = fk.oprcom);
+ ctid | oprcom 
+------+--------
+(0 rows)
+
+SELECT  ctid, oprnegate
+FROM    pg_catalog.pg_operator fk
+WHERE   oprnegate != 0 AND
+        NOT EXISTS(SELECT 1 FROM pg_catalog.pg_operator pk WHERE pk.oid = fk.oprnegate);
+ ctid | oprnegate 
+------+-----------
+(0 rows)
+
+DROP OPERATOR ===(bigint, bigint);
+CREATE OPERATOR <| (
+        PROCEDURE = int8lt,
+        LEFTARG = bigint,
+        RIGHTARG = bigint
+);
+CREATE OPERATOR |> (
+        PROCEDURE = int8gt,
+        LEFTARG = bigint,
+        RIGHTARG = bigint,
+        NEGATOR = <|,
+        COMMUTATOR = <|
+);
+DROP OPERATOR |>(bigint, bigint);
+SELECT  ctid, oprcom
+FROM    pg_catalog.pg_operator fk
+WHERE   oprcom != 0 AND
+        NOT EXISTS(SELECT 1 FROM pg_catalog.pg_operator pk WHERE pk.oid = fk.oprcom);
+ ctid | oprcom 
+------+--------
+(0 rows)
+
+SELECT  ctid, oprnegate
+FROM    pg_catalog.pg_operator fk
+WHERE   oprnegate != 0 AND
+        NOT EXISTS(SELECT 1 FROM pg_catalog.pg_operator pk WHERE pk.oid = fk.oprnegate);
+ ctid | oprnegate 
+------+-----------
+(0 rows)
+
+DROP OPERATOR <|(bigint, bigint);

diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 8be4b831a1efc8dd7cbf7f0edde19a333d7ee80b..7c7b58d43d9d109f5498348c9b154b1cf1edf90a 100644 (file)
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -84,7 +84,7 @@ test: select_into select_distinct select_distinct_on select_implicit select_havi
 # ----------
 # Another group of parallel tests
 # ----------
-test: brin gin gist spgist privileges security_label collate matview lock replica_identity rowsecurity object_address tablesample groupingsets
+test: brin gin gist spgist privileges security_label collate matview lock replica_identity rowsecurity object_address tablesample groupingsets drop_operator
 
 # ----------
 # Another group of parallel tests

diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
index 1de3da8bac0a985bcaddd354cf3dee487e2f5fbd..1b66516a13f1509f7ee2ce053c2d4a8c72abb6e2 100644 (file)
--- a/src/test/regress/serial_schedule
+++ b/src/test/regress/serial_schedule
@@ -89,7 +89,6 @@ test: union
 test: case
 test: join
 test: aggregates
-test: groupingsets
 test: transactions
 ignore: random
 test: random
@@ -114,6 +113,8 @@ test: replica_identity
 test: rowsecurity
 test: object_address
 test: tablesample
+test: groupingsets
+test: drop_operator
 test: alter_generic
 test: alter_operator
 test: misc

diff --git a/src/test/regress/sql/drop_operator.sql b/src/test/regress/sql/drop_operator.sql
new file mode 100644 (file)
index 0000000..cc62cfa
--- /dev/null
+++ b/src/test/regress/sql/drop_operator.sql
@@ -0,0 +1,56 @@
+CREATE OPERATOR === (
+        PROCEDURE = int8eq,
+        LEFTARG = bigint,
+        RIGHTARG = bigint,
+        COMMUTATOR = ===
+);
+
+CREATE OPERATOR !== (
+        PROCEDURE = int8ne,
+        LEFTARG = bigint,
+        RIGHTARG = bigint,
+        NEGATOR = ===,
+        COMMUTATOR = !==
+);
+
+DROP OPERATOR !==(bigint, bigint);
+
+SELECT  ctid, oprcom
+FROM    pg_catalog.pg_operator fk
+WHERE   oprcom != 0 AND
+        NOT EXISTS(SELECT 1 FROM pg_catalog.pg_operator pk WHERE pk.oid = fk.oprcom);
+
+SELECT  ctid, oprnegate
+FROM    pg_catalog.pg_operator fk
+WHERE   oprnegate != 0 AND
+        NOT EXISTS(SELECT 1 FROM pg_catalog.pg_operator pk WHERE pk.oid = fk.oprnegate);
+
+DROP OPERATOR ===(bigint, bigint);
+
+CREATE OPERATOR <| (
+        PROCEDURE = int8lt,
+        LEFTARG = bigint,
+        RIGHTARG = bigint
+);
+
+CREATE OPERATOR |> (
+        PROCEDURE = int8gt,
+        LEFTARG = bigint,
+        RIGHTARG = bigint,
+        NEGATOR = <|,
+        COMMUTATOR = <|
+);
+
+DROP OPERATOR |>(bigint, bigint);
+
+SELECT  ctid, oprcom
+FROM    pg_catalog.pg_operator fk
+WHERE   oprcom != 0 AND
+        NOT EXISTS(SELECT 1 FROM pg_catalog.pg_operator pk WHERE pk.oid = fk.oprcom);
+
+SELECT  ctid, oprnegate
+FROM    pg_catalog.pg_operator fk
+WHERE   oprnegate != 0 AND
+        NOT EXISTS(SELECT 1 FROM pg_catalog.pg_operator pk WHERE pk.oid = fk.oprnegate);
+
+DROP OPERATOR <|(bigint, bigint);