sql: mark timezone(string, string) as VolatilityStable

[mgartner]

The timezone(string, string) function overload was incorrectly marked
as VolatilityImmutable. When the second argument a special string,
like 'now', 'today', 'tomorrow', or 'yesterday', the function is
non-immutable because those strings are cast to timestamps which are
dependent on the current clock time.

This commit correctly marks it as VolatilityStable.

Release note: None

[cockroach-teamcity]

This change is 

[maddyblue]

Should some of the overloads marked volatile be bumped up to stable?

[otan]

i think it should be immutable. the timezone function should not be affected by any external factors.

also subverts postgres expectations:

otan=# select proname, proargtypes, provolatile from pg_proc where proname = 'timezone';
 proname  | proargtypes | provolatile
----------+-------------+-------------
 timezone | 1186 1184   | i
 timezone | 25 1184     | i
 timezone | 25 1266     | v -- this seems incorrect.
 timezone | 1186 1266   | i
 timezone | 25 1114     | i
 timezone | 1186 1114   | i
(6 rows)

otan=# select * from typname where oid=1266;
ERROR:  relation "typname" does not exist
LINE 1: select * from typname where oid=1266;
                      ^
otan=# select * from pg_type where oid=1266;
otan=# select typname from pg_type where oid=1266;
 typname
---------
 timetz
(1 row)

[otan]

huh, my mistake - looks like we accept arguments like 'now', which i missed in the PR description. maybe postgres has this wrong. we should probably bump everything up and add a whitelist for allowed postgres differences in the builtins_test which ensure volatility matches postgres..

[RaduBerinde]

@otan the overload we are talking about takes two string arguments. Based on your query above, Postgres doesn't have that.

I am not 100% sure why we have it but I am guessing that we need this overload because we don't do the implicit casts that postgres does. It's the string->timestamp cast itself that is non-immutable.

[RaduBerinde]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)

---

pkg/sql/sem/builtins/builtins.go, line 2419 at r1 (raw file):

			},
			Info:       "Convert given time stamp with time zone to the new time zone, with no time zone designation.",
			Volatility: tree.VolatilityStable,

It's worth adding a comment - mention that parsing a string timestamp is not immutable in general because of things like 'now'.

[mgartner]

Should some of the overloads marked volatile be bumped up to stable?

That's a great question. I'm not sure, so I went down a rabbit hole...

Interestingly, we define both a TIME and TIMETZ overload, while Postgres only defines one for TIMETZ that is marked as volatile.

timezone=# \df+ timezone
                                                                                                             List of functions
   Schema   |   Name   |      Result data type       |          Argument data types          | Type | Volatility | Parallel | Owner  | Security | Access privileges | Language |    Source code    |              Description
------------+----------+-----------------------------+---------------------------------------+------+------------+----------+--------+----------+-------------------+----------+-------------------+----------------------------------------
 pg_catalog | timezone | time with time zone         | interval, time with time zone         | func | immutable  | safe     | marcus | invoker  |                   | internal | timetz_izone      | adjust time with time zone to new zone
 pg_catalog | timezone | timestamp without time zone | interval, timestamp with time zone    | func | immutable  | safe     | marcus | invoker  |                   | internal | timestamptz_izone | adjust timestamp to new time zone
 pg_catalog | timezone | timestamp with time zone    | interval, timestamp without time zone | func | immutable  | safe     | marcus | invoker  |                   | internal | timestamp_izone   | adjust timestamp to new time zone
 pg_catalog | timezone | time with time zone         | text, time with time zone             | func | volatile   | safe     | marcus | invoker  |                   | internal | timetz_zone       | adjust time with time zone to new zone
 pg_catalog | timezone | timestamp without time zone | text, timestamp with time zone        | func | immutable  | safe     | marcus | invoker  |                   | internal | timestamptz_zone  | adjust timestamp to new time zone
 pg_catalog | timezone | timestamp with time zone    | text, timestamp without time zone     | func | immutable  | safe     | marcus | invoker  |                   | internal | timestamp_zone    | adjust timestamp to new time zone
(6 rows)

I searched the PG source code to determine why timetz_zone is marked as volatile and not stable. The only thing I found was this commit.

Given its current definition that depends on time(NULL), timetz_zone is certainly no longer immutable, but must indeed be marked volatile. I wonder if it should use the value of now() (that is, transaction start time) so that it could be marked stable. But it's probably not important enough to be worth changing the code for ... indeed, I'm not even going to force an initdb for this catalog change, seeing that we just did one a few hours ago.

It looks like it's marked as volatile because it's dependent on time(NULL), rather than the transaction start time, and therefore (I'm assuming) the return value can change during query execution.

I'm not sure if this distinction applies in the context of CockroachDB. In our case, I don't think now() is even equal to the timestamp at the start of the transaction. So I'm thinking that it's fine to mark these TIME and TIMETZ overloads as stable.

But I'd like input from someone who knows more. @knz I've seen your name come up quite a bit with regard to some of the time-related functions. Do you have any input here?

[mgartner]

@otan I thought it was also incorrect that it's marked volatile in Postgres (and was excited to have found a bug), but it seems corrected, based on the commit and code I found above.

[otan]

@otan the overload we are talking about takes two string arguments. Based on your query above, Postgres doesn't have that.

postgres doesn't have a (string, string) version because of its type resolution system. as the preferred type for the time type category is TIMESTAMPTZ, it resolves the overload (unknown, unknown) as (timestamptz, string). See https://docs.google.com/document/d/1DUUjKtFhnXDbHTmetl5qD2t9AbIi0gOu3fM_E9LwcDo/edit for the type resolution algorithm in more detail.

the reason we have (string, string) is because we don't have preferred types / type categories, and i had to insert that in (it would be ambiguous, and that breaks some usages of AT TIME ZONE). (string, string) should technically be deleted from CRDB when we have the same resolution system.

if we change (string, string) to stable / volatile, we should do the same thing for timestamptz, string and the others.

tl;dr in CRDB (string, string) masquerades as (timestamptz, string) and is there for compatibility with the psql type system. psql seems to define this as immutable - i'm not sure that's right. if we're going down this path, we should change all of them and fix the test to whitelist these deviations.

[RaduBerinde]

tl;dr in CRDB (string, string) masquerades as (timestamptz, string) and is there for compatibility with the psql type system. psql seems to define this as immutable

This is consistent with what I've been saying. The overloads that operate on timestamp inputs are immutable. They always give the same result for the same inputs. What is not immutable is the interpretation of a string as a timestamp. In postgres that has nothing to do with evaluating the timestamp function - by the time this function is evaluated, we already have a timestamp. That is why IMO our timestamp(string, string) variant (and only that) should be marked as "stable" - it is special, because it incorporates the parsing of the string.

[otan]

edit: nvm i'm a bad reader :D carry on

[otan]

sorry for the back and forth, missed a few layers of confusingness in between (missed that the cast happens before the function is evaluated). LGTM

[maddyblue]

It looks like it's marked as volatile because it's dependent on time(NULL), rather than the transaction start time, and therefore (I'm assuming) the return value can change during query execution.

I'm not sure if this distinction applies in the context of CockroachDB. In our case, I don't think now() is even equal to the timestamp at the start of the transaction. So I'm thinking that it's fine to mark these TIME and TIMETZ overloads as stable.

Interesting! Thanks for doing that research. I'm ok with whatever decision we make, but at least now it seems that we could safely mark them as stable, even if we choose not to for compat or other reasons. (And definitely not something that needs to be done in this PR.)

[mgartner]

Interesting! Thanks for doing that research. I'm ok with whatever decision we make, but at least now it seems that we could safely mark them as stable, even if we choose not to for compat or other reasons. (And definitely not something that needs to be done in this PR.)

For now I've left TODOs above each overload so we don't lose this train-of-thought. I'll try to confirm that it's safe to mark them as stable and follow up in a future PR.

[otan]

this should fail

cockroach/pkg/sql/sem/builtins/all_builtins_test.go

Line 143 in 793ac98

postgresVolatility, found := findOverloadVolatility(name, overload)

and you'll need to whitelist this difference from postgres.

[otan]

(setting IgnoreVolatilityCheck: true should do it)

[mgartner]

Good to know. Thanks for the heads-up!

[mgartner]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build failed

• GitHub CI (Cockroach)

[mgartner]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)