From f94915816fdf127d231bb5631a7ab09cec8892cb Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Mon, 6 Jul 2020 17:22:07 -0700 Subject: [PATCH] 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: https://github.com/postgres/postgres/commit/35979e6c35b75b0f6ed68b60f120459380ead3c4 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: https://github.com/cockroachdb/cockroach/pull/48756#issuecomment-627672686 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 | 6 +-- pkg/sql/sem/builtins/builtins.go | 48 ++++++++++++----------- pkg/sql/sem/builtins/pg_builtins.go | 7 ++-- pkg/sql/sem/tree/function_definition.go | 5 --- pkg/sql/sem/tree/overload.go | 5 +++ 5 files changed, 38 insertions(+), 33 deletions(-) diff --git a/pkg/sql/sem/builtins/all_builtins_test.go b/pkg/sql/sem/builtins/all_builtins_test.go index 25d968e2d093..57a3178f55ab 100644 --- a/pkg/sql/sem/builtins/all_builtins_test.go +++ b/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 diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index a01cef2e2eb7..ff47909f9e78 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/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, }, ), diff --git a/pkg/sql/sem/builtins/pg_builtins.go b/pkg/sql/sem/builtins/pg_builtins.go index 8b3307eedb16..527f5f839306 100644 --- a/pkg/sql/sem/builtins/pg_builtins.go +++ b/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, }, }, } diff --git a/pkg/sql/sem/tree/function_definition.go b/pkg/sql/sem/tree/function_definition.go index e715d9d75bb2..2cc0f790453b 100644 --- a/pkg/sql/sem/tree/function_definition.go +++ b/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 diff --git a/pkg/sql/sem/tree/overload.go b/pkg/sql/sem/tree/overload.go index 6a5fb979e931..4718beb4c4ff 100644 --- a/pkg/sql/sem/tree/overload.go +++ b/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.