From 2b5154beab794eae6e624c162d497df927ec9d27 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 20 Oct 2023 12:28:38 -0400 Subject: [PATCH] Extend ALTER OPERATOR to allow setting more optimization attributes. Allow the COMMUTATOR, NEGATOR, MERGES, and HASHES attributes to be set by ALTER OPERATOR. However, we don't allow COMMUTATOR/NEGATOR to be changed once set, nor allow the MERGES/HASHES flags to be unset once set. Changes like that might invalidate plans already made, and dealing with the consequences seems like more trouble than it's worth. The main use-case we foresee for this is to allow addition of missed properties in extension update scripts, such as extending an existing operator to support hashing. So only transitions from not-set to set states seem very useful. This patch also causes us to reject some incorrect cases that formerly resulted in inconsistent catalog state, such as trying to set the commutator of an operator to be some other operator that already has a (different) commutator. While at it, move the InvokeObjectPostCreateHook call for CREATE OPERATOR to not occur until after we've fixed up commutator or negator links as needed. The previous ordering could only be justified by thinking of the OperatorUpd call as a kind of ALTER OPERATOR step; but we don't call InvokeObjectPostAlterHook therein. It seems better to let the hook see the final state of the operator object. In the documentation, move the discussion of how to establish commutator pairs from xoper.sgml to the CREATE OPERATOR ref page. Tommy Pavlicek, reviewed and editorialized a bit by me Discussion: https://postgr.es/m/CAEhP-W-vGVzf4udhR5M8Bdv88UYnPrhoSkj3ieR3QNrsGQoqdg@mail.gmail.com --- doc/src/sgml/ref/alter_operator.sgml | 86 +++++- doc/src/sgml/ref/create_operator.sgml | 66 ++++- doc/src/sgml/xoper.sgml | 44 --- src/backend/catalog/pg_operator.c | 254 ++++++++++++------ src/backend/commands/operatorcmds.c | 198 ++++++++++++-- src/backend/parser/gram.y | 2 + src/backend/parser/parse_oper.c | 38 +-- src/include/catalog/pg_operator.h | 15 ++ src/include/parser/parse_oper.h | 3 + src/test/regress/expected/alter_operator.out | 148 +++++++++- src/test/regress/expected/create_operator.out | 44 +++ src/test/regress/sql/alter_operator.sql | 137 +++++++++- src/test/regress/sql/create_operator.sql | 43 +++ 13 files changed, 871 insertions(+), 207 deletions(-) diff --git a/doc/src/sgml/ref/alter_operator.sgml b/doc/src/sgml/ref/alter_operator.sgml index a4a1af564f..673dcce2f5 100644 --- a/doc/src/sgml/ref/alter_operator.sgml +++ b/doc/src/sgml/ref/alter_operator.sgml @@ -30,7 +30,11 @@ ALTER OPERATOR name ( { left_typename ( { left_type | NONE } , right_type ) SET ( { RESTRICT = { res_proc | NONE } | JOIN = { join_proc | NONE } - } [, ... ] ) + | COMMUTATOR = com_op + | NEGATOR = neg_op + | HASHES + | MERGES + } [, ... ] ) @@ -121,9 +125,69 @@ ALTER OPERATOR name ( { left_type + + com_op + + + The commutator of this operator. Can only be changed if the operator + does not have an existing commutator. + + + + + + neg_op + + + The negator of this operator. Can only be changed if the operator does + not have an existing negator. + + + + + + HASHES + + + Indicates this operator can support a hash join. Can only be enabled and + not disabled. + + + + + + MERGES + + + Indicates this operator can support a merge join. Can only be enabled + and not disabled. + + + + + + Notes + + + Refer to and + for further information. + + + + Since commutators come in pairs that are commutators of each other, + ALTER OPERATOR SET COMMUTATOR will also set the + commutator of the com_op + to be the target operator. Likewise, ALTER OPERATOR SET + NEGATOR will also set the negator of + the neg_op to be the + target operator. Therefore, you must own the commutator or negator + operator as well as the target operator. + + + Examples @@ -131,13 +195,25 @@ ALTER OPERATOR name ( { left_typea @@ b for type text: ALTER OPERATOR @@ (text, text) OWNER TO joe; - + + - Change the restriction and join selectivity estimator functions of a custom operator a && b for type int[]: + Change the restriction and join selectivity estimator functions of a + custom operator a && b for + type int[]: -ALTER OPERATOR && (_int4, _int4) SET (RESTRICT = _int_contsel, JOIN = _int_contjoinsel); - +ALTER OPERATOR && (int[], int[]) SET (RESTRICT = _int_contsel, JOIN = _int_contjoinsel); + + + + + Mark the && operator as being its own + commutator: + +ALTER OPERATOR && (int[], int[]) SET (COMMUTATOR = &&); + + diff --git a/doc/src/sgml/ref/create_operator.sgml b/doc/src/sgml/ref/create_operator.sgml index e27512ff39..c421fd21e9 100644 --- a/doc/src/sgml/ref/create_operator.sgml +++ b/doc/src/sgml/ref/create_operator.sgml @@ -104,7 +104,7 @@ CREATE OPERATOR name ( - The other clauses specify optional operator optimization clauses. + The other clauses specify optional operator optimization attributes. Their meaning is detailed in . @@ -112,7 +112,7 @@ CREATE OPERATOR name ( To be able to create an operator, you must have USAGE privilege on the argument types and the return type, as well as EXECUTE privilege on the underlying function. If a - commutator or negator operator is specified, you must own these operators. + commutator or negator operator is specified, you must own those operators. @@ -231,7 +231,67 @@ COMMUTATOR = OPERATOR(myschema.===) , Notes - Refer to for further information. + Refer to and + for further information. + + + + When you are defining a self-commutative operator, you just do it. + When you are defining a pair of commutative operators, things are + a little trickier: how can the first one to be defined refer to the + other one, which you haven't defined yet? There are three solutions + to this problem: + + + + + One way is to omit the COMMUTATOR clause in the + first operator that you define, and then provide one in the second + operator's definition. Since PostgreSQL + knows that commutative operators come in pairs, when it sees the + second definition it will automatically go back and fill in the + missing COMMUTATOR clause in the first + definition. + + + + + + Another, more straightforward way is just to + include COMMUTATOR clauses in both definitions. + When PostgreSQL processes the first + definition and realizes that COMMUTATOR refers to + a nonexistent operator, the system will make a dummy entry for that + operator in the system catalog. This dummy entry will have valid + data only for the operator name, left and right operand types, and + owner, since that's all that PostgreSQL + can deduce at this point. The first operator's catalog entry will + link to this dummy entry. Later, when you define the second + operator, the system updates the dummy entry with the additional + information from the second definition. If you try to use the dummy + operator before it's been filled in, you'll just get an error + message. + + + + + + Alternatively, both operators can be defined + without COMMUTATOR clauses + and then ALTER OPERATOR can be used to set their + commutator links. It's sufficient to ALTER + either one of the pair. + + + + + In all three cases, you must own both operators in order to mark + them as commutators. + + + + Pairs of negator operators can be defined using the same methods + as for commutator pairs. diff --git a/doc/src/sgml/xoper.sgml b/doc/src/sgml/xoper.sgml index a929ced07d..954a90d77d 100644 --- a/doc/src/sgml/xoper.sgml +++ b/doc/src/sgml/xoper.sgml @@ -146,44 +146,6 @@ SELECT (a + b) AS c FROM test_complex; = operator must specify that it is valid, by marking the operator with commutator information. - - - When you are defining a self-commutative operator, you just do it. - When you are defining a pair of commutative operators, things are - a little trickier: how can the first one to be defined refer to the - other one, which you haven't defined yet? There are two solutions - to this problem: - - - - - One way is to omit the COMMUTATOR clause in the first operator that - you define, and then provide one in the second operator's definition. - Since PostgreSQL knows that commutative - operators come in pairs, when it sees the second definition it will - automatically go back and fill in the missing COMMUTATOR clause in - the first definition. - - - - - - The other, more straightforward way is just to include COMMUTATOR clauses - in both definitions. When PostgreSQL processes - the first definition and realizes that COMMUTATOR refers to a nonexistent - operator, the system will make a dummy entry for that operator in the - system catalog. This dummy entry will have valid data only - for the operator name, left and right operand types, and result type, - since that's all that PostgreSQL can deduce - at this point. The first operator's catalog entry will link to this - dummy entry. Later, when you define the second operator, the system - updates the dummy entry with the additional information from the second - definition. If you try to use the dummy operator before it's been filled - in, you'll just get an error message. - - - - @@ -217,12 +179,6 @@ SELECT (a + b) AS c FROM test_complex; x <> y. This comes up more often than you might think, because NOT operations can be inserted as a consequence of other rearrangements. - - - Pairs of negator operators can be defined using the same methods - explained above for commutator pairs. - - diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c index 95918a77a1..4d1b9758fa 100644 --- a/src/backend/catalog/pg_operator.c +++ b/src/backend/catalog/pg_operator.c @@ -44,11 +44,6 @@ static Oid OperatorGet(const char *operatorName, Oid rightObjectId, bool *defined); -static Oid OperatorLookup(List *operatorName, - Oid leftObjectId, - Oid rightObjectId, - bool *defined); - static Oid OperatorShellMake(const char *operatorName, Oid operatorNamespace, Oid leftTypeId, @@ -57,8 +52,7 @@ static Oid OperatorShellMake(const char *operatorName, static Oid get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId, const char *operatorName, Oid operatorNamespace, - Oid leftTypeId, Oid rightTypeId, - bool isCommutator); + Oid leftTypeId, Oid rightTypeId); /* @@ -166,7 +160,7 @@ OperatorGet(const char *operatorName, * * *defined is set true if defined (not a shell) */ -static Oid +Oid OperatorLookup(List *operatorName, Oid leftObjectId, Oid rightObjectId, @@ -361,53 +355,17 @@ OperatorCreate(const char *operatorName, errmsg("\"%s\" is not a valid operator name", operatorName))); - if (!(OidIsValid(leftTypeId) && OidIsValid(rightTypeId))) - { - /* If it's not a binary op, these things mustn't be set: */ - if (commutatorName) - ereport(ERROR, - (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("only binary operators can have commutators"))); - if (OidIsValid(joinId)) - ereport(ERROR, - (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("only binary operators can have join selectivity"))); - if (canMerge) - ereport(ERROR, - (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("only binary operators can merge join"))); - if (canHash) - ereport(ERROR, - (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("only binary operators can hash"))); - } - operResultType = get_func_rettype(procedureId); - if (operResultType != BOOLOID) - { - /* If it's not a boolean op, these things mustn't be set: */ - if (negatorName) - ereport(ERROR, - (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("only boolean operators can have negators"))); - if (OidIsValid(restrictionId)) - ereport(ERROR, - (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("only boolean operators can have restriction selectivity"))); - if (OidIsValid(joinId)) - ereport(ERROR, - (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("only boolean operators can have join selectivity"))); - if (canMerge) - ereport(ERROR, - (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("only boolean operators can merge join"))); - if (canHash) - ereport(ERROR, - (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("only boolean operators can hash"))); - } + OperatorValidateParams(leftTypeId, + rightTypeId, + operResultType, + commutatorName != NIL, + negatorName != NIL, + OidIsValid(restrictionId), + OidIsValid(joinId), + canMerge, + canHash); operatorObjectId = OperatorGet(operatorName, operatorNamespace, @@ -442,8 +400,7 @@ OperatorCreate(const char *operatorName, commutatorId = get_other_operator(commutatorName, rightTypeId, leftTypeId, operatorName, operatorNamespace, - leftTypeId, rightTypeId, - true); + leftTypeId, rightTypeId); /* Permission check: must own other operator */ if (OidIsValid(commutatorId) && @@ -452,8 +409,9 @@ OperatorCreate(const char *operatorName, NameListToString(commutatorName)); /* - * self-linkage to this operator; will fix below. Note that only - * self-linkage for commutation makes sense. + * If self-linkage to the new operator is requested, we'll fix it + * below. (In case of self-linkage to an existing shell operator, we + * need do nothing special.) */ if (!OidIsValid(commutatorId)) selfCommutator = true; @@ -467,14 +425,24 @@ OperatorCreate(const char *operatorName, negatorId = get_other_operator(negatorName, leftTypeId, rightTypeId, operatorName, operatorNamespace, - leftTypeId, rightTypeId, - false); + leftTypeId, rightTypeId); /* Permission check: must own other operator */ if (OidIsValid(negatorId) && !object_ownercheck(OperatorRelationId, negatorId, GetUserId())) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_OPERATOR, NameListToString(negatorName)); + + /* + * Prevent self negation, as it doesn't make sense. It's self + * negation if result is InvalidOid (negator would be the same + * operator but it doesn't exist yet) or operatorObjectId (we are + * replacing a shell that would need to be its own negator). + */ + if (!OidIsValid(negatorId) || negatorId == operatorObjectId) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("operator cannot be its own negator"))); } else negatorId = InvalidOid; @@ -548,11 +516,6 @@ OperatorCreate(const char *operatorName, /* Add dependencies for the entry */ address = makeOperatorDependencies(tup, true, isUpdate); - /* Post creation hook for new operator */ - InvokeObjectPostCreateHook(OperatorRelationId, operatorObjectId, 0); - - table_close(pg_operator_desc, RowExclusiveLock); - /* * If a commutator and/or negator link is provided, update the other * operator(s) to point at this one, if they don't already have a link. @@ -570,21 +533,95 @@ OperatorCreate(const char *operatorName, if (OidIsValid(commutatorId) || OidIsValid(negatorId)) OperatorUpd(operatorObjectId, commutatorId, negatorId, false); + /* Post creation hook for new operator */ + InvokeObjectPostCreateHook(OperatorRelationId, operatorObjectId, 0); + + table_close(pg_operator_desc, RowExclusiveLock); + return address; } /* - * Try to lookup another operator (commutator, etc) + * OperatorValidateParams * - * If not found, check to see if it is exactly the operator we are trying - * to define; if so, return InvalidOid. (Note that this case is only - * sensible for a commutator, so we error out otherwise.) If it is not - * the same operator, create a shell operator. + * Check that an operator with argument types leftTypeId and rightTypeId, + * returning operResultType, can have the attributes that are set to true. + * Raise an error for any disallowed attribute. + * + * Note: in ALTER OPERATOR, we only bother to pass "true" for attributes + * the command is trying to set, not those that may already be set. + * This is OK as long as the attribute checks are independent. + */ +void +OperatorValidateParams(Oid leftTypeId, + Oid rightTypeId, + Oid operResultType, + bool hasCommutator, + bool hasNegator, + bool hasRestrictionSelectivity, + bool hasJoinSelectivity, + bool canMerge, + bool canHash) +{ + if (!(OidIsValid(leftTypeId) && OidIsValid(rightTypeId))) + { + /* If it's not a binary op, these things mustn't be set: */ + if (hasCommutator) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("only binary operators can have commutators"))); + if (hasJoinSelectivity) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("only binary operators can have join selectivity"))); + if (canMerge) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("only binary operators can merge join"))); + if (canHash) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("only binary operators can hash"))); + } + + if (operResultType != BOOLOID) + { + /* If it's not a boolean op, these things mustn't be set: */ + if (hasNegator) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("only boolean operators can have negators"))); + if (hasRestrictionSelectivity) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("only boolean operators can have restriction selectivity"))); + if (hasJoinSelectivity) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("only boolean operators can have join selectivity"))); + if (canMerge) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("only boolean operators can merge join"))); + if (canHash) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("only boolean operators can hash"))); + } +} + +/* + * Try to lookup another operator (commutator, etc); return its OID + * + * If not found, check to see if it would be the same operator we are trying + * to define; if so, return InvalidOid. (Caller must decide whether + * that is sensible.) If it is not the same operator, create a shell + * operator. */ static Oid get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId, const char *operatorName, Oid operatorNamespace, - Oid leftTypeId, Oid rightTypeId, bool isCommutator) + Oid leftTypeId, Oid rightTypeId) { Oid other_oid; bool otherDefined; @@ -611,14 +648,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId, otherLeftTypeId == leftTypeId && otherRightTypeId == rightTypeId) { - /* - * self-linkage to this operator; caller will fix later. Note that - * only self-linkage for commutation makes sense. - */ - if (!isCommutator) - ereport(ERROR, - (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("operator cannot be its own negator or sort operator"))); + /* self-linkage to new operator; caller must handle this */ return InvalidOid; } @@ -643,7 +673,7 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId, * For a given operator, look up its negator and commutator operators. * 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. + * fields to be InvalidOid. * * 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 @@ -681,17 +711,40 @@ OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete) 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. + * We can skip doing anything if the commutator's oprcom field is + * already what we want. While that's not expected in the isDelete + * case, it's perfectly possible when filling in a shell operator. */ - if (isDelete && t->oprcom == baseId) + if (isDelete && OidIsValid(t->oprcom)) { t->oprcom = InvalidOid; update_commutator = true; } - else if (!isDelete && !OidIsValid(t->oprcom)) + else if (!isDelete && t->oprcom != baseId) { + /* + * If commutator's oprcom field is already set to point to some + * third operator, it's an error. Changing its link would be + * unsafe, and letting the inconsistency stand would not be good + * either. This might be indicative of catalog corruption, so + * don't assume t->oprcom is necessarily a valid operator. + */ + if (OidIsValid(t->oprcom)) + { + char *thirdop = get_opname(t->oprcom); + + if (thirdop != NULL) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("commutator operator %s is already the commutator of operator %s", + NameStr(t->oprname), thirdop))); + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("commutator operator %s is already the commutator of operator %u", + NameStr(t->oprname), t->oprcom))); + } + t->oprcom = baseId; update_commutator = true; } @@ -726,17 +779,40 @@ OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete) bool update_negator = false; /* - * 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. + * We can skip doing anything if the negator's oprnegate field is + * already what we want. While that's not expected in the isDelete + * case, it's perfectly possible when filling in a shell operator. */ - if (isDelete && t->oprnegate == baseId) + if (isDelete && OidIsValid(t->oprnegate)) { t->oprnegate = InvalidOid; update_negator = true; } - else if (!isDelete && !OidIsValid(t->oprnegate)) + else if (!isDelete && t->oprnegate != baseId) { + /* + * If negator's oprnegate field is already set to point to some + * third operator, it's an error. Changing its link would be + * unsafe, and letting the inconsistency stand would not be good + * either. This might be indicative of catalog corruption, so + * don't assume t->oprnegate is necessarily a valid operator. + */ + if (OidIsValid(t->oprnegate)) + { + char *thirdop = get_opname(t->oprnegate); + + if (thirdop != NULL) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("negator operator %s is already the negator of operator %s", + NameStr(t->oprname), thirdop))); + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("negator operator %s is already the negator of operator %u", + NameStr(t->oprname), t->oprnegate))); + } + t->oprnegate = baseId; update_negator = true; } diff --git a/src/backend/commands/operatorcmds.c b/src/backend/commands/operatorcmds.c index cd7f83136f..df84b839f5 100644 --- a/src/backend/commands/operatorcmds.c +++ b/src/backend/commands/operatorcmds.c @@ -54,6 +54,9 @@ static Oid ValidateRestrictionEstimator(List *restrictionName); static Oid ValidateJoinEstimator(List *joinName); +static Oid ValidateOperatorReference(List *name, + Oid leftTypeId, + Oid rightTypeId); /* * DefineOperator @@ -359,6 +362,53 @@ ValidateJoinEstimator(List *joinName) return joinOid; } +/* + * Look up and return the OID of an operator, + * given a possibly-qualified name and left and right type IDs. + * + * Verifies that the operator is defined (not a shell) and owned by + * the current user, so that we have permission to associate it with + * the operator being altered. Rejecting shell operators is a policy + * choice to help catch mistakes, rather than something essential. + */ +static Oid +ValidateOperatorReference(List *name, + Oid leftTypeId, + Oid rightTypeId) +{ + Oid oid; + bool defined; + + oid = OperatorLookup(name, + leftTypeId, + rightTypeId, + &defined); + + /* These message strings are chosen to match parse_oper.c */ + if (!OidIsValid(oid)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_FUNCTION), + errmsg("operator does not exist: %s", + op_signature_string(name, + leftTypeId, + rightTypeId)))); + + if (!defined) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_FUNCTION), + errmsg("operator is only a shell: %s", + op_signature_string(name, + leftTypeId, + rightTypeId)))); + + if (!object_ownercheck(OperatorRelationId, oid, GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_OPERATOR, + NameListToString(name)); + + return oid; +} + + /* * Guts of operator deletion. */ @@ -406,6 +456,10 @@ RemoveOperatorById(Oid operOid) * routine implementing ALTER OPERATOR SET (option = ...). * * Currently, only RESTRICT and JOIN estimator functions can be changed. + * COMMUTATOR, NEGATOR, MERGES, and HASHES attributes can be set if they + * have not been set previously. (Changing or removing one of these + * attributes could invalidate existing plans, which seems more trouble + * than it's worth.) */ ObjectAddress AlterOperator(AlterOperatorStmt *stmt) @@ -426,6 +480,14 @@ AlterOperator(AlterOperatorStmt *stmt) List *joinName = NIL; /* optional join sel. function */ bool updateJoin = false; Oid joinOid; + List *commutatorName = NIL; /* optional commutator operator name */ + Oid commutatorOid; + List *negatorName = NIL; /* optional negator operator name */ + Oid negatorOid; + bool canMerge = false; + bool updateMerges = false; + bool canHash = false; + bool updateHashes = false; /* Look up the operator */ oprId = LookupOperWithArgs(stmt->opername, false); @@ -456,6 +518,24 @@ AlterOperator(AlterOperatorStmt *stmt) joinName = param; updateJoin = true; } + else if (strcmp(defel->defname, "commutator") == 0) + { + commutatorName = defGetQualifiedName(defel); + } + else if (strcmp(defel->defname, "negator") == 0) + { + negatorName = defGetQualifiedName(defel); + } + else if (strcmp(defel->defname, "merges") == 0) + { + canMerge = defGetBoolean(defel); + updateMerges = true; + } + else if (strcmp(defel->defname, "hashes") == 0) + { + canHash = defGetBoolean(defel); + updateHashes = true; + } /* * The rest of the options that CREATE accepts cannot be changed. @@ -464,11 +544,7 @@ AlterOperator(AlterOperatorStmt *stmt) else if (strcmp(defel->defname, "leftarg") == 0 || strcmp(defel->defname, "rightarg") == 0 || strcmp(defel->defname, "function") == 0 || - strcmp(defel->defname, "procedure") == 0 || - strcmp(defel->defname, "commutator") == 0 || - strcmp(defel->defname, "negator") == 0 || - strcmp(defel->defname, "hashes") == 0 || - strcmp(defel->defname, "merges") == 0) + strcmp(defel->defname, "procedure") == 0) { ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -488,7 +564,7 @@ AlterOperator(AlterOperatorStmt *stmt) NameStr(oprForm->oprname)); /* - * Look up restriction and join estimators if specified + * Look up OIDs for any parameters specified */ if (restrictionName) restrictionOid = ValidateRestrictionEstimator(restrictionName); @@ -499,27 +575,78 @@ AlterOperator(AlterOperatorStmt *stmt) else joinOid = InvalidOid; - /* Perform additional checks, like OperatorCreate does */ - if (!(OidIsValid(oprForm->oprleft) && OidIsValid(oprForm->oprright))) + if (commutatorName) { - /* If it's not a binary op, these things mustn't be set: */ - if (OidIsValid(joinOid)) + /* commutator has reversed arg types */ + commutatorOid = ValidateOperatorReference(commutatorName, + oprForm->oprright, + oprForm->oprleft); + + /* + * We don't need to do anything extra for a self commutator as in + * OperatorCreate, since the operator surely exists already. + */ + } + else + commutatorOid = InvalidOid; + + if (negatorName) + { + negatorOid = ValidateOperatorReference(negatorName, + oprForm->oprleft, + oprForm->oprright); + + /* Must reject self-negation */ + if (negatorOid == oprForm->oid) ereport(ERROR, (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("only binary operators can have join selectivity"))); + errmsg("operator cannot be its own negator"))); + } + else + { + negatorOid = InvalidOid; } - if (oprForm->oprresult != BOOLOID) - { - if (OidIsValid(restrictionOid)) - ereport(ERROR, - (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("only boolean operators can have restriction selectivity"))); - if (OidIsValid(joinOid)) - ereport(ERROR, - (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("only boolean operators can have join selectivity"))); - } + /* + * Check that we're not changing any attributes that might be depended on + * by plans, while allowing no-op updates. + */ + if (OidIsValid(commutatorOid) && OidIsValid(oprForm->oprcom) && + commutatorOid != oprForm->oprcom) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("operator attribute \"%s\" cannot be changed if it has already been set", + "commutator"))); + + if (OidIsValid(negatorOid) && OidIsValid(oprForm->oprnegate) && + negatorOid != oprForm->oprnegate) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("operator attribute \"%s\" cannot be changed if it has already been set", + "negator"))); + + if (updateMerges && oprForm->oprcanmerge && !canMerge) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("operator attribute \"%s\" cannot be changed if it has already been set", + "merges"))); + + if (updateHashes && oprForm->oprcanhash && !canHash) + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("operator attribute \"%s\" cannot be changed if it has already been set", + "hashes"))); + + /* Perform additional checks, like OperatorCreate does */ + OperatorValidateParams(oprForm->oprleft, + oprForm->oprright, + oprForm->oprresult, + OidIsValid(commutatorOid), + OidIsValid(negatorOid), + OidIsValid(restrictionOid), + OidIsValid(joinOid), + canMerge, + canHash); /* Update the tuple */ for (i = 0; i < Natts_pg_operator; ++i) @@ -531,12 +658,32 @@ AlterOperator(AlterOperatorStmt *stmt) if (updateRestriction) { replaces[Anum_pg_operator_oprrest - 1] = true; - values[Anum_pg_operator_oprrest - 1] = restrictionOid; + values[Anum_pg_operator_oprrest - 1] = ObjectIdGetDatum(restrictionOid); } if (updateJoin) { replaces[Anum_pg_operator_oprjoin - 1] = true; - values[Anum_pg_operator_oprjoin - 1] = joinOid; + values[Anum_pg_operator_oprjoin - 1] = ObjectIdGetDatum(joinOid); + } + if (OidIsValid(commutatorOid)) + { + replaces[Anum_pg_operator_oprcom - 1] = true; + values[Anum_pg_operator_oprcom - 1] = ObjectIdGetDatum(commutatorOid); + } + if (OidIsValid(negatorOid)) + { + replaces[Anum_pg_operator_oprnegate - 1] = true; + values[Anum_pg_operator_oprnegate - 1] = ObjectIdGetDatum(negatorOid); + } + if (updateMerges) + { + replaces[Anum_pg_operator_oprcanmerge - 1] = true; + values[Anum_pg_operator_oprcanmerge - 1] = BoolGetDatum(canMerge); + } + if (updateHashes) + { + replaces[Anum_pg_operator_oprcanhash - 1] = true; + values[Anum_pg_operator_oprcanhash - 1] = BoolGetDatum(canHash); } tup = heap_modify_tuple(tup, RelationGetDescr(catalog), @@ -546,6 +693,9 @@ AlterOperator(AlterOperatorStmt *stmt) address = makeOperatorDependencies(tup, false, true); + if (OidIsValid(commutatorOid) || OidIsValid(negatorOid)) + OperatorUpd(oprId, commutatorOid, negatorOid, false); + InvokeObjectPostAlterHook(OperatorRelationId, oprId, 0); table_close(catalog, NoLock); diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 50ed504e5a..c224df4ecc 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -10151,6 +10151,8 @@ operator_def_elem: ColLabel '=' NONE { $$ = makeDefElem($1, NULL, @1); } | ColLabel '=' operator_def_arg { $$ = makeDefElem($1, (Node *) $3, @1); } + | ColLabel + { $$ = makeDefElem($1, NULL, @1); } ; /* must be similar enough to def_arg to avoid reduce/reduce conflicts */ diff --git a/src/backend/parser/parse_oper.c b/src/backend/parser/parse_oper.c index bdc8f8e26a..d08fc23f5b 100644 --- a/src/backend/parser/parse_oper.c +++ b/src/backend/parser/parse_oper.c @@ -70,9 +70,7 @@ static FuncDetailCode oper_select_candidate(int nargs, Oid *input_typeids, FuncCandidateList candidates, Oid *operOid); -static const char *op_signature_string(List *op, char oprkind, - Oid arg1, Oid arg2); -static void op_error(ParseState *pstate, List *op, char oprkind, +static void op_error(ParseState *pstate, List *op, Oid arg1, Oid arg2, FuncDetailCode fdresult, int location); static bool make_oper_cache_key(ParseState *pstate, OprCacheKey *key, @@ -110,26 +108,16 @@ LookupOperName(ParseState *pstate, List *opername, Oid oprleft, Oid oprright, /* we don't use op_error here because only an exact match is wanted */ if (!noError) { - char oprkind; - - if (!OidIsValid(oprleft)) - oprkind = 'l'; - else if (OidIsValid(oprright)) - oprkind = 'b'; - else - { + if (!OidIsValid(oprright)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("postfix operators are not supported"), parser_errposition(pstate, location))); - oprkind = 0; /* keep compiler quiet */ - } ereport(ERROR, (errcode(ERRCODE_UNDEFINED_FUNCTION), errmsg("operator does not exist: %s", - op_signature_string(opername, oprkind, - oprleft, oprright)), + op_signature_string(opername, oprleft, oprright)), parser_errposition(pstate, location))); } @@ -446,7 +434,7 @@ oper(ParseState *pstate, List *opname, Oid ltypeId, Oid rtypeId, make_oper_cache_entry(&key, operOid); } else if (!noError) - op_error(pstate, opname, 'b', ltypeId, rtypeId, fdresult, location); + op_error(pstate, opname, ltypeId, rtypeId, fdresult, location); return (Operator) tup; } @@ -483,7 +471,7 @@ compatible_oper(ParseState *pstate, List *op, Oid arg1, Oid arg2, ereport(ERROR, (errcode(ERRCODE_UNDEFINED_FUNCTION), errmsg("operator requires run-time type coercion: %s", - op_signature_string(op, 'b', arg1, arg2)), + op_signature_string(op, arg1, arg2)), parser_errposition(pstate, location))); return (Operator) NULL; @@ -597,7 +585,7 @@ left_oper(ParseState *pstate, List *op, Oid arg, bool noError, int location) make_oper_cache_entry(&key, operOid); } else if (!noError) - op_error(pstate, op, 'l', InvalidOid, arg, fdresult, location); + op_error(pstate, op, InvalidOid, arg, fdresult, location); return (Operator) tup; } @@ -610,14 +598,14 @@ left_oper(ParseState *pstate, List *op, Oid arg, bool noError, int location) * This is typically used in the construction of operator-not-found error * messages. */ -static const char * -op_signature_string(List *op, char oprkind, Oid arg1, Oid arg2) +const char * +op_signature_string(List *op, Oid arg1, Oid arg2) { StringInfoData argbuf; initStringInfo(&argbuf); - if (oprkind != 'l') + if (OidIsValid(arg1)) appendStringInfo(&argbuf, "%s ", format_type_be(arg1)); appendStringInfoString(&argbuf, NameListToString(op)); @@ -631,7 +619,7 @@ op_signature_string(List *op, char oprkind, Oid arg1, Oid arg2) * op_error - utility routine to complain about an unresolvable operator */ static void -op_error(ParseState *pstate, List *op, char oprkind, +op_error(ParseState *pstate, List *op, Oid arg1, Oid arg2, FuncDetailCode fdresult, int location) { @@ -639,7 +627,7 @@ op_error(ParseState *pstate, List *op, char oprkind, ereport(ERROR, (errcode(ERRCODE_AMBIGUOUS_FUNCTION), errmsg("operator is not unique: %s", - op_signature_string(op, oprkind, arg1, arg2)), + op_signature_string(op, arg1, arg2)), errhint("Could not choose a best candidate operator. " "You might need to add explicit type casts."), parser_errposition(pstate, location))); @@ -647,7 +635,7 @@ op_error(ParseState *pstate, List *op, char oprkind, ereport(ERROR, (errcode(ERRCODE_UNDEFINED_FUNCTION), errmsg("operator does not exist: %s", - op_signature_string(op, oprkind, arg1, arg2)), + op_signature_string(op, arg1, arg2)), (!arg1 || !arg2) ? errhint("No operator matches the given name and argument type. " "You might need to add an explicit type cast.") : @@ -713,7 +701,6 @@ make_op(ParseState *pstate, List *opname, Node *ltree, Node *rtree, (errcode(ERRCODE_UNDEFINED_FUNCTION), errmsg("operator is only a shell: %s", op_signature_string(opname, - opform->oprkind, opform->oprleft, opform->oprright)), parser_errposition(pstate, location))); @@ -827,7 +814,6 @@ make_scalar_array_op(ParseState *pstate, List *opname, (errcode(ERRCODE_UNDEFINED_FUNCTION), errmsg("operator is only a shell: %s", op_signature_string(opname, - opform->oprkind, opform->oprleft, opform->oprright)), parser_errposition(pstate, location))); diff --git a/src/include/catalog/pg_operator.h b/src/include/catalog/pg_operator.h index aff372b4bb..e890cee2df 100644 --- a/src/include/catalog/pg_operator.h +++ b/src/include/catalog/pg_operator.h @@ -86,6 +86,11 @@ DECLARE_UNIQUE_INDEX_PKEY(pg_operator_oid_index, 2688, OperatorOidIndexId, pg_op DECLARE_UNIQUE_INDEX(pg_operator_oprname_l_r_n_index, 2689, OperatorNameNspIndexId, pg_operator, btree(oprname name_ops, oprleft oid_ops, oprright oid_ops, oprnamespace oid_ops)); +extern Oid OperatorLookup(List *operatorName, + Oid leftObjectId, + Oid rightObjectId, + bool *defined); + extern ObjectAddress OperatorCreate(const char *operatorName, Oid operatorNamespace, Oid leftTypeId, @@ -102,6 +107,16 @@ extern ObjectAddress makeOperatorDependencies(HeapTuple tuple, bool makeExtensionDep, bool isUpdate); +extern void OperatorValidateParams(Oid leftTypeId, + Oid rightTypeId, + Oid operResultType, + bool hasCommutator, + bool hasNegator, + bool hasRestrictionSelectivity, + bool hasJoinSelectivity, + bool canMerge, + bool canHash); + extern void OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete); #endif /* PG_OPERATOR_H */ diff --git a/src/include/parser/parse_oper.h b/src/include/parser/parse_oper.h index 5768a1ce87..c756caa360 100644 --- a/src/include/parser/parse_oper.h +++ b/src/include/parser/parse_oper.h @@ -42,6 +42,9 @@ extern Operator compatible_oper(ParseState *pstate, List *op, /* currently no need for compatible_left_oper/compatible_right_oper */ +/* Error reporting support */ +extern const char *op_signature_string(List *op, Oid arg1, Oid arg2); + /* Routines for identifying "<", "=", ">" operators for a type */ extern void get_sort_group_operators(Oid argtype, bool needLT, bool needEQ, bool needGT, diff --git a/src/test/regress/expected/alter_operator.out b/src/test/regress/expected/alter_operator.out index 71bd484282..4217ba15de 100644 --- a/src/test/regress/expected/alter_operator.out +++ b/src/test/regress/expected/alter_operator.out @@ -25,7 +25,7 @@ ORDER BY 1; (3 rows) -- --- Reset and set params +-- Test resetting and setting restrict and join attributes. -- ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE); ALTER OPERATOR === (boolean, boolean) SET (JOIN = NONE); @@ -109,18 +109,10 @@ ORDER BY 1; -- -- Test invalid options. -- -ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ====); -ERROR: operator attribute "commutator" cannot be changed -ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = ====); -ERROR: operator attribute "negator" cannot be changed ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = non_existent_func); ERROR: function non_existent_func(internal, oid, internal, integer) does not exist ALTER OPERATOR === (boolean, boolean) SET (JOIN = non_existent_func); ERROR: function non_existent_func(internal, oid, internal, smallint, internal) does not exist -ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = !==); -ERROR: operator attribute "commutator" cannot be changed -ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = !==); -ERROR: operator attribute "negator" cannot be changed -- invalid: non-lowercase quoted identifiers ALTER OPERATOR & (bit, bit) SET ("Restrict" = _int_contsel, "Join" = _int_contjoinsel); ERROR: operator attribute "Restrict" not recognized @@ -131,9 +123,145 @@ CREATE USER regress_alter_op_user; SET SESSION AUTHORIZATION regress_alter_op_user; ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE); ERROR: must be owner of operator === --- Clean up RESET SESSION AUTHORIZATION; +-- +-- Test setting commutator, negator, merges, and hashes attributes, +-- which can only be set if not already set +-- +CREATE FUNCTION alter_op_test_fn_bool_real(boolean, real) +RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE; +CREATE FUNCTION alter_op_test_fn_real_bool(real, boolean) +RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE; +-- operator +CREATE OPERATOR === ( + LEFTARG = boolean, + RIGHTARG = real, + PROCEDURE = alter_op_test_fn_bool_real +); +-- commutator +CREATE OPERATOR ==== ( + LEFTARG = real, + RIGHTARG = boolean, + PROCEDURE = alter_op_test_fn_real_bool +); +-- negator +CREATE OPERATOR !==== ( + LEFTARG = boolean, + RIGHTARG = real, + PROCEDURE = alter_op_test_fn_bool_real +); +-- No-op setting already false hashes and merges to false works +ALTER OPERATOR === (boolean, real) SET (MERGES = false); +ALTER OPERATOR === (boolean, real) SET (HASHES = false); +-- Test setting merges and hashes +ALTER OPERATOR === (boolean, real) SET (MERGES); +ALTER OPERATOR === (boolean, real) SET (HASHES); +SELECT oprcanmerge, oprcanhash +FROM pg_operator WHERE oprname = '===' + AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype; + oprcanmerge | oprcanhash +-------------+------------ + t | t +(1 row) + +-- Test setting commutator +ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====); +-- Check that oprcom has been set on both the operator and commutator, +-- that they reference each other, and that the operator used is the existing +-- one we created and not a new shell operator. +SELECT op.oprname AS operator_name, com.oprname AS commutator_name, + com.oprcode AS commutator_func + FROM pg_operator op + INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid) + WHERE op.oprname = '===' + AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype; + operator_name | commutator_name | commutator_func +---------------+-----------------+---------------------------- + === | ==== | alter_op_test_fn_real_bool +(1 row) + +-- Cannot set self as negator +ALTER OPERATOR === (boolean, real) SET (NEGATOR = ===); +ERROR: operator cannot be its own negator +-- Test setting negator +ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====); +-- Check that oprnegate has been set on both the operator and negator, +-- that they reference each other, and that the operator used is the existing +-- one we created and not a new shell operator. +SELECT op.oprname AS operator_name, neg.oprname AS negator_name, + neg.oprcode AS negator_func + FROM pg_operator op + INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid) + WHERE op.oprname = '===' + AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype; + operator_name | negator_name | negator_func +---------------+--------------+---------------------------- + === | !==== | alter_op_test_fn_bool_real +(1 row) + +-- Test that no-op set succeeds +ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====); +ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====); +ALTER OPERATOR === (boolean, real) SET (MERGES); +ALTER OPERATOR === (boolean, real) SET (HASHES); +-- Check that the final state of the operator is as we expect +SELECT oprcanmerge, oprcanhash, + pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator, + pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator + FROM pg_operator WHERE oprname = '===' + AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype; + oprcanmerge | oprcanhash | commutator | negator +-------------+------------+-----------------------------+------------------------------ + t | t | operator ====(real,boolean) | operator !====(boolean,real) +(1 row) + +-- Cannot change commutator, negator, merges, and hashes when already set +CREATE OPERATOR @= ( + LEFTARG = real, + RIGHTARG = boolean, + PROCEDURE = alter_op_test_fn_real_bool +); +CREATE OPERATOR @!= ( + LEFTARG = boolean, + RIGHTARG = real, + PROCEDURE = alter_op_test_fn_bool_real +); +ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = @=); +ERROR: operator attribute "commutator" cannot be changed if it has already been set +ALTER OPERATOR === (boolean, real) SET (NEGATOR = @!=); +ERROR: operator attribute "negator" cannot be changed if it has already been set +ALTER OPERATOR === (boolean, real) SET (MERGES = false); +ERROR: operator attribute "merges" cannot be changed if it has already been set +ALTER OPERATOR === (boolean, real) SET (HASHES = false); +ERROR: operator attribute "hashes" cannot be changed if it has already been set +-- Cannot set an operator that already has a commutator as the commutator +ALTER OPERATOR @=(real, boolean) SET (COMMUTATOR = ===); +ERROR: commutator operator === is already the commutator of operator ==== +-- Cannot set an operator that already has a negator as the negator +ALTER OPERATOR @!=(boolean, real) SET (NEGATOR = ===); +ERROR: negator operator === is already the negator of operator !==== +-- Check no changes made +SELECT oprcanmerge, oprcanhash, + pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator, + pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator + FROM pg_operator WHERE oprname = '===' + AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype; + oprcanmerge | oprcanhash | commutator | negator +-------------+------------+-----------------------------+------------------------------ + t | t | operator ====(real,boolean) | operator !====(boolean,real) +(1 row) + +-- +-- Clean up +-- DROP USER regress_alter_op_user; DROP OPERATOR === (boolean, boolean); +DROP OPERATOR === (boolean, real); +DROP OPERATOR ==== (real, boolean); +DROP OPERATOR !==== (boolean, real); +DROP OPERATOR @= (real, boolean); +DROP OPERATOR @!= (boolean, real); DROP FUNCTION customcontsel(internal, oid, internal, integer); DROP FUNCTION alter_op_test_fn(boolean, boolean); +DROP FUNCTION alter_op_test_fn_bool_real(boolean, real); +DROP FUNCTION alter_op_test_fn_real_bool(real, boolean); diff --git a/src/test/regress/expected/create_operator.out b/src/test/regress/expected/create_operator.out index f71b601f2d..d776d9c18c 100644 --- a/src/test/regress/expected/create_operator.out +++ b/src/test/regress/expected/create_operator.out @@ -260,6 +260,50 @@ CREATE OPERATOR #*# ( ); ERROR: permission denied for type type_op6 ROLLBACK; +-- Should fail. An operator cannot be its own negator. +BEGIN TRANSACTION; +CREATE OPERATOR === ( + leftarg = integer, + rightarg = integer, + procedure = int4eq, + negator = === +); +ERROR: operator cannot be its own negator +ROLLBACK; +-- Should fail. An operator cannot be its own negator. Here we check that +-- this error is detected when replacing a shell operator. +BEGIN TRANSACTION; +-- create a shell operator for ===!!! by referencing it as a commutator +CREATE OPERATOR === ( + leftarg = integer, + rightarg = integer, + procedure = int4eq, + commutator = ===!!! +); +CREATE OPERATOR ===!!! ( + leftarg = integer, + rightarg = integer, + procedure = int4ne, + negator = ===!!! +); +ERROR: operator cannot be its own negator +ROLLBACK; +-- test that we can't use part of an existing commutator or negator pair +-- as a commutator or negator +CREATE OPERATOR === ( + leftarg = integer, + rightarg = integer, + procedure = int4eq, + commutator = = +); +ERROR: commutator operator = is already the commutator of operator = +CREATE OPERATOR === ( + leftarg = integer, + rightarg = integer, + procedure = int4eq, + negator = <> +); +ERROR: negator operator <> is already the negator of operator = -- invalid: non-lowercase quoted identifiers CREATE OPERATOR === ( diff --git a/src/test/regress/sql/alter_operator.sql b/src/test/regress/sql/alter_operator.sql index fd40370165..8faecf7830 100644 --- a/src/test/regress/sql/alter_operator.sql +++ b/src/test/regress/sql/alter_operator.sql @@ -22,7 +22,7 @@ WHERE classid = 'pg_operator'::regclass AND ORDER BY 1; -- --- Reset and set params +-- Test resetting and setting restrict and join attributes. -- ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE); @@ -74,12 +74,8 @@ ORDER BY 1; -- -- Test invalid options. -- -ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = ====); -ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = ====); ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = non_existent_func); ALTER OPERATOR === (boolean, boolean) SET (JOIN = non_existent_func); -ALTER OPERATOR === (boolean, boolean) SET (COMMUTATOR = !==); -ALTER OPERATOR === (boolean, boolean) SET (NEGATOR = !==); -- invalid: non-lowercase quoted identifiers ALTER OPERATOR & (bit, bit) SET ("Restrict" = _int_contsel, "Join" = _int_contjoinsel); @@ -92,9 +88,138 @@ SET SESSION AUTHORIZATION regress_alter_op_user; ALTER OPERATOR === (boolean, boolean) SET (RESTRICT = NONE); --- Clean up RESET SESSION AUTHORIZATION; + +-- +-- Test setting commutator, negator, merges, and hashes attributes, +-- which can only be set if not already set +-- + +CREATE FUNCTION alter_op_test_fn_bool_real(boolean, real) +RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE; + +CREATE FUNCTION alter_op_test_fn_real_bool(real, boolean) +RETURNS boolean AS $$ SELECT NULL::BOOLEAN; $$ LANGUAGE sql IMMUTABLE; + +-- operator +CREATE OPERATOR === ( + LEFTARG = boolean, + RIGHTARG = real, + PROCEDURE = alter_op_test_fn_bool_real +); + +-- commutator +CREATE OPERATOR ==== ( + LEFTARG = real, + RIGHTARG = boolean, + PROCEDURE = alter_op_test_fn_real_bool +); + +-- negator +CREATE OPERATOR !==== ( + LEFTARG = boolean, + RIGHTARG = real, + PROCEDURE = alter_op_test_fn_bool_real +); + +-- No-op setting already false hashes and merges to false works +ALTER OPERATOR === (boolean, real) SET (MERGES = false); +ALTER OPERATOR === (boolean, real) SET (HASHES = false); + +-- Test setting merges and hashes +ALTER OPERATOR === (boolean, real) SET (MERGES); +ALTER OPERATOR === (boolean, real) SET (HASHES); +SELECT oprcanmerge, oprcanhash +FROM pg_operator WHERE oprname = '===' + AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype; + +-- Test setting commutator +ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====); + +-- Check that oprcom has been set on both the operator and commutator, +-- that they reference each other, and that the operator used is the existing +-- one we created and not a new shell operator. +SELECT op.oprname AS operator_name, com.oprname AS commutator_name, + com.oprcode AS commutator_func + FROM pg_operator op + INNER JOIN pg_operator com ON (op.oid = com.oprcom AND op.oprcom = com.oid) + WHERE op.oprname = '===' + AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype; + +-- Cannot set self as negator +ALTER OPERATOR === (boolean, real) SET (NEGATOR = ===); + +-- Test setting negator +ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====); + +-- Check that oprnegate has been set on both the operator and negator, +-- that they reference each other, and that the operator used is the existing +-- one we created and not a new shell operator. +SELECT op.oprname AS operator_name, neg.oprname AS negator_name, + neg.oprcode AS negator_func + FROM pg_operator op + INNER JOIN pg_operator neg ON (op.oid = neg.oprnegate AND op.oprnegate = neg.oid) + WHERE op.oprname = '===' + AND op.oprleft = 'boolean'::regtype AND op.oprright = 'real'::regtype; + +-- Test that no-op set succeeds +ALTER OPERATOR === (boolean, real) SET (NEGATOR = !====); +ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = ====); +ALTER OPERATOR === (boolean, real) SET (MERGES); +ALTER OPERATOR === (boolean, real) SET (HASHES); + +-- Check that the final state of the operator is as we expect +SELECT oprcanmerge, oprcanhash, + pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator, + pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator + FROM pg_operator WHERE oprname = '===' + AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype; + +-- Cannot change commutator, negator, merges, and hashes when already set + +CREATE OPERATOR @= ( + LEFTARG = real, + RIGHTARG = boolean, + PROCEDURE = alter_op_test_fn_real_bool +); +CREATE OPERATOR @!= ( + LEFTARG = boolean, + RIGHTARG = real, + PROCEDURE = alter_op_test_fn_bool_real +); + +ALTER OPERATOR === (boolean, real) SET (COMMUTATOR = @=); +ALTER OPERATOR === (boolean, real) SET (NEGATOR = @!=); +ALTER OPERATOR === (boolean, real) SET (MERGES = false); +ALTER OPERATOR === (boolean, real) SET (HASHES = false); + +-- Cannot set an operator that already has a commutator as the commutator +ALTER OPERATOR @=(real, boolean) SET (COMMUTATOR = ===); + +-- Cannot set an operator that already has a negator as the negator +ALTER OPERATOR @!=(boolean, real) SET (NEGATOR = ===); + +-- Check no changes made +SELECT oprcanmerge, oprcanhash, + pg_describe_object('pg_operator'::regclass, oprcom, 0) AS commutator, + pg_describe_object('pg_operator'::regclass, oprnegate, 0) AS negator + FROM pg_operator WHERE oprname = '===' + AND oprleft = 'boolean'::regtype AND oprright = 'real'::regtype; + +-- +-- Clean up +-- + DROP USER regress_alter_op_user; + DROP OPERATOR === (boolean, boolean); +DROP OPERATOR === (boolean, real); +DROP OPERATOR ==== (real, boolean); +DROP OPERATOR !==== (boolean, real); +DROP OPERATOR @= (real, boolean); +DROP OPERATOR @!= (boolean, real); + DROP FUNCTION customcontsel(internal, oid, internal, integer); DROP FUNCTION alter_op_test_fn(boolean, boolean); +DROP FUNCTION alter_op_test_fn_bool_real(boolean, real); +DROP FUNCTION alter_op_test_fn_real_bool(real, boolean); diff --git a/src/test/regress/sql/create_operator.sql b/src/test/regress/sql/create_operator.sql index f53e24db3c..a3096f17df 100644 --- a/src/test/regress/sql/create_operator.sql +++ b/src/test/regress/sql/create_operator.sql @@ -210,6 +210,49 @@ CREATE OPERATOR #*# ( ); ROLLBACK; +-- Should fail. An operator cannot be its own negator. +BEGIN TRANSACTION; +CREATE OPERATOR === ( + leftarg = integer, + rightarg = integer, + procedure = int4eq, + negator = === +); +ROLLBACK; + +-- Should fail. An operator cannot be its own negator. Here we check that +-- this error is detected when replacing a shell operator. +BEGIN TRANSACTION; +-- create a shell operator for ===!!! by referencing it as a commutator +CREATE OPERATOR === ( + leftarg = integer, + rightarg = integer, + procedure = int4eq, + commutator = ===!!! +); +CREATE OPERATOR ===!!! ( + leftarg = integer, + rightarg = integer, + procedure = int4ne, + negator = ===!!! +); +ROLLBACK; + +-- test that we can't use part of an existing commutator or negator pair +-- as a commutator or negator +CREATE OPERATOR === ( + leftarg = integer, + rightarg = integer, + procedure = int4eq, + commutator = = +); +CREATE OPERATOR === ( + leftarg = integer, + rightarg = integer, + procedure = int4eq, + negator = <> +); + -- invalid: non-lowercase quoted identifiers CREATE OPERATOR === (