tree: move telemetry references to sql package

[otan]

Individual commits should be fairly easy to review by themselves!

There are still indirect references as of writing, but this removes the direct ones!

[cockroach-teamcity]

This change is 

[otan]

hmm, the last commit causes

fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x2 addr=0x70eaa78 pc=0x102dab7f8]

goroutine 2895 [running]:
runtime.throw({0x105fb7b77, 0x2a})
	/opt/homebrew/opt/[email protected]/libexec/src/runtime/panic.go:1198 +0x54 fp=0x14004da7150 sp=0x14004da7120 pc=0x102dcf964
runtime.sigpanic()
	/opt/homebrew/opt/[email protected]/libexec/src/runtime/signal_unix.go:719 +0x230 fp=0x14004da7190 sp=0x14004da7150 pc=0x102de84c0
runtime.heapBitsSetType(0x14004f6a8f0, 0xd0, 0xc8, 0x107aee6c0)
	/opt/homebrew/opt/[email protected]/libexec/src/runtime/mbitmap.go:1119 +0x608 fp=0x14004da7260 sp=0x14004da71a0 pc=0x102dab7f8
runtime.mallocgc(0xc8, 0x107aee6c0, 0x1)
	/opt/homebrew/opt/[email protected]/libexec/src/runtime/malloc.go:1106 +0x654 fp=0x14004da72f0 sp=0x14004da7260 pc=0x102da3074
runtime.newobject(0x107aee6c0)
	/opt/homebrew/opt/[email protected]/libexec/src/runtime/malloc.go:1234 +0x38 fp=0x14004da7320 sp=0x14004da72f0 pc=0x102da3488
github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scdeps.NewJobRunDependencies(...)
	/Users/otan/go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scdeps/run_deps.go:49
github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scjob.(*newSchemaChangeResumer).run(0x14004e4b310, {0x107c72858, 0x140053dbe90}, {0x1079c4660, 0x1400078a148})
	/Users/otan/go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scjob/job.go:66 +0x2a4 fp=0x14004da7620 sp=0x14004da7320 pc=0x105c63c84
github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scjob.(*newSchemaChangeResumer).Resume(0x14004e4b310, {0x107c72858, 0x140053dbe90}, {0x1079c4660, 0x1400078a148})
	/Users/otan/go/src/github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scjob/job.go:42 +0x48 fp=0x14004da7670 sp=0x14004da7620 pc=0x105c63938
github.com/cockroachdb/cockroach/pkg/jobs.(*Registry).stepThroughStateMachine.func2(0x140013be580, 0x14001990500, {0x107c02688, 0x14004e4b310}, {0x107c72858, 0x140053dbe90}, {0x1079c4660, 0x1400078a148}, 0x14004da78c0)
	/Users/otan/go/src/github.com/cockroachdb/cockroach/pkg/jobs/registry.go:1168 +0xec fp=0x14004da76e0 sp=0x14004da7670 pc=0x1045b182c
github.com/cockroachdb/cockroach/pkg/jobs.(*Registry).stepThroughStateMachine(0x14001990500, {0x107c72858, 0x14003950780}, {0x1079c4660, 0x1400078a148}, {0x107c02688, 0x14004e4b310}, 0x140014ac680, {0x14004e4f130, 0x7}, ...)
	/Users/otan/go/src/github.com/cockroachdb/cockroach/pkg/jobs/registry.go:1169 +0x670 fp=0x14004da7cb0 sp=0x14004da76e0 pc=0x1045b00d0
github.com/cockroachdb/cockroach/pkg/jobs.(*Registry).runJob(0x14001990500, {0x107c727b0, 0x14000f7fd80}, {0x107c02688, 0x14004e4b310}, 0x140014ac680, {0x14004e4f130, 0x7}, {0x14002c5a330, 0x16})
	/Users/otan/go/src/github.com/cockroachdb/cockroach/pkg/jobs/adopt.go:408 +0x554 fp=0x14004da7e60 sp=0x14004da7cb0 pc=0x10459b914
github.com/cockroachdb/cockroach/pkg/jobs.(*Registry).resumeJob.func1({0x107c72858, 0x140039504b0})
	/Users/otan/go/src/github.com/cockroachdb/cockroach/pkg/jobs/adopt.go:329 +0x100 fp=0x14004da7f20 sp=0x14004da7e60 pc=0x10459b1a0
github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunAsyncTaskEx.func2(0x140013b3290, {0x107c72858, 0x140039504b0}, 0x14001262900, 0x0, 0x14004e6b720)
	/Users/otan/go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:494 +0xf8 fp=0x14004da7fa0 sp=0x14004da7f20 pc=0x1039d5278
runtime.goexit()
	/opt/homebrew/opt/[email protected]/libexec/src/runtime/asm_arm64.s:1133 +0x4 fp=0x14004da7fa0 sp=0x14004da7fa0 pc=0x102e06a74
created by github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunAsyncTaskEx
	/Users/otan/go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:483 +0x384

when i run logic tests locally (make testbaselogic FILES='geospatial'), which is confusing...

locally but not on CI though o_o"

[ajwerner]

Can I be annoying? How would you feel about factoring this out even more so that each of these call sites can just pass in the tree node? We're going to want to record the same telemetry in the declarative schema changer. It'll be easier to factor that out if there's a centralized entry point. I think it'll also make adding future telemetry easier.

Reviewed 2 of 2 files at r1, 4 of 4 files at r2, 2 of 2 files at r3, 2 of 4 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

[ajwerner]

Of all the PRs, this one makes me feel the most gross.

Reviewed 2 of 4 files at r4, 4 of 4 files at r5, 4 of 4 files at r6, 1 of 5 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @otan)

---

pkg/sql/sem/tree/alter_table.go line 146 at r5 (raw file):

//
//     ALTER TABLE t ADD COLUMN a INT CHECK (a < b)
func (node *AlterTable) HoistAddColumnConstraints(onHoistedFKConstraint func()) {

:/ this definitely does not feel good

[otan]

Can you give an example of usage of what you mean by what you just said?

[ajwerner]

Instaed of each of these:

telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra(
		tree.GetTableType(n.n.IsSequence, n.n.IsView, n.n.IsMaterialized),
		n.n.TelemetryName(),
	))

	telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra("type", n.n.Cmd.TelemetryName()))

	telemetry.Inc(sqltelemetry.SchemaChangeDropCounter(
		tree.GetTableType(false /* isSequence */, true /* isView */, n.n.IsMaterialized),
	))

Have:

func incrementTelemetryCounter(n tree.Statement) {
   switch n := n.(type) {
   case *tree.DropView:
     	telemetry.Inc(sqltelemetry.SchemaChangeDropCounter(
		tree.GetTableType(false /* isSequence */, true /* isView */, n.n.IsMaterialized),
	))
    case *tree.AlterType:
	telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra("type", n.n.Cmd.TelemetryName()))
    case *tree.AlterTableSetSchema:
	telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra(
		tree.GetTableType(n.n.IsSequence, n.n.IsView, n.n.IsMaterialized),
		n.n.TelemetryName(),
	))
   }
}

[ajwerner]

Maybe you should just wave me off

[otan]

func incrementTelemetryCounter(n tree.Statement)

D: i'm a bigger fan of this way as its statically coded that the TelemetryName exists. but if you feel super strong about it, i guess i can do that.

(edit: currently i have decided not to)

[ajwerner]

Works for me.

[otan]

bors r=ajwerner

thanks!

[craig]

Build succeeded:

• GitHub CI (Cockroach)