Fix type-safety problem with parallel aggregate serial/deserialization.

The original specification for this called for the deserialization function
to have signature "deserialize(serialtype) returns transtype", which is a
security violation if transtype is INTERNAL (which it always would be in
practice) and serialtype is not (which ditto).  The patch blithely overrode
the opr_sanity check for that, which was sloppy-enough work in itself,
but the indisputable reason this cannot be allowed to stand is that CREATE
FUNCTION will reject such a signature and thus it'd be impossible for
extensions to create parallelizable aggregates.

The minimum fix to make the signature type-safe is to add a second, dummy
argument of type INTERNAL.  But to lock it down a bit more and make misuse
of INTERNAL-accepting functions less likely, let's get rid of the ability
to specify a "serialtype" for an aggregate and just say that the only
useful serialtype is BYTEA --- which, in practice, is the only interesting
value anyway, due to the usefulness of the send/recv infrastructure for
this purpose.  That means we only have to allow "serialize(internal)
returns bytea" and "deserialize(bytea, internal) returns internal" as
the signatures for these support functions.

In passing fix bogus signature of int4_avg_combine, which I found thanks
to adding an opr_sanity check on combinefunc signatures.

catversion bump due to removing pg_aggregate.aggserialtype and adjusting
signatures of assorted built-in functions.

David Rowley and Tom Lane

Discussion: <27247.1466185504@sss.pgh.pa.us>

1 files changed, 8 insertions, 61 deletions

diff --git a/src/backend/commands/aggregatecmds.c b/src/backend/commands/aggregatecmds.c
index f1fdc1a3603..d34c82c5baf 100644
--- a/src/backend/commands/aggregatecmds.c
+++ b/src/backend/commands/aggregatecmds.c
@@ -72,7 +72,6 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters,
 	List	   *sortoperatorName = NIL;
 	TypeName   *baseType = NULL;
 	TypeName   *transType = NULL;
-	TypeName   *serialType = NULL;
 	TypeName   *mtransType = NULL;
 	int32		transSpace = 0;
 	int32		mtransSpace = 0;
@@ -88,7 +87,6 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters,
 	List	   *parameterDefaults;
 	Oid			variadicArgType;
 	Oid			transTypeId;
-	Oid			serialTypeId = InvalidOid;
 	Oid			mtransTypeId = InvalidOid;
 	char		transTypeType;
 	char		mtransTypeType = 0;
@@ -164,8 +162,6 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters,
 		}
 		else if (pg_strcasecmp(defel->defname, "stype") == 0)
 			transType = defGetTypeName(defel);
-		else if (pg_strcasecmp(defel->defname, "serialtype") == 0)
-			serialType = defGetTypeName(defel);
 		else if (pg_strcasecmp(defel->defname, "stype1") == 0)
 			transType = defGetTypeName(defel);
 		else if (pg_strcasecmp(defel->defname, "sspace") == 0)
@@ -333,73 +329,25 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters,
 							format_type_be(transTypeId))));
 	}
 
-	if (serialType)
+	if (serialfuncName && deserialfuncName)
 	{
 		/*
-		 * There's little point in having a serialization/deserialization
-		 * function on aggregates that don't have an internal state, so let's
-		 * just disallow this as it may help clear up any confusion or
-		 * needless authoring of these functions.
+		 * Serialization is only needed/allowed for transtype INTERNAL.
 		 */
 		if (transTypeId != INTERNALOID)
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("a serialization type must only be specified when the aggregate transition data type is %s",
+					 errmsg("serialization functions may be specified only when the aggregate transition data type is %s",
 							format_type_be(INTERNALOID))));
-
-		serialTypeId = typenameTypeId(NULL, serialType);
-
-		if (get_typtype(mtransTypeId) == TYPTYPE_PSEUDO &&
-			!IsPolymorphicType(serialTypeId))
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("aggregate serialization data type cannot be %s",
-							format_type_be(serialTypeId))));
-
-		/*
-		 * We disallow INTERNAL serialType as the whole point of the
-		 * serialized types is to allow the aggregate state to be output, and
-		 * we cannot output INTERNAL. This check, combined with the one above
-		 * ensures that the trans type and serialization type are not the
-		 * same.
-		 */
-		if (serialTypeId == INTERNALOID)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("aggregate serialization data type cannot be %s",
-							format_type_be(serialTypeId))));
-
-		/*
-		 * If serialType is specified then serialfuncName and deserialfuncName
-		 * must be present; if not, then none of the serialization options
-		 * should have been specified.
-		 */
-		if (serialfuncName == NIL)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("aggregate serialization function must be specified when serialization type is specified")));
-
-		if (deserialfuncName == NIL)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("aggregate deserialization function must be specified when serialization type is specified")));
 	}
-	else
+	else if (serialfuncName || deserialfuncName)
 	{
 		/*
-		 * If serialization type was not specified then there shouldn't be a
-		 * serialization function.
+		 * Cannot specify one function without the other.
 		 */
-		if (serialfuncName != NIL)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("must specify serialization type when specifying serialization function")));
-
-		/* likewise for the deserialization function */
-		if (deserialfuncName != NIL)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("must specify serialization type when specifying deserialization function")));
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+				 errmsg("must specify both or neither of serialization and deserialization functions")));
 	}
 
 	/*
@@ -493,7 +441,6 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters,
 						   mfinalfuncExtraArgs,
 						   sortoperatorName,	/* sort operator name */
 						   transTypeId, /* transition data type */
-						   serialTypeId,		/* serialization data type */
 						   transSpace,	/* transition space */
 						   mtransTypeId,		/* transition data type */
 						   mtransSpace, /* transition space */