sql: implement CREATE FUNCTION in legacy schema changer.

[chengxiong-ruan]

Release note (sql change): This commit implements the CREATE FUNCTION
statement in legacy schema changer. It allows user to create a
function descriptor in data store with all cross references properly
tracked, but function descriptor cannot be used yet. Follwing commits
will add more components to fullfill the user defined function
feature.

[cockroach-teamcity]

This change is 

[chengxiong-ruan]

I only manually tested by checking descriptor jsons for attributes and cross references, didn't add tests yet.
But want to get a quick first pass to get feedbacks.

[chengxiong-ruan]

I added tests for

1. cross reference validation + basic function descriptor validation
2. some logic tests for create function, but can't do show function ATM, so just printing out jsons
3. tests to the schema changer to verify cross references fields in function, tables and type.

[chengxiong-ruan]

I'm sorry that this PR is touching many files. But most of them are pretty much small fixes here and there.
The main functionality changes lies in a few files for:
(1) optbuilder support for create function statement
(2) legacy schema changer support creating new function descriptor
(3) basic function descriptor validation + cross validation for functions, table, types, schemas
(4) adding EXECUTE privilege and new function privilege type.
Let me know if there's strong need to break this into multiple commits.

[mgartner]

Reviewed 9 of 26 files at r19, 2 of 57 files at r22, 3 of 44 files at r25, 2 of 26 files at r26, 1 of 16 files at r27, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @chengxiong-ruan, and @mgartner)

[ajwerner]

Still need to make it through the last two commits. Looking good. Flushing out my buffered comments.

Reviewed 14 of 111 files at r1, 1 of 42 files at r4, 1 of 123 files at r7, 36 of 42 files at r8, 2 of 40 files at r11, 3 of 83 files at r13, 10 of 16 files at r14, 1 of 17 files at r17, 6 of 73 files at r18, 10 of 26 files at r19, 12 of 57 files at r22, 2 of 18 files at r24, 43 of 44 files at r25, 4 of 26 files at r26.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @chengxiong-ruan, and @mgartner)

---

pkg/sql/create_function.go line 54 at r26 (raw file):

func (n *createFunctionNode) startExec(params runParams) error {
	if n.routineBody != nil {
		return unimplemented.New("create function sql_body", "CREATE FUNCTION...sql_body unimplemented")

should this be linked to a tracking issue?

---

pkg/sql/catalog/descriptor.go line 861 at r25 (raw file):

	GetDependsOnTypes() []descpb.ID

	GetDependedOnBy() []descpb.FunctionDescriptor_Reference

nit: needs a comment

---

pkg/sql/catalog/descs/function.go line 53 at r25 (raw file):

	}

	// TODO (Chengxiong): #83232 UDF hydration for user defined types.

nit: usually there is no space here and it's your github handle.

---

pkg/sql/catalog/descs/function.go line 53 at r26 (raw file):

	}

	// TODO (Chengxiong): #83232 UDF hydration for user defined types.

Should we prevent UDF type references in creation until this is fixed?

---

pkg/sql/catalog/funcdesc/func_desc.go line 47 at r25 (raw file):

type Mutable struct {
	immutable
	ClusterVersion *immutable

exporting the field with an unexported type can be awkward to use. Consider making ClusterVersion a method which returns catalog.FunctionDescriptor

---

pkg/sql/catalog/nstree/map.go line 27 at r25 (raw file):

	byID   byIDMap
	byName byNameMap
	// nameSkipped record the ids of items upsert by skipping the name map.

I've been thinking about this and I wonder if the right thing to do is not to skip the name map but rather to have a NameKind enum and make it part of the NameEntry interface such that we have two name maps, one for functions and one for types+relations. This can be deferred. What you have is fine for now.

---

pkg/sql/catalog/nstree/map.go line 52 at r25 (raw file):

			dt.byName.delete(replaced)
		}
		//delete(dt.nameSkipped, d.GetID())

detritus

---

pkg/sql/schemachanger/scdecomp/decomp.go line 107 at r25 (raw file):

	case catalog.FunctionDescriptor:
		// TODO (Chengxiong) #83235 implement DROP FUNCTION
		panic(unimplemented.NewWithIssue(

I think this should be the scerrors unimplemented error

[ajwerner]

Alright, I made it through. My comments seem mostly like nits. Feel free to merge after your next revision.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @chengxiong-ruan, and @mgartner)

---

pkg/sql/descriptor.go line 250 at r27 (raw file):

	descID := descriptor.GetID()

	if !descriptor.SkipNamespace() {

One observation is that you're also passing in a zero-value ID key. Can we do some defensive checking to make sure that if the ID key is non-zero this is false and if it is zero that this is true?

---

pkg/sql/create_function_test.go line 178 at r27 (raw file):

	_, err = sqlDB.Exec("ALTER TABLE t ALTER COLUMN b TYPE STRING")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

nit: refactor this to be table-driven where you have a slice literal with entries for the statements and expected errors. I think it'll read better.

Code quote:

	_, err = sqlDB.Exec("DROP SEQUENCE sq1")
	require.Equal(t, "pq: cannot drop sequence sq1 because other objects depend on it", err.Error())

	_, err = sqlDB.Exec("DROP SEQUENCE sq1 CASCADE")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

	_, err = sqlDB.Exec("ALTER SEQUENCE sq1 RENAME TO sq1_new")
	require.NoError(t, err)

	_, err = sqlDB.Exec("DROP TABLE t")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

	_, err = sqlDB.Exec("DROP TABLE t CASCADE")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

	_, err = sqlDB.Exec("ALTER TABLE t RENAME TO t_new")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

	_, err = sqlDB.Exec("ALTER TABLE t SET SCHEMA test_sc")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

	_, err = sqlDB.Exec("ALTER TABLE t DROP COLUMN b")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

	_, err = sqlDB.Exec("ALTER TABLE t DROP COLUMN b CASCADE")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

	_, err = sqlDB.Exec("ALTER TABLE t RENAME COLUMN b TO bb")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

	_, err = sqlDB.Exec("ALTER TABLE t ALTER COLUMN b TYPE STRING")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

	_, err = sqlDB.Exec("DROP INDEX t@t_idx_b")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

	_, err = sqlDB.Exec("DROP INDEX t@t_idx_b CASCADE")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

	_, err = sqlDB.Exec("ALTER INDEX t@t_idx_b RENAME TO t_idx_b_new")
	require.Equal(t, "pq: unimplemented: drop function not supported", err.Error())

[chengxiong-ruan]

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

---

pkg/sql/create_function.go line 54 at r26 (raw file):

Previously, ajwerner wrote…

should this be linked to a tracking issue?

yeah, good catch, filed one issue for it.

---

pkg/sql/descriptor.go line 250 at r27 (raw file):

Previously, ajwerner wrote…

One observation is that you're also passing in a zero-value ID key. Can we do some defensive checking to make sure that if the ID key is non-zero this is false and if it is zero that this is true?

good call, added checks.

---

pkg/sql/catalog/descriptor.go line 861 at r25 (raw file):

Previously, ajwerner wrote…

nit: needs a comment

done.

---

pkg/sql/catalog/descs/function.go line 53 at r25 (raw file):

Previously, ajwerner wrote…

nit: usually there is no space here and it's your github handle.

fixed

---

pkg/sql/catalog/descs/function.go line 53 at r26 (raw file):

Previously, ajwerner wrote…

Should we prevent UDF type references in creation until this is fixed?

good question. since the hydration is coming soon...I'd leave it as it is.

---

pkg/sql/catalog/funcdesc/func_desc.go line 47 at r25 (raw file):

Previously, ajwerner wrote…

exporting the field with an unexported type can be awkward to use. Consider making ClusterVersion a method which returns catalog.FunctionDescriptor

agree, changed it to be unexported. It's only used to check original version internally.

---

pkg/sql/catalog/nstree/map.go line 52 at r25 (raw file):

Previously, ajwerner wrote…

detritus

this is actually nice to have...I uncommented it.

---

pkg/sql/opt/memo/interner.go line 766 at r15 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I don't love using reflection for this case. Can you leave a TODO for me and I'll fix this up: TODO(mgartner): create a field for each option in CreateFunctionExpr.

Added a TODO to myself. After more thinking... I feel more of your point.

---

pkg/sql/opt/ops/statement.opt line 78 at r15 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Good point, it's a function option. But you've made it its own field here too. Either way, I can revisit this and make all the options their own field. Can you leave a TODO for me?

Added a todo to myself as well :)

---

pkg/sql/opt/optbuilder/create_function.go line 173 at r15 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

There is pgerror.Wrapf. Example usage:

cockroach/pkg/sql/sem/tree/type_check.go

Line 487 in b29f48b

err = pgerror.Wrapf(err, pgcode.InvalidParameterValue, "%s::%s", castFrom, castTo)

oh I knew of that one, I didn't want to have an extra error message.. just looked at the implementation of pgerror.Wrapf it's using pgerror.WithCandidateCode actually heh. I'm keeping it as it is if it doesn't bother you too much.

---

pkg/sql/opt/optgen/cmd/optgen/metadata.go line 255 at r15 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

What function input list are you referring to? I don't think that using existing types would change the implementation all that much. Feel free to leave this as-is though and I can take another look - maybe leave a TODO for me.

Added a todo for myself :)

---

pkg/sql/schemachanger/scdecomp/decomp.go line 107 at r25 (raw file):

Previously, ajwerner wrote…

I think this should be the scerrors unimplemented error

oh, I wanted to have the issue number, keeping it as it is.

---

pkg/sql/create_function_test.go line 178 at r27 (raw file):

Previously, ajwerner wrote…

nit: refactor this to be table-driven where you have a slice literal with entries for the statements and expected errors. I think it'll read better.

ah yeah, fixed!

[chengxiong-ruan]

TFTRs!
bors+

[chengxiong-ruan]

bors r+

[chengxiong-ruan]

bors -

[craig]

Did you mean "r-"?

[chengxiong-ruan]

bors r-

[craig]

Canceled.

[chengxiong-ruan]

conflicts resolved
bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed:

• GitHub CI (Cockroach)

[chengxiong-ruan]

looks like a flake...
bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)