Fix XXX comments

Author: Nikita Glukhov

src/backend/commands/operatorcmds.c

@@ -52,12 +52,7 @@
 
 static Oid	ValidateRestrictionEstimator(List *restrictionName);
 static Oid	ValidateJoinEstimator(List *joinName);
-
-/*
- * XXX Maybe not the right name, because "estimator" implies we're calculating
- * selectivity. But we're actually deriving statistics for an expression.
- */
-static Oid	ValidateStatisticsEstimator(List *joinName);
+static Oid	ValidateStatisticsDerivator(List *joinName);
 
 /*
  * DefineOperator
@@ -139,8 +134,7 @@ DefineOperator(List *names, List *parameters)
 			restrictionName = defGetQualifiedName(defel);
 		else if (strcmp(defel->defname, "join") == 0)
 			joinName = defGetQualifiedName(defel);
-		/* XXX perhaps full "statistics" wording would be better */
-		else if (strcmp(defel->defname, "stats") == 0)
+		else if (strcmp(defel->defname, "statistics") == 0)
 			statisticsName = defGetQualifiedName(defel);
 		else if (strcmp(defel->defname, "hashes") == 0)
 			canHash = defGetBoolean(defel);
@@ -258,7 +252,7 @@ DefineOperator(List *names, List *parameters)
 	else
 		joinOid = InvalidOid;
 	if (statisticsName)
-		statisticsOid = ValidateStatisticsEstimator(statisticsName);
+		statisticsOid = ValidateStatisticsDerivator(statisticsName);
 	else
 		statisticsOid = InvalidOid;
 
@@ -378,7 +372,7 @@ ValidateJoinEstimator(List *joinName)
  * correct signature and we have the permissions to attach it to an operator.
  */
 static Oid
-ValidateStatisticsEstimator(List *statName)
+ValidateStatisticsDerivator(List *statName)
 {
 	Oid			typeId[4];
 	Oid			statOid;
@@ -392,11 +386,11 @@ ValidateStatisticsEstimator(List *statName)
 	statOid = LookupFuncName(statName, 4, typeId, false);
 
 	/* statistics estimators must return void */
-	if (get_func_rettype(statOid) != BOOLOID)
+	if (get_func_rettype(statOid) != VOIDOID)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
 				 errmsg("statistics estimator function %s must return type %s",
-						NameListToString(statName), "boolean")));
+						NameListToString(statName), "void")));
 
 	/* Require EXECUTE rights for the estimator */
 	aclresult = pg_proc_aclcheck(statOid, GetUserId(), ACL_EXECUTE);
@@ -555,7 +549,7 @@ AlterOperator(AlterOperatorStmt *stmt)
 	else
 		joinOid = InvalidOid;
 	if (statName)
-		statOid = ValidateStatisticsEstimator(statName);
+		statOid = ValidateStatisticsDerivator(statName);
 	else
 		statOid = InvalidOid;
 

src/backend/utils/adt/selfuncs.c

@@ -4922,25 +4922,23 @@ ReleaseDummy(HeapTuple tuple)
  *		Try to derive optimizer statistics for the operator expression using
  *		operator's oprstat function.
  *
- * XXX Not sure why this returns bool - we ignore the return value anyway. We
- * might as well return the calculated vardata (or NULL).
- *
- * XXX Not sure what to do about recursion - there can be another OpExpr in
- * one of the arguments, and we should call this recursively from the oprstat
- * procedure. But that's not possible, because it's marked as static.
+ * There can be another OpExpr in one of the arguments, and it will be called
+ * recursively from the oprstat procedure through the following chain:
+ * get_restriction_variable() => examine_variable() =>
+ * examine_operator_expression().
  */
-static bool
+static void
 examine_operator_expression(PlannerInfo *root, OpExpr *opexpr, int varRelid,
 							VariableStatData *vardata)
 {
 	RegProcedure oprstat = get_oprstat(opexpr->opno);
 
-	return OidIsValid(oprstat) &&
-		DatumGetBool(OidFunctionCall4(oprstat,
-									  PointerGetDatum(root),
-									  PointerGetDatum(opexpr),
-									  Int32GetDatum(varRelid),
-									  PointerGetDatum(vardata)));
+	if (OidIsValid(oprstat))
+		OidFunctionCall4(oprstat,
+						 PointerGetDatum(root),
+						 PointerGetDatum(opexpr),
+						 Int32GetDatum(varRelid),
+						 PointerGetDatum(vardata));
 }
 
 /*
@@ -5365,8 +5363,9 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
 		 * operator expression (OpExpr). We do this last because it may be quite
 		 * expensive, and it's unclear how accurate the statistics will be.
 		 *
-		 * XXX Shouldn't this put more restrictions on the OpExpr? E.g. that
-		 * one of the arguments has to be a Const or something?
+		 * More restrictions on the OpExpr (e.g. that one of the arguments
+		 * has to be a Const or something) can be put by the operator itself
+		 * in its oprstat function.
 		 */
 		if (!vardata->statsTuple && IsA(basenode, OpExpr))
 			examine_operator_expression(root, (OpExpr *) basenode, varRelid,