Clean up the loose ends in selectivity estimation left by my patch for semi

and anti joins.  To do this, pass the SpecialJoinInfo struct for the current
join as an additional optional argument to operator join selectivity
estimation functions.  This allows the estimator to tell not only what kind
of join is being formed, but which variable is on which side of the join;
a requirement long recognized but not dealt with till now.  This also leaves
the door open for future improvements in the estimators, such as accounting
for the null-insertion effects of lower outer joins.  I didn't do anything
about that in the current patch but the information is in principle deducible
from what's passed.

The patch also clarifies the definition of join selectivity for semi/anti
joins: it's the fraction of the left input that has (at least one) match
in the right input.  This allows getting rid of some very fuzzy thinking
that I had committed in the original 7.4-era IN-optimization patch.
There's probably room to estimate this better than the present patch does,
but at least we know what to estimate.

Since I had to touch CREATE OPERATOR anyway to allow a variant signature
for join estimator functions, I took the opportunity to add a couple of
additional checks that were missing, per my recent message to -hackers:
* Check that estimator functions return float8;
* Require execute permission at the time of CREATE OPERATOR on the
operator's function as well as the estimator functions;
* Require ownership of any pre-existing operator that's modified by
the command.
I also moved the lookup of the functions out of OperatorCreate() and
into operatorcmds.c, since that seemed more consistent with most of
the other catalog object creation processes, eg CREATE TYPE.

Author: tglsfdc

src/backend/catalog/pg_operator.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/catalog/pg_operator.c,v 1.104 2008/06/19 00:46:04 alvherre Exp $
+ *	  $PostgreSQL: pgsql/src/backend/catalog/pg_operator.c,v 1.105 2008/08/16 00:01:35 tgl Exp $
  *
  * NOTES
  *	  these routines moved here from commands/define.c and somewhat cleaned up.
@@ -21,12 +21,12 @@
 #include "access/xact.h"
 #include "catalog/dependency.h"
 #include "catalog/indexing.h"
+#include "catalog/namespace.h"
 #include "catalog/pg_namespace.h"
 #include "catalog/pg_operator.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
 #include "miscadmin.h"
-#include "parser/parse_func.h"
 #include "parser/parse_oper.h"
 #include "utils/acl.h"
 #include "utils/builtins.h"
@@ -273,6 +273,11 @@ OperatorShellMake(const char *operatorName,
 
 	heap_freetuple(tup);
 
+	/*
+	 * Make sure the tuple is visible for subsequent lookups/updates.
+	 */
+	CommandCounterIncrement();
+
 	/*
 	 * close the operator relation and return the oid.
 	 */
@@ -289,14 +294,19 @@ OperatorShellMake(const char *operatorName,
  *		operatorNamespace		namespace for new operator
  *		leftTypeId				X left type ID
  *		rightTypeId				X right type ID
- *		procedureName			procedure for operator
+ *		procedureId				procedure ID for operator
  *		commutatorName			X commutator operator
  *		negatorName				X negator operator
- *		restrictionName			X restriction sel. procedure
- *		joinName				X join sel. procedure
+ *		restrictionId			X restriction selectivity procedure ID
+ *		joinId					X join selectivity procedure ID
  *		canMerge				merge join can be used with this operator
  *		canHash					hash join can be used with this operator
  *
+ * The caller should have validated properties and permissions for the
+ * objects passed as OID references.  We must handle the commutator and
+ * negator operator references specially, however, since those need not
+ * exist beforehand.
+ *
  * This routine gets complicated because it allows the user to
  * specify operators that do not exist.  For example, if operator
  * "op" is being defined, the negator operator "negop" and the
@@ -310,59 +320,17 @@ OperatorShellMake(const char *operatorName,
  * information about the operator (just its name and types).
  * Forward declaration is used only for this purpose, it is
  * not available to the user as it is for type definition.
- *
- * Algorithm:
- *
- * check if operator already defined
- *	  if so, but oprcode is null, save the Oid -- we are filling in a shell
- *	  otherwise error
- * get the attribute types from relation descriptor for pg_operator
- * assign values to the fields of the operator:
- *	 operatorName
- *	 owner id (simply the user id of the caller)
- *	 operator "kind" either "b" for binary or "l" for left unary
- *	 canMerge boolean
- *	 canHash boolean
- *	 leftTypeObjectId -- type must already be defined
- *	 rightTypeObjectId -- this is optional, enter ObjectId=0 if none specified
- *	 resultType -- defer this, since it must be determined from
- *				   the pg_procedure catalog
- *	 commutatorObjectId -- if this is NULL, enter ObjectId=0
- *					  else if this already exists, enter its ObjectId
- *					  else if this does not yet exist, and is not
- *						the same as the main operatorName, then create
- *						a shell and enter the new ObjectId
- *					  else if this does not exist but IS the same
- *						name & types as the main operator, set the ObjectId=0.
- *						(We are creating a self-commutating operator.)
- *						The link will be fixed later by OperatorUpd.
- *	 negatorObjectId   -- same as for commutatorObjectId
- *	 operatorProcedure -- must access the pg_procedure catalog to get the
- *				   ObjectId of the procedure that actually does the operator
- *				   actions this is required.  Do a lookup to find out the
- *				   return type of the procedure
- *	 restrictionProcedure -- must access the pg_procedure catalog to get
- *				   the ObjectId but this is optional
- *	 joinProcedure -- same as restrictionProcedure
- * now either insert or replace the operator into the pg_operator catalog
- * if the operator shell is being filled in
- *	 access the catalog in order to get a valid buffer
- *	 create a tuple using ModifyHeapTuple
- *	 get the t_self from the modified tuple and call RelationReplaceHeapTuple
- * else if a new operator is being created
- *	 create a tuple using heap_formtuple
- *	 call simple_heap_insert
  */
 void
 OperatorCreate(const char *operatorName,
 			   Oid operatorNamespace,
 			   Oid leftTypeId,
 			   Oid rightTypeId,
-			   List *procedureName,
+			   Oid procedureId,
 			   List *commutatorName,
 			   List *negatorName,
-			   List *restrictionName,
-			   List *joinName,
+			   Oid restrictionId,
+			   Oid joinId,
 			   bool canMerge,
 			   bool canHash)
 {
@@ -373,15 +341,10 @@ OperatorCreate(const char *operatorName,
 	Datum		values[Natts_pg_operator];
 	Oid			operatorObjectId;
 	bool		operatorAlreadyDefined;
-	Oid			procOid;
 	Oid			operResultType;
 	Oid			commutatorId,
-				negatorId,
-				restOid,
-				joinOid;
+				negatorId;
 	bool		selfCommutator = false;
-	Oid			typeId[4];		/* only need up to 4 args here */
-	int			nargs;
 	NameData	oname;
 	TupleDesc	tupDesc;
 	int			i;
@@ -395,19 +358,14 @@ OperatorCreate(const char *operatorName,
 				 errmsg("\"%s\" is not a valid operator name",
 						operatorName)));
 
-	if (!OidIsValid(leftTypeId) && !OidIsValid(rightTypeId))
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-		   errmsg("at least one of leftarg or rightarg must be specified")));
-
 	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 (joinName)
+		if (OidIsValid(joinId))
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
 				 errmsg("only binary operators can have join selectivity")));
@@ -421,6 +379,33 @@ OperatorCreate(const char *operatorName,
 					 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")));
+	}
+
 	operatorObjectId = OperatorGet(operatorName,
 								   operatorNamespace,
 								   leftTypeId,
@@ -435,85 +420,13 @@ OperatorCreate(const char *operatorName,
 
 	/*
 	 * At this point, if operatorObjectId is not InvalidOid then we are
-	 * filling in a previously-created shell.
-	 */
-
-	/*
-	 * Look up registered procedures -- find the return type of procedureName
-	 * to place in "result" field. Do this before shells are created so we
-	 * don't have to worry about deleting them later.
-	 */
-	if (!OidIsValid(leftTypeId))
-	{
-		typeId[0] = rightTypeId;
-		nargs = 1;
-	}
-	else if (!OidIsValid(rightTypeId))
-	{
-		typeId[0] = leftTypeId;
-		nargs = 1;
-	}
-	else
-	{
-		typeId[0] = leftTypeId;
-		typeId[1] = rightTypeId;
-		nargs = 2;
-	}
-	procOid = LookupFuncName(procedureName, nargs, typeId, false);
-	operResultType = get_func_rettype(procOid);
-
-	/*
-	 * find restriction estimator
-	 */
-	if (restrictionName)
-	{
-		typeId[0] = INTERNALOID;	/* Query */
-		typeId[1] = OIDOID;		/* operator OID */
-		typeId[2] = INTERNALOID;	/* args list */
-		typeId[3] = INT4OID;	/* varRelid */
-
-		restOid = LookupFuncName(restrictionName, 4, typeId, false);
-	}
-	else
-		restOid = InvalidOid;
-
-	/*
-	 * find join estimator
-	 */
-	if (joinName)
-	{
-		typeId[0] = INTERNALOID;	/* Query */
-		typeId[1] = OIDOID;		/* operator OID */
-		typeId[2] = INTERNALOID;	/* args list */
-		typeId[3] = INT2OID;	/* jointype */
-
-		joinOid = LookupFuncName(joinName, 4, typeId, false);
-	}
-	else
-		joinOid = InvalidOid;
-
-	/*
-	 * set up values in the operator tuple
+	 * filling in a previously-created shell.  Insist that the user own
+	 * any such shell.
 	 */
-
-	for (i = 0; i < Natts_pg_operator; ++i)
-	{
-		values[i] = (Datum) NULL;
-		replaces[i] = 'r';
-		nulls[i] = ' ';
-	}
-
-	i = 0;
-	namestrcpy(&oname, operatorName);
-	values[i++] = NameGetDatum(&oname); /* oprname */
-	values[i++] = ObjectIdGetDatum(operatorNamespace);	/* oprnamespace */
-	values[i++] = ObjectIdGetDatum(GetUserId());		/* oprowner */
-	values[i++] = CharGetDatum(leftTypeId ? (rightTypeId ? 'b' : 'r') : 'l');	/* oprkind */
-	values[i++] = BoolGetDatum(canMerge);		/* oprcanmerge */
-	values[i++] = BoolGetDatum(canHash);		/* oprcanhash */
-	values[i++] = ObjectIdGetDatum(leftTypeId); /* oprleft */
-	values[i++] = ObjectIdGetDatum(rightTypeId);		/* oprright */
-	values[i++] = ObjectIdGetDatum(operResultType);		/* oprresult */
+	if (OidIsValid(operatorObjectId) &&
+		!pg_oper_ownercheck(operatorObjectId, GetUserId()))
+		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_OPER,
+					   operatorName);
 
 	/*
 	 * Set up the other operators.	If they do not currently exist, create
@@ -529,6 +442,12 @@ OperatorCreate(const char *operatorName,
 										  leftTypeId, rightTypeId,
 										  true);
 
+		/* Permission check: must own other operator */
+		if (OidIsValid(commutatorId) &&
+			!pg_oper_ownercheck(commutatorId, GetUserId()))
+			aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_OPER,
+						   NameListToString(commutatorName));
+
 		/*
 		 * self-linkage to this operator; will fix below. Note that only
 		 * self-linkage for commutation makes sense.
@@ -538,7 +457,6 @@ OperatorCreate(const char *operatorName,
 	}
 	else
 		commutatorId = InvalidOid;
-	values[i++] = ObjectIdGetDatum(commutatorId);		/* oprcom */
 
 	if (negatorName)
 	{
@@ -548,19 +466,48 @@ OperatorCreate(const char *operatorName,
 									   operatorName, operatorNamespace,
 									   leftTypeId, rightTypeId,
 									   false);
+
+		/* Permission check: must own other operator */
+		if (OidIsValid(negatorId) &&
+			!pg_oper_ownercheck(negatorId, GetUserId()))
+			aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_OPER,
+						   NameListToString(negatorName));
 	}
 	else
 		negatorId = InvalidOid;
-	values[i++] = ObjectIdGetDatum(negatorId);	/* oprnegate */
 
-	values[i++] = ObjectIdGetDatum(procOid);	/* oprcode */
-	values[i++] = ObjectIdGetDatum(restOid);	/* oprrest */
-	values[i++] = ObjectIdGetDatum(joinOid);	/* oprjoin */
+	/*
+	 * set up values in the operator tuple
+	 */
+
+	for (i = 0; i < Natts_pg_operator; ++i)
+	{
+		values[i] = (Datum) NULL;
+		replaces[i] = 'r';
+		nulls[i] = ' ';
+	}
+
+	i = 0;
+	namestrcpy(&oname, operatorName);
+	values[i++] = NameGetDatum(&oname); /* oprname */
+	values[i++] = ObjectIdGetDatum(operatorNamespace);	/* oprnamespace */
+	values[i++] = ObjectIdGetDatum(GetUserId());		/* oprowner */
+	values[i++] = CharGetDatum(leftTypeId ? (rightTypeId ? 'b' : 'r') : 'l');	/* oprkind */
+	values[i++] = BoolGetDatum(canMerge);		/* oprcanmerge */
+	values[i++] = BoolGetDatum(canHash);		/* oprcanhash */
+	values[i++] = ObjectIdGetDatum(leftTypeId); /* oprleft */
+	values[i++] = ObjectIdGetDatum(rightTypeId);		/* oprright */
+	values[i++] = ObjectIdGetDatum(operResultType);		/* oprresult */
+	values[i++] = ObjectIdGetDatum(commutatorId);		/* oprcom */
+	values[i++] = ObjectIdGetDatum(negatorId);		/* oprnegate */
+	values[i++] = ObjectIdGetDatum(procedureId);	/* oprcode */
+	values[i++] = ObjectIdGetDatum(restrictionId);	/* oprrest */
+	values[i++] = ObjectIdGetDatum(joinId);			/* oprjoin */
 
 	pg_operator_desc = heap_open(OperatorRelationId, RowExclusiveLock);
 
 	/*
-	 * If we are adding to an operator shell, update; else insert
+	 * If we are replacing an operator shell, update; else insert
 	 */
 	if (operatorObjectId)
 	{
@@ -706,7 +653,8 @@ OperatorUpd(Oid baseId, Oid commId, Oid negId)
 	/*
 	 * check and update the commutator & negator, if necessary
 	 *
-	 * First make sure we can see them...
+	 * We need a CommandCounterIncrement here in case of a self-commutator
+	 * operator: we'll need to update the tuple that we just inserted.
 	 */
 	CommandCounterIncrement();