sql: mark functions as Immutable/Stable/Volatile

[otan]

This commit adds the "Volatile" argument to all function properties,
replacing the "Impure" definition. I've replaced all "Impure" checks
with "!= VolatilityImmutable` to give equivalent functionality for now -
we can enhance the folding and the like in a later commit. The planner
tweaks can come from someone more familiar with it :).

Also updated the pg_proc table, which should be the only visible user
change.

Refs #26582.

Release note (sql change): Populated the pg_proc table's provolatile
field based on the internal builtin volatility definition. This value
used to always be NULL.

[cockroach-teamcity]

This change is 

[otan]

(let me know if this should be "touches" instead)

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 198edc0f.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 695575e2.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[maddyblue]

I would like a test added that asserts that all for all builtins with fnProps defined, the volatility is not the zero value.

[otan]

I've made the builtins be defined as immutable (like we do today as impure=false) if not filled in on init time. Do you want that to be different?

[otan]

(would you rather make fnProps fill in the volatility by default, and then anyone who defined FunctionProperties manually must define their own?)

[maddyblue]

Can you add something like: TODO: instead of the default being immutable (which could be a problem if a new function forgets to set its volatility if not immutable), explicitly define volatility of all functions and add a test that asserts it is never the zero value.

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 3826bba5.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[RaduBerinde]

I've made the builtins be defined as immutable (like we do today as impure=false) if not filled in on init time. Do you want that to be different?

Unfortunately, what we currently do is the worst thing we can do.. If someone adds a builtin without thinking about volatility or even knowing much about it, we should be conservative with the default. I do prefer that we require it explicitly everywhere, and maybe just document that "if in doubt, use volatile". I realize this is a very tedious change :/

Note that because of this unsafe default, we can't completely trust this flag today and the optimizer has its own "whitelist" of functions here: https://github.com/cockroachdb/cockroach/blob/2f08e8ea772d4619b66182020cb9453ad341fcbb/pkg/sql/opt/norm/fold_constants.go#L418
This would need to be resolved before closing the issue.

[otan]

alright, we can make that change separately. I'll put this in as a first step.

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 6eeb0c53.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on b2b926eb.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[maddyblue]

Is this ready to go now?

[otan]

yep!

i'm planning on auditing everything for my flex friday, and doing the overload approach (unless you had something in mind). may be some more refactors over the same file, but whatever :P

bors r=mjibson

[maddyblue]

Thanks, that sounds great.

[RaduBerinde]

Argh..... I had some comments but apparently never hit Publish.. sorry about that. Can be addressed in a follow-up PR

Nice, thanks for working on this! CC @rytaft @andy-kimball

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mjibson, @otan, and @RaduBerinde)

---

pkg/sql/sem/tree/function_definition.go, line 43 at r1 (raw file):

type Volatility int

const (

We need to document these as clearly as possible because this will be consulted any time someone adds or messes with a builtin, and it will be used when figuring out the correct logic in the optimizer. Better to be extra verbose - we should mention exactly what we can do with some examples. A while back I had thought of what semantics we would want for this property on the optimizer side, I wrote this comment: https://github.com/RaduBerinde/cockroach/blob/c11ad89fe595f9d378e234691bbd1f7d42eab488/pkg/sql/opt/props/volatility.go

We can lift some descriptions / comments from there. We should consider separating "volatile" from "mutation" (or "side-effecting") at the function level.

---

pkg/sql/sem/tree/function_definition.go, line 45 at r1 (raw file):

const (
	// VolatilityImmutable indicates the builtin result never changes for a
	// given input.

It does not depend on configuration settings, and does not modify the database, the transaction state, or any other state. Immutable functions can always be constant-folded (even in cached plans).

---

pkg/sql/sem/tree/function_definition.go, line 48 at r1 (raw file):

	VolatilityImmutable Volatility = iota
	// VolatilityStable indicates the builtin result never changes during
	// a given scan.

"during a given scan" is not very helpful. Maybe change to "the function cannot modify the database or the transaction state and is guaranteed to return the same results given the same arguments whenever it is evaluated within the same statement."

---

pkg/sql/sem/tree/function_definition.go, line 50 at r1 (raw file):

	// a given scan.
	VolatilityStable
	// VolatilityVolatile indicates the builtin result can change even

mention "or that it has side effects". I would remove the "impure" thing because it doesn't help here without a proper definition; we can add some examples of different things the function can do.

---

pkg/sql/sqlbase/table.go, line 34 at r2 (raw file):

// type and contains no variable expressions. It returns the type-checked and
// constant-folded expression.
func SanitizeVarFreeExpr(

[nit] I would break this out into SanitizeVarFreeExpr and SanitizeImmutableExpr. They can call out to the same unexported function (maybe passing the flags directly)

[craig]

Build succeeded

• GitHub CI (Cockroach)

[andy-kimball]

The "given scan" part confuses me. I was expecting something like:

Immutable: never changes for a given input, even across multiple executions of a statement
Stable: never changes for a given input when invoked multiple times in a single statement, but may change across multiple executions of a statement
Volatile: may change when invoked multiple times in a single statement

Examples of each case would be helpful as well, to give people a quick intuition:

Immutable: length(...)
Stable: now()
Volatile: random()

[otan]

I'll fix this up tomorrow! I have 1000 functions to audit :D