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

1 files changed, 172 insertions, 22 deletions

diff --git a/src/backend/commands/operatorcmds.c b/src/backend/commands/operatorcmds.c
index cd7f83136f7..df84b839f53 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
@@ -360,6 +363,53 @@ ValidateJoinEstimator(List *joinName)
 }
 
 /*
+ * 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.
  */
 void
@@ -406,6 +456,10 @@ RemoveOperatorById(Oid operOid)
  *		routine implementing ALTER OPERATOR <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,28 +575,79 @@ 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))
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("only binary operators can have join selectivity")));
+		/* 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 (oprForm->oprresult != BOOLOID)
+	if (negatorName)
 	{
-		if (OidIsValid(restrictionOid))
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("only boolean operators can have restriction selectivity")));
-		if (OidIsValid(joinOid))
+		negatorOid = ValidateOperatorReference(negatorName,
+											   oprForm->oprleft,
+											   oprForm->oprright);
+
+		/* Must reject self-negation */
+		if (negatorOid == oprForm->oid)
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("only boolean operators can have join selectivity")));
+					 errmsg("operator cannot be its own negator")));
+	}
+	else
+	{
+		negatorOid = InvalidOid;
 	}
 
+	/*
+	 * 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);