sql: change volatility of timezone with TIME and TIMETZ to stable

The `timezone(STRING, TIME)` and `timezone(STRING, TIMETZ)` function
overloads were originally marked as volatile to copy Postgres's
volatility for the `timezone(STRING, TIMETZ)` overload (Postgres does
not implement `timezone (STRING, TIME)`.

Postgres's volatility settings for all overloads of `timezone` are
below.

       Name   |          Argument data types          | Volatility
    ----------+---------------------------------------+------------------
     timezone | interval, time with time zone         | immutable   zone
     timezone | interval, timestamp with time zone    | immutable
     timezone | interval, timestamp without time zone | immutable
     timezone | text, time with time zone             | volatile    zone
     timezone | text, timestamp with time zone        | immutable
     timezone | text, timestamp without time zone     | immutable

This single overload is singled out as volatile by Postgres because it
is dependent on C's `time(NULL)`. See the Postgres commit originally
marking this overload volatility, and the lines of code showing the
dependence:

postgres/postgres@35979e6
https://github.com/postgres/postgres/blob/ac3a4866c0bf1d7a14009f18d3b42ffcb063a7e9/src/backend/utils/adt/date.c#L2816

CRDB does not have the same issue. All `timezone` overloads have the
same volatility, stable. This commit marks them as such.

Additional context: #48756 (comment)

This commit also moves the `IgnoreVolatilityCheck` field from function
definitions to overloads. This allows overloads of the same function to
be ignored when asserting that the volalitity of an overload matches
Postgres's assigned volatility.

Release note: None

pkg/sql/sem/builtins/all_builtins_test.go

@@ -140,10 +140,10 @@ func TestOverloadsVolatilityMatchesPostgres(t *testing.T) {
 
 	// Check each builtin against Postgres.
 	for name, builtin := range builtins {
-		if builtin.props.IgnoreVolatilityCheck {
-			continue
-		}
 		for idx, overload := range builtin.overloads {
+			if overload.IgnoreVolatilityCheck {
+				continue
+			}
 			postgresVolatility, found := findOverloadVolatility(name, overload)
 			if !found {
 				continue

pkg/sql/sem/builtins/builtins.go

@@ -249,11 +249,6 @@ var builtins = map[string]builtinDefinition{
 	"concat": makeBuiltin(
 		tree.FunctionProperties{
 			NullableArgs: true,
-			// In Postgres concat can take any arguments, converting them to their
-			// text representation. Since the text representation can depend on the
-			// context (e.g. timezone), the function is Stable. In our case, we only
-			// take String inputs so our version is Immutable.
-			IgnoreVolatilityCheck: true,
 		},
 		tree.Overload{
 			Types:      tree.VariadicType{VarType: types.String},
@@ -275,17 +270,17 @@ var builtins = map[string]builtinDefinition{
 			},
 			Info:       "Concatenates a comma-separated list of strings.",
 			Volatility: tree.VolatilityImmutable,
+			// In Postgres concat can take any arguments, converting them to
+			// their text representation. Since the text representation can
+			// depend on the context (e.g. timezone), the function is Stable. In
+			// our case, we only take String inputs so our version is Immutable.
+			IgnoreVolatilityCheck: true,
 		},
 	),
 
 	"concat_ws": makeBuiltin(
 		tree.FunctionProperties{
 			NullableArgs: true,
-			// In Postgres concat_ws can take any arguments, converting them to their
-			// text representation. Since the text representation can depend on the
-			// context (e.g. timezone), the function is Stable. In our case, we only
-			// take String inputs so our version is Immutable.
-			IgnoreVolatilityCheck: true,
 		},
 		tree.Overload{
 			Types:      tree.VariadicType{VarType: types.String},
@@ -321,11 +316,16 @@ var builtins = map[string]builtinDefinition{
 				"subsequent arguments. \n\nFor example `concat_ws('!','wow','great')` " +
 				"returns `wow!great`.",
 			Volatility: tree.VolatilityImmutable,
+			// In Postgres concat_ws can take any arguments, converting them to
+			// their text representation. Since the text representation can
+			// depend on the context (e.g. timezone), the function is Stable. In
+			// our case, we only take String inputs so our version is Immutable.
+			IgnoreVolatilityCheck: true,
 		},
 	),
 
 	// https://www.postgresql.org/docs/10/static/functions-string.html#FUNCTIONS-STRING-OTHER
-	"convert_from": makeBuiltin(tree.FunctionProperties{Category: categoryString, IgnoreVolatilityCheck: true},
+	"convert_from": makeBuiltin(tree.FunctionProperties{Category: categoryString},
 		tree.Overload{
 			Types:      tree.ArgTypes{{"str", types.Bytes}, {"enc", types.String}},
 			ReturnType: tree.FixedReturnType(types.String),
@@ -353,11 +353,12 @@ var builtins = map[string]builtinDefinition{
 			},
 			Info: "Decode the bytes in `str` into a string using encoding `enc`. " +
 				"Supports encodings 'UTF8' and 'LATIN1'.",
-			Volatility: tree.VolatilityImmutable,
+			Volatility:            tree.VolatilityImmutable,
+			IgnoreVolatilityCheck: true,
 		}),
 
 	// https://www.postgresql.org/docs/10/static/functions-string.html#FUNCTIONS-STRING-OTHER
-	"convert_to": makeBuiltin(tree.FunctionProperties{Category: categoryString, IgnoreVolatilityCheck: true},
+	"convert_to": makeBuiltin(tree.FunctionProperties{Category: categoryString},
 		tree.Overload{
 			Types:      tree.ArgTypes{{"str", types.String}, {"enc", types.String}},
 			ReturnType: tree.FixedReturnType(types.Bytes),
@@ -385,7 +386,8 @@ var builtins = map[string]builtinDefinition{
 			},
 			Info: "Encode the string `str` as a byte array using encoding `enc`. " +
 				"Supports encodings 'UTF8' and 'LATIN1'.",
-			Volatility: tree.VolatilityImmutable,
+			Volatility:            tree.VolatilityImmutable,
+			IgnoreVolatilityCheck: true,
 		}),
 
 	// https://www.postgresql.org/docs/9.0/functions-binarystring.html#FUNCTIONS-BINARYSTRING-OTHER
@@ -2498,10 +2500,11 @@ may increase either contention or retry errors, or both.`,
 				durationDelta := time.Duration(-beforeOffsetSecs) * time.Second
 				return tree.NewDTimeTZ(timetz.MakeTimeTZFromTime(tTime.In(loc).Add(durationDelta))), nil
 			},
-			Info: "Treat given time without time zone as located in the specified time zone.",
-			// TODO(mgartner): This overload might be stable, not volatile.
-			// See: https://github.com/cockroachdb/cockroach/pull/48756#issuecomment-627672686
-			Volatility: tree.VolatilityVolatile,
+			Info:       "Treat given time without time zone as located in the specified time zone.",
+			Volatility: tree.VolatilityStable,
+			// This overload's volatility does not match Postgres. See
+			// https://github.com/cockroachdb/cockroach/pull/51037 for details.
+			IgnoreVolatilityCheck: true,
 		},
 		tree.Overload{
 			Types: tree.ArgTypes{
@@ -2523,10 +2526,11 @@ may increase either contention or retry errors, or both.`,
 				tTime := tArg.TimeTZ.ToTime()
 				return tree.NewDTimeTZ(timetz.MakeTimeTZFromTime(tTime.In(loc))), nil
 			},
-			Info: "Convert given time with time zone to the new time zone.",
-			// TODO(mgartner): This overload might be stable, not volatile.
-			// See: https://github.com/cockroachdb/cockroach/pull/48756#issuecomment-627672686
-			Volatility: tree.VolatilityVolatile,
+			Info:       "Convert given time with time zone to the new time zone.",
+			Volatility: tree.VolatilityStable,
+			// This overload's volatility does not match Postgres. See
+			// https://github.com/cockroachdb/cockroach/pull/51037 for details.
+			IgnoreVolatilityCheck: true,
 		},
 	),
 

pkg/sql/sem/builtins/pg_builtins.go

@@ -138,9 +138,6 @@ func makeTypeIOBuiltin(argTypes tree.TypeList, returnType *types.T) builtinDefin
 	return builtinDefinition{
 		props: tree.FunctionProperties{
 			Category: categoryCompatibility,
-			// Ignore validity checks for typeio builtins.
-			// We don't implement these anyway, and they are very hard to special case.
-			IgnoreVolatilityCheck: true,
 		},
 		overloads: []tree.Overload{
 			{
@@ -151,6 +148,10 @@ func makeTypeIOBuiltin(argTypes tree.TypeList, returnType *types.T) builtinDefin
 				},
 				Info:       notUsableInfo,
 				Volatility: tree.VolatilityVolatile,
+				// Ignore validity checks for typeio builtins. We don't
+				// implement these anyway, and they are very hard to special
+				// case.
+				IgnoreVolatilityCheck: true,
 			},
 		},
 	}

pkg/sql/sem/tree/function_definition.go

@@ -84,11 +84,6 @@ type FunctionProperties struct {
 	// with the FmtParsable directive.
 	AmbiguousReturnType bool
 
-	// IgnoreVolatilityCheck ignores checking the functions overloads against
-	// the Postgres ones at test time.
-	// This should be used with caution.
-	IgnoreVolatilityCheck bool
-
 	// HasSequenceArguments is true if the builtin function takes in a sequence
 	// name (string) and can be used in a scalar expression.
 	// TODO(richardjcai): When implicit casting is supported, these builtins

pkg/sql/sem/tree/overload.go

@@ -80,6 +80,11 @@ type Overload struct {
 	// SpecializedVecBuiltin is used to let the vectorized engine
 	// know when an Overload has a specialized vectorized operator.
 	SpecializedVecBuiltin SpecializedVectorizedBuiltin
+
+	// IgnoreVolatilityCheck ignores checking the functions overload's
+	// volatility against Postgres's volatility at test time.
+	// This should be used with caution.
+	IgnoreVolatilityCheck bool
 }
 
 // params implements the overloadImpl interface.