Code review for protransform patches.

Fix loss of previous expression-simplification work when a transform
function fires: we must not simply revert to untransformed input tree.
Instead build a dummy FuncExpr node to pass to the transform function.
This has the additional advantage of providing a simpler, more uniform
API for transform functions.

Move documentation to a somewhat less buried spot, relocate some
poorly-placed code, be more wary of null constants and invalid typmod
values, add an opr_sanity check on protransform function signatures,
and some other minor cosmetic adjustments.

Note: although this patch touches pg_proc.h, no need for catversion
bump, because the changes are cosmetic and don't actually change the
intended catalog contents.

src/backend/utils/adt/datetime.c

@@ -24,7 +24,6 @@
 #include "funcapi.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
-#include "parser/parse_clause.h"
 #include "utils/builtins.h"
 #include "utils/date.h"
 #include "utils/datetime.h"
@@ -4153,32 +4152,34 @@ CheckDateTokenTables(void)
 }
 
 /*
- * Helper for temporal protransform functions.  Types time, timetz, timestamp
- * and timestamptz each have a range of allowed precisions.  An unspecified
- * precision is rigorously equivalent to the highest specifiable precision.
+ * Common code for temporal protransform functions.  Types time, timetz,
+ * timestamp and timestamptz each have a range of allowed precisions.  An
+ * unspecified precision is rigorously equivalent to the highest specifiable
+ * precision.
+ *
+ * Note: timestamp_scale throws an error when the typmod is out of range, but
+ * we can't get there from a cast: our typmodin will have caught it already.
  */
 Node *
 TemporalTransform(int32 max_precis, Node *node)
 {
 	FuncExpr   *expr = (FuncExpr *) node;
-	Node	   *typmod;
 	Node	   *ret = NULL;
+	Node	   *typmod;
 
-	if (!IsA(expr, FuncExpr))
-		return ret;
+	Assert(IsA(expr, FuncExpr));
+	Assert(list_length(expr->args) >= 2);
 
-	Assert(list_length(expr->args) == 2);
-	typmod = lsecond(expr->args);
+	typmod = (Node *) lsecond(expr->args);
 
-	if (IsA(typmod, Const))
+	if (IsA(typmod, Const) && !((Const *) typmod)->constisnull)
 	{
-		Node	   *source = linitial(expr->args);
+		Node	   *source = (Node *) linitial(expr->args);
 		int32		old_precis = exprTypmod(source);
 		int32		new_precis = DatumGetInt32(((Const *) typmod)->constvalue);
 
-		if (new_precis == -1 ||
-			new_precis == max_precis ||
-			(old_precis != -1 && new_precis >= old_precis))
+		if (new_precis < 0 || new_precis == max_precis ||
+			(old_precis >= 0 && new_precis >= old_precis))
 			ret = relabel_to_typmod(source, new_precis);
 	}
 

src/backend/utils/adt/numeric.c

@@ -31,7 +31,6 @@
 #include "libpq/pqformat.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
-#include "parser/parse_clause.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/int8.h"
@@ -717,7 +716,7 @@ numeric_send(PG_FUNCTION_ARGS)
 /*
  * numeric_transform() -
  *
- * Flatten calls to our length coercion function that solely represent
+ * Flatten calls to numeric's length coercion function that solely represent
  * increases in allowable precision.  Scale changes mutate every datum, so
  * they are unoptimizable.  Some values, e.g. 1E-1001, can only fit into an
  * unconstrained numeric, so a change from an unconstrained numeric to any
@@ -727,18 +726,17 @@ Datum
 numeric_transform(PG_FUNCTION_ARGS)
 {
 	FuncExpr   *expr = (FuncExpr *) PG_GETARG_POINTER(0);
-	Node	   *typmod;
 	Node	   *ret = NULL;
+	Node	   *typmod;
 
-	if (!IsA(expr, FuncExpr))
-		PG_RETURN_POINTER(ret);
+	Assert(IsA(expr, FuncExpr));
+	Assert(list_length(expr->args) >= 2);
 
-	Assert(list_length(expr->args) == 2);
-	typmod = lsecond(expr->args);
+	typmod = (Node *) lsecond(expr->args);
 
-	if (IsA(typmod, Const))
+	if (IsA(typmod, Const) && !((Const *) typmod)->constisnull)
 	{
-		Node	   *source = linitial(expr->args);
+		Node	   *source = (Node *) linitial(expr->args);
 		int32		old_typmod = exprTypmod(source);
 		int32		new_typmod = DatumGetInt32(((Const *) typmod)->constvalue);
 		int32		old_scale = (old_typmod - VARHDRSZ) & 0xffff;
@@ -748,11 +746,12 @@ numeric_transform(PG_FUNCTION_ARGS)
 
 		/*
 		 * If new_typmod < VARHDRSZ, the destination is unconstrained; that's
-		 * always OK.  If old_typmod >= VARHDRSZ, the source is constrained.
-		 * and we're OK if the scale is unchanged and the precison is not
+		 * always OK.  If old_typmod >= VARHDRSZ, the source is constrained,
+		 * and we're OK if the scale is unchanged and the precision is not
 		 * decreasing.  See further notes in function header comment.
 		 */
-		if (new_typmod < VARHDRSZ || (old_typmod >= VARHDRSZ &&
+		if (new_typmod < (int32) VARHDRSZ ||
+			(old_typmod >= (int32) VARHDRSZ &&
 			 new_scale == old_scale && new_precision >= old_precision))
 			ret = relabel_to_typmod(source, new_typmod);
 	}

src/backend/utils/adt/timestamp.c

@@ -28,7 +28,6 @@
 #include "libpq/pqformat.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
-#include "parser/parse_clause.h"
 #include "parser/scansup.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
@@ -316,10 +315,6 @@ timestamptypmodout(PG_FUNCTION_ARGS)
 Datum
 timestamp_transform(PG_FUNCTION_ARGS)
 {
-	/*
-	 * timestamp_scale throws an error when the typmod is out of range, but we
-	 * can't get there from a cast: our typmodin will have caught it already.
-	 */
 	PG_RETURN_POINTER(TemporalTransform(MAX_TIMESTAMP_PRECISION,
 										(Node *) PG_GETARG_POINTER(0)));
 }
@@ -937,18 +932,17 @@ Datum
 interval_transform(PG_FUNCTION_ARGS)
 {
 	FuncExpr   *expr = (FuncExpr *) PG_GETARG_POINTER(0);
-	Node	   *typmod;
 	Node	   *ret = NULL;
+	Node	   *typmod;
 
-	if (!IsA(expr, FuncExpr))
-		PG_RETURN_POINTER(ret);
+	Assert(IsA(expr, FuncExpr));
+	Assert(list_length(expr->args) >= 2);
 
-	Assert(list_length(expr->args) == 2);
-	typmod = lsecond(expr->args);
+	typmod = (Node *) lsecond(expr->args);
 
-	if (IsA(typmod, Const))
+	if (IsA(typmod, Const) && !((Const *) typmod)->constisnull)
 	{
-		Node	   *source = linitial(expr->args);
+		Node	   *source = (Node *) linitial(expr->args);
 		int32		old_typmod = exprTypmod(source);
 		int32		new_typmod = DatumGetInt32(((Const *) typmod)->constvalue);
 		int			old_range;
@@ -958,7 +952,7 @@ interval_transform(PG_FUNCTION_ARGS)
 		int			new_range_fls;
 		int			old_range_fls;
 
-		if (old_typmod == -1)
+		if (old_typmod < 0)
 		{
 			old_range = INTERVAL_FULL_RANGE;
 			old_precis = INTERVAL_FULL_PRECISION;
@@ -978,11 +972,9 @@ interval_transform(PG_FUNCTION_ARGS)
 		 */
 		new_range_fls = fls(new_range);
 		old_range_fls = fls(old_range);
-		if (new_typmod == -1 ||
-			((new_range_fls >= SECOND ||
-			  new_range_fls >= old_range_fls) &&
-			 (old_range_fls < SECOND ||
-			  new_precis >= MAX_INTERVAL_PRECISION ||
+		if (new_typmod < 0 ||
+			((new_range_fls >= SECOND || new_range_fls >= old_range_fls) &&
+			 (old_range_fls < SECOND || new_precis >= MAX_INTERVAL_PRECISION ||
 			  new_precis >= old_precis)))
 			ret = relabel_to_typmod(source, new_typmod);
 	}
@@ -1068,7 +1060,7 @@ AdjustIntervalForTypmod(Interval *interval, int32 typmod)
 		 * can't do it consistently.  (We cannot enforce a range limit on the
 		 * highest expected field, since we do not have any equivalent of
 		 * SQL's <interval leading field precision>.)  If we ever decide to
-		 * revisit this, interval_transform will likely requite adjusting.
+		 * revisit this, interval_transform will likely require adjusting.
 		 *
 		 * Note: before PG 8.4 we interpreted a limited set of fields as
 		 * actually causing a "modulo" operation on a given value, potentially

src/backend/utils/adt/varbit.c

@@ -19,7 +19,6 @@
 #include "access/htup.h"
 #include "libpq/pqformat.h"
 #include "nodes/nodeFuncs.h"
-#include "parser/parse_clause.h"
 #include "utils/array.h"
 #include "utils/varbit.h"
 
@@ -649,31 +648,31 @@ varbit_send(PG_FUNCTION_ARGS)
 
 /*
  * varbit_transform()
- * Flatten calls to our length coercion function that leave the new maximum
- * length >= the previous maximum length.  We ignore the isExplicit argument,
- * which only affects truncation.
+ * Flatten calls to varbit's length coercion function that set the new maximum
+ * length >= the previous maximum length.  We can ignore the isExplicit
+ * argument, since that only affects truncation cases.
  */
 Datum
 varbit_transform(PG_FUNCTION_ARGS)
 {
 	FuncExpr   *expr = (FuncExpr *) PG_GETARG_POINTER(0);
-	Node	   *typmod;
 	Node	   *ret = NULL;
+	Node	   *typmod;
 
-	if (!IsA(expr, FuncExpr))
-		PG_RETURN_POINTER(ret);
+	Assert(IsA(expr, FuncExpr));
+	Assert(list_length(expr->args) >= 2);
 
-	Assert(list_length(expr->args) == 3);
-	typmod = lsecond(expr->args);
+	typmod = (Node *) lsecond(expr->args);
 
-	if (IsA(typmod, Const))
+	if (IsA(typmod, Const) && !((Const *) typmod)->constisnull)
 	{
-		Node	   *source = linitial(expr->args);
+		Node	   *source = (Node *) linitial(expr->args);
 		int32		new_typmod = DatumGetInt32(((Const *) typmod)->constvalue);
 		int32		old_max = exprTypmod(source);
 		int32		new_max = new_typmod;
 
-		if (new_max <= 0 || (old_max >= 0 && old_max <= new_max))
+		/* Note: varbit() treats typmod 0 as invalid, so we do too */
+		if (new_max <= 0 || (old_max > 0 && old_max <= new_max))
 			ret = relabel_to_typmod(source, new_typmod);
 	}
 

src/backend/utils/adt/varchar.c

@@ -19,7 +19,6 @@
 #include "access/tuptoaster.h"
 #include "libpq/pqformat.h"
 #include "nodes/nodeFuncs.h"
-#include "parser/parse_clause.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "mb/pg_wchar.h"
@@ -551,31 +550,32 @@ varcharsend(PG_FUNCTION_ARGS)
 
 
 /*
- * Flatten calls to our length coercion function that leave the new maximum
- * length >= the previous maximum length.  We ignore the isExplicit argument,
- * which only affects truncation.
+ * varchar_transform()
+ * Flatten calls to varchar's length coercion function that set the new maximum
+ * length >= the previous maximum length.  We can ignore the isExplicit
+ * argument, since that only affects truncation cases.
  */
 Datum
 varchar_transform(PG_FUNCTION_ARGS)
 {
 	FuncExpr   *expr = (FuncExpr *) PG_GETARG_POINTER(0);
-	Node	   *typmod;
 	Node	   *ret = NULL;
+	Node	   *typmod;
 
-	if (!IsA(expr, FuncExpr))
-		PG_RETURN_POINTER(ret);
+	Assert(IsA(expr, FuncExpr));
+	Assert(list_length(expr->args) >= 2);
 
-	Assert(list_length(expr->args) == 3);
-	typmod = lsecond(expr->args);
+	typmod = (Node *) lsecond(expr->args);
 
-	if (IsA(typmod, Const))
+	if (IsA(typmod, Const) && !((Const *) typmod)->constisnull)
 	{
-		Node	   *source = linitial(expr->args);
+		Node	   *source = (Node *) linitial(expr->args);
+		int32		old_typmod = exprTypmod(source);
 		int32		new_typmod = DatumGetInt32(((Const *) typmod)->constvalue);
-		int32		old_max = exprTypmod(source) - VARHDRSZ;
+		int32		old_max = old_typmod - VARHDRSZ;
 		int32		new_max = new_typmod - VARHDRSZ;
 
-		if (new_max < 0 || (old_max >= 0 && old_max <= new_max))
+		if (new_typmod < 0 || (old_typmod >= 0 && old_max <= new_max))
 			ret = relabel_to_typmod(source, new_typmod);
 	}