sql: unit test, benchmark and logictests for COPY

[cucaroach]

As a new/half supported feature COPY FROM STDIN support is under tested.
Add tests and a simple benchmark to pave the way for enhancements and
optimizations.

Assists: #81731

Release note: None

Release justification: non-production code change

[cockroach-teamcity]

This change is 

[yuzefovich]

I'll defer to Oliver on Copy stuff, but the conn executor changes LGTM.

Reviewed 19 of 19 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @otan)

---

pkg/sql/copy.go line 694 at r1 (raw file):

	stmtTs := txnOpt.stmtTimestamp
	autoCommit := false
	// TODO: turn on insertFastPath!

nit: add a name to the TODOs :)

---

pkg/sql/copyshim.go line 1 at r1 (raw file):

// Copyright 2016 The Cockroach Authors.

nit: s/2016/2022/

---

pkg/sql/copyshim.go line 70 at r1 (raw file):

}

// RunCopyFrom exposes copy functionality for the logictest "copy" command, its test

nit: s/test only/test-only/ for readability.

---

pkg/sql/copyshim.go line 92 at r1 (raw file):

	// TODO: test open transaction and implicit txn
	var txnOpt copyTxnOpt
	if true {

This seems suspicious.

---

pkg/sql/copyshim.go line 138 at r1 (raw file):

	}
	rows := 0
	c, err := newCopyMachine(context.TODO(), conn, stmt.AST.(*tree.CopyFrom), p, txnOpt,

Looks like there is ctx in scope.

---

pkg/sql/distsql_running.go line 1296 at r1 (raw file):

}

// PlanAndRunAll combines running the the main query, subqueries and cascades/checks.

nit: add a comment about the error return argument (maybe just copy-paste from execWithDistSQLEngine?).

[cucaroach]

Thanks Yahor! I'll wait for @otan to get back before moving forward with this.

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

---

pkg/sql/copyshim.go line 92 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

This seems suspicious.

Yeah I'll clean it up, I was hoping to figure out a way to test this but it will require some heavy lifting.

---

pkg/sql/copyshim.go line 138 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Looks like there is ctx in scope.

Done.

[otan]

Reviewed 2 of 19 files at r1, 8 of 15 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

---

pkg/sql/copy_from_test.go line 101 at r2 (raw file):

	// send data in 5 batches of 10k rows
	const ROWS = sql.CopyBatchRowSize * 4

nit: constants are usually not fully uppercase. make this copyRowSize or something?

---

pkg/sql/copy_from_test.go line 142 at r2 (raw file):

	ROWS := 50000
	datalen := 0
	THREADS := 10

same here and for ROWS

---

pkg/sql/logictest/logic.go line 2303 at r2 (raw file):

			// not ideal but the go sql.DB interface doesn't support COPY so fixing it the right way that would
			// require major surgery (ie making logictest use libpq or something low level like that).
			rows, err := sql.RunCopyFrom(context.Background(), t.cluster.Server(0), "test", nil, query.sql, []string{data.String()})

is there an advantage to make this runnable using logictest vs. make a new custom datadriven test that specifically tests COPY, ideally using a driver? i can see a case in logic tests for using COPY to insert data, but the caveat you mention here feels like it can be an easy gotcha.

this just seems like it's adding a few more hacks than i understand in full.

[cucaroach]

Dismissed @otan from a discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @yuzefovich)

---

pkg/sql/copy_from_test.go line 101 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

nit: constants are usually not fully uppercase. make this copyRowSize or something?

Hmm, you're right, there are only a handful of upper case vars in the codebase, but they do exist and our code guidelines don't mention it so I'm not inclined to change it.

❯ rg  "\s[A-Z]+\s*:=" pkg/**/*.go                                                                                                                                      -130-
pkg/sql/type_change.go
760:    for _, ID := range typeDesc.ReferencingDescriptorIDs {

pkg/sql/sqlliveness/slstorage/slstorage_test.go
693:    ID := sqlliveness.SessionID("foo")

pkg/upgrade/upgrademanager/manager_external_test.go
376:    N := 25

pkg/sql/physicalplan/aggregator_funcs_test.go
428:    A := math.Abs(a)
429:    B := math.Abs(b)

pkg/sql/pgwire/types_test.go
88:     CET := time.FixedZone("Europe/Paris", 0)
89:     EST := time.FixedZone("America/New_York", 0)

pkg/sql/opt/xform/physical_props.go
243:    H := func(n float64) float64 {

pkg/sql/opt/memo/statistics_builder.go
4207:                   FD := &filtersFDs[i]

---

pkg/sql/logictest/logic.go line 2303 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

is there an advantage to make this runnable using logictest vs. make a new custom datadriven test that specifically tests COPY, ideally using a driver? i can see a case in logic tests for using COPY to insert data, but the caveat you mention here feels like it can be an easy gotcha.

this just seems like it's adding a few more hacks than i understand in full.

Yeah its ugly, I'll file an enhancement request to use a custom datadriven approach, then I can probably use pgx and do it all from the client side right?

[otan]

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

---

pkg/sql/copy_from_test.go line 101 at r2 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Hmm, you're right, there are only a handful of upper case vars in the codebase, but they do exist and our code guidelines don't mention it so I'm not inclined to change it.

❯ rg  "\s[A-Z]+\s*:=" pkg/**/*.go                                                                                                                                      -130-
pkg/sql/type_change.go
760:    for _, ID := range typeDesc.ReferencingDescriptorIDs {

pkg/sql/sqlliveness/slstorage/slstorage_test.go
693:    ID := sqlliveness.SessionID("foo")

pkg/upgrade/upgrademanager/manager_external_test.go
376:    N := 25

pkg/sql/physicalplan/aggregator_funcs_test.go
428:    A := math.Abs(a)
429:    B := math.Abs(b)

pkg/sql/pgwire/types_test.go
88:     CET := time.FixedZone("Europe/Paris", 0)
89:     EST := time.FixedZone("America/New_York", 0)

pkg/sql/opt/xform/physical_props.go
243:    H := func(n float64) float64 {

pkg/sql/opt/memo/statistics_builder.go
4207:                   FD := &filtersFDs[i]

i'm not sure we should be copying something that is "not in the standard style" (and i would also be inclined to change if we weren't on stability), but i'm not gonna block on that.

---

pkg/sql/logictest/logic.go line 2303 at r2 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Yeah its ugly, I'll file an enhancement request to use a custom datadriven approach, then I can probably use pgx and do it all from the client side right?

should we do that upfront instead of committing this? i could be convinced otherwise (i don't want to block progress but if we're going to build something better straight up i'd rather just commit that) but i'm inclined to only commit that.

[cucaroach]

i'm not sure we should be copying something that is "not in the standard style" (and i would also be inclined to change if we weren't on stability), but i'm not gonna block on that.

IMHO discussions about coding conventions that aren't spelled out publicly or encoded in a linter somewhere are bordering on noise and just a drag on code review velocity.

should we do that upfront instead of committing this? i could be convinced otherwise (i don't want to block progress but if we're going to build something better straight up i'd rather just commit that) but i'm inclined to only commit that.

This week is supposed to be SQL queries testing week so I wouldn't be able to get to it until next week and at that point this and the downstream changes that depend on this run the risk of missing the 22.2 boat. Maybe I'm being hyperbolic but seems unwise to hold up feature testing because part of the testing framework is a little janky.

[otan]

IMHO discussions about coding conventions that aren't spelled out publicly or encoded in a linter somewhere are bordering on noise and just a drag on code review velocity.

to be fair, i do think it's somewhere in go conventions, and it's nitty having to edit special cases in code.

Maybe I'm being hyperbolic but seems unwise to hold up feature testing because part of the testing framework is a little janky.

yeah that's fine, just asking.

[cucaroach]

to be fair, i do think it's somewhere in go conventions, and it's nitty having to edit special cases in code.

I went looking for it but came up empty. If the IDE used a different style for const's that would make me happier, still its hard to teach an old C/C++ dog new tricks ;-) If you widen the search to include vendor or the go runtime there's a couple dozen hits so I think its pretty squarely in a grey area. The goruntime uses a lot of single letter capital locals (but not consts). 🤷

[cucaroach]

bors r+

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[cucaroach]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)