diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c index 3cd1899..e774571 100644 --- 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; } @@ -643,9 +641,14 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId, * (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. + * + * If we are removing given operator, we'd like to reset links on commutator + * and negator, but only if they are point to given operator (i.e. haven't + * been somehow updated. This check is mostly useless, since we forbid + * overwriting oprcom and oprnegate). */ -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; @@ -653,6 +656,9 @@ OperatorUpd(Oid baseId, Oid commId, Oid negId) bool nulls[Natts_pg_operator]; bool replaces[Natts_pg_operator]; Datum values[Natts_pg_operator]; + Datum target; + bool updateTuple; + for (i = 0; i < Natts_pg_operator; ++i) { @@ -660,104 +666,108 @@ OperatorUpd(Oid baseId, Oid commId, Oid negId) replaces[i] = false; nulls[i] = false; } - + updateTuple = 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 deleting operator, we will not do any updates for + * self-commutator: we will just delete it. */ - CommandCounterIncrement(); + if (!isDelete) + CommandCounterIncrement(); pg_operator_desc = heap_open(OperatorRelationId, RowExclusiveLock); + /* + * When deleting, reset other operator field to InvalidOid, otherwise, + * set it to point to operator being updated. + */ + target = (isDelete ? InvalidOid : ObjectIdGetDatum(baseId)); + tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(commId)); /* - * 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 the commutator and negator are the same operator, as for example, + * comparison operators, like < and >, we will do one update. Otherwise + * we will update other operators separately, with two modifications. */ - if (commId == negId) + + if (HeapTupleIsValid(tup)) { - if (HeapTupleIsValid(tup)) + Form_pg_operator t = (Form_pg_operator) GETSTRUCT(tup); + /* + * When deleting, check, whether other op field points to operator + * being updated. Otherwise, check, whether field is not set. + */ + if (isDelete ? t->oprcom == baseId : !OidIsValid(t->oprcom)) { - 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); - } + values[Anum_pg_operator_oprcom - 1] = target; + replaces[Anum_pg_operator_oprcom - 1] = true; + updateTuple = 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))) + /* + * If the commutator and negator are different, check whether we need + * update tuple and retrieve tuple for negator op. Otherwise, continue + * with the same tuple for negator. + */ + if (commId != negId) { - 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); + /* + * If we should update tuple, issue update and reset values and replaces + * for possible subsequent update. + */ + if (updateTuple) + { + tup = heap_modify_tuple(tup, + RelationGetDescr(pg_operator_desc), + values, + nulls, + replaces); - simple_heap_update(pg_operator_desc, &tup->t_self, tup); + simple_heap_update(pg_operator_desc, &tup->t_self, tup); - CatalogUpdateIndexes(pg_operator_desc, tup); + CatalogUpdateIndexes(pg_operator_desc, tup); - values[Anum_pg_operator_oprcom - 1] = (Datum) NULL; - replaces[Anum_pg_operator_oprcom - 1] = false; + values[Anum_pg_operator_oprcom - 1] = (Datum) NULL; + replaces[Anum_pg_operator_oprcom - 1] = false; + updateTuple = false; + } + tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(negId)); } - /* check and update the negator, if necessary */ - - tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(negId)); - - if (HeapTupleIsValid(tup) && - !(OidIsValid(((Form_pg_operator) GETSTRUCT(tup))->oprnegate))) + /* check, whether we should update the negator */ + 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); + if (isDelete ? t->oprnegate == baseId : !OidIsValid(t->oprnegate)) + { + values[Anum_pg_operator_oprnegate - 1] = target; + replaces[Anum_pg_operator_oprnegate - 1] = true; + updateTuple = true; + + } + } + /* + * Here we either should update tuple for commutator and negator, + * or just tuple for negator. + */ + if (updateTuple) + { 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); } - + heap_close(pg_operator_desc, RowExclusiveLock); } diff --git a/src/backend/commands/operatorcmds.c b/src/backend/commands/operatorcmds.c index 664e5d7..af95b9a 100644 --- a/src/backend/commands/operatorcmds.c +++ b/src/backend/commands/operatorcmds.c @@ -339,15 +339,32 @@ ValidateJoinEstimator(List *joinName) void RemoveOperatorById(Oid operOid) { - Relation relation; - HeapTuple tup; + Relation relation; + HeapTuple tup; + Form_pg_operator op; relation = heap_open(OperatorRelationId, RowExclusiveLock); tup = SearchSysCache1(OPEROID, ObjectIdGetDatum(operOid)); + op = (Form_pg_operator) GETSTRUCT(tup); if (!HeapTupleIsValid(tup)) /* should not happen */ elog(ERROR, "cache lookup failed for operator %u", operOid); + /* + * Reset links on commutator and negator. Only do that if either + * oprcom or oprnegate is set and given operator is not self-commutator. + * For self-commutator with negator, prevent meaningful updates of the + * same tuple by sending InvalidOid. + * Since operator can't be its own negator, we don't need this check for + * negator: if there is oprnegate, it should be updated. + */ + if (OidIsValid(op->oprnegate) || + (OidIsValid(op->oprcom) && operOid != op->oprcom)) + OperatorUpd(operOid, + operOid == op->oprcom ? InvalidOid : op->oprcom, + op->oprnegate, + true); + simple_heap_delete(relation, &tup->t_self); ReleaseSysCache(tup); diff --git a/src/include/catalog/pg_operator_fn.h b/src/include/catalog/pg_operator_fn.h index 5315e19..3e1c7ae 100644 --- a/src/include/catalog/pg_operator_fn.h +++ b/src/include/catalog/pg_operator_fn.h @@ -30,5 +30,5 @@ extern ObjectAddress OperatorCreate(const char *operatorName, bool canHash); 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 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 bec0316..731677f 100644 --- a/src/test/regress/parallel_schedule +++ b/src/test/regress/parallel_schedule @@ -79,7 +79,7 @@ ignore: random # ---------- # Another group of parallel tests # ---------- -test: select_into select_distinct select_distinct_on select_implicit select_having subselect union case join aggregates transactions random portals arrays btree_index hash_index update namespace prepared_xacts delete +test: select_into select_distinct select_distinct_on select_implicit select_having subselect union case join aggregates transactions random portals arrays btree_index hash_index update namespace prepared_xacts delete drop_operator # ---------- # Another group of parallel tests diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule index 7e9b319..4e2fe83 100644 --- a/src/test/regress/serial_schedule +++ b/src/test/regress/serial_schedule @@ -115,6 +115,7 @@ test: object_address test: tablesample test: alter_generic test: alter_operator +test: drop_operator test: misc test: psql test: async diff --git a/src/test/regress/sql/drop_operator.sql b/src/test/regress/sql/drop_operator.sql new file mode 100644 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);