ttljob: wire up range_concurrency and cleanup query construction

[otan]

See individual commits for details.

[cockroach-teamcity]

This change is 

[otan]

this file was a move from ttljob_integration_test.go; only the concurrent tests are new

[rafiss]

some minor nits

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

---

pkg/sql/paramparse/paramobserver.go, line 323 at r6 (raw file):

		onReset: func(po *TableStorageParamObserver, evalCtx *tree.EvalContext, key string) error {
			if po.tableDesc.RowLevelTTL != nil {
				po.tableDesc.RowLevelTTL.RangeConcurrency = 0

should it reset to 1? (and if so, needs a test)

---

pkg/sql/ttl/ttljob/ttljob.go, line 110 at r6 (raw file):

		// early as the delete will not work.
		if desc.GetModificationTime().GoTime().After(aost.Time) {
			return errors.Newf("found a recent schema change on the table, aborting")

is there any use in including the timestamps in the error message?

---

pkg/sql/ttl/ttljob/ttljob.go, line 142 at r6 (raw file):

		startPK, endPK tree.Datums
	}
	ch := make(chan rangeToProcess)

should this channel be size-capped at the level of concurrency?

---

pkg/sql/ttl/ttljob/ttljob.go, line 178 at r6 (raw file):

	if err := func() error {
		defer close(ch)

i feel like it might be nicer to not have to call g.Wait in two separate places. what about

func() (retErr error) {
  defer func() { retErr = errors.CombineErrors(retErr, g.Wait()) }()
  defer close(ch)
....

---

pkg/sql/ttl/ttljob/ttljob.go, line 183 at r6 (raw file):

				return err
			}
			var r rangeToProcess

is r shadowed here on purpose? if not, would be good to use some descriptive names

---

pkg/sql/ttl/ttljob/ttljob_query_builder.go, line 36 at r6 (raw file):

	aost            tree.DTimestampTZ

	// isFirst is true if we have no invoked a query using the builder yet.

nit: no->not

---

pkg/sql/ttl/ttljob/ttljob_query_builder.go, line 40 at r6 (raw file):

	// queryCache is the cached query, which stays the same from the second
	// iteration onwards.
	queryCache string

nit: maybe rename to cachedQuery and cachedArgs? the phrasing now makes it seem like they are caches, but they actually are cached data

---

pkg/sql/ttl/ttljob/ttljob_query_builder_test.go, line 271 at r6 (raw file):

	}

	for _, tc := range testCases {

nice tests

[rafiss]

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

---

pkg/sql/exec_util.go, line 1479 at r6 (raw file):

	// MockDescriptorVersionDuringDelete is a version to mock the delete descriptor
	// as during delete.
	MockDescriptorVersionDuringDelete *descpb.DescriptorVersion

do these two knobs need to be pointers? (or is that a convention?)

[otan]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/exec_util.go, line 1479 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

do these two knobs need to be pointers? (or is that a convention?)

it's to differentiate do not set vs run and set. i've seen it in a few knobs.

---

pkg/sql/paramparse/paramobserver.go, line 323 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

should it reset to 1? (and if so, needs a test)

nah it's 0. if it is 0, we use the cluster setting default.

---

pkg/sql/ttl/ttljob/ttljob.go, line 110 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

is there any use in including the timestamps in the error message?

Done.

---

pkg/sql/ttl/ttljob/ttljob.go, line 142 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

should this channel be size-capped at the level of concurrency?

Done.

---

pkg/sql/ttl/ttljob/ttljob.go, line 178 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i feel like it might be nicer to not have to call g.Wait in two separate places. what about

func() (retErr error) {
  defer func() { retErr = errors.CombineErrors(retErr, g.Wait()) }()
  defer close(ch)
....

that's more prone to bugs imo. if retErr is nil, CombinedErrors will be nil even if g.Wait() is not nil!

---

pkg/sql/ttl/ttljob/ttljob.go, line 183 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

is r shadowed here on purpose? if not, would be good to use some descriptive names

Done. (Yes but descriptive names are good)

---

pkg/sql/ttl/ttljob/ttljob_query_builder_test.go, line 271 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nice tests

danke!

[rafiss]

lgtm! still an optional nit on CombineErrors usage

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

---

pkg/sql/ttl/ttljob/ttljob.go, line 178 at r6 (raw file):

Previously, otan (Oliver Tan) wrote…

that's more prone to bugs imo. if retErr is nil, CombinedErrors will be nil even if g.Wait() is not nil!

that's how Wrap works. CombineErrors does what we want

// CombineErrors returns err, or, if err is nil, otherErr.
// if err is non-nil, otherErr is attached as secondary error.

we rely on this pattern a lot. e.g.

cockroach/pkg/server/admin.go

Line 264 in 4996177

defer func(it sqlutil.InternalRows) { retErr = errors.CombineErrors(retErr, it.Close()) }(it)

[otan]

done

bors r=rafiss

[craig]

Canceled.

[otan]

bors r=rafiss

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)