kv: introduce new "max outstanding size" setting to txnPipeliner

[nvanbenschoten]

Fixes #32522.

This change creates a new cluster setting called:

kv.transaction.write_pipelining_max_outstanding_size

It limits the size in bytes that can be dedicated to tracking in-flight
writes that have been pipelined. Once this limit is hit, not more writes
will be pipelined by a transaction until some of the writes are proven
and removed from the outstanding write set.

This change once again illustrates the need for periodic proving of
outstanding writes. We touch upon that in the type definition's comment
and in #35009.

Release note: None

[cockroach-teamcity]

This change is 

[tbg]

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @nvanbenschoten)

---

pkg/kv/txn_interceptor_pipeliner.go, line 533 at r1 (raw file):

	}
	tp.owSizeBytes -= int64(len(key))
}

Is there a convenient places for an assertion somewhere? The silly example I can think of is EndTransaction where Key is set kind of late, though that isn't something the pipeliner would track.

[andreimatei]

LGTM

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

---

pkg/kv/txn_interceptor_pipeliner.go, line 36 at r1 (raw file):

)
var pipelinedWritesMaxOutstandingSize = settings.RegisterIntSetting(
	// TODO(nvanbenschoten): The need for this extra setting alongside

I'd say the existence of this setting indicates the need for background proving of intents, not necessarily anything to do with the other setting... no?

---

pkg/kv/txn_interceptor_pipeliner.go, line 152 at r1 (raw file):

	outstandingWrites *btree.BTree
	owSizeBytes       int64

pls comment the field

---

pkg/kv/txn_interceptor_pipeliner.go, line 212 at r1 (raw file):

	asyncConsensus := pipelinedWritesEnabled.Get(&tp.st.SV) && !tp.disabled

	// We provide a setting to bound the size of outstanding writes that the

why declare these variable with such a broad scope? They're only needed in a deeply nested place.

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @tbg)

---

pkg/kv/txn_interceptor_pipeliner.go, line 36 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I'd say the existence of this setting indicates the need for background proving of intents, not necessarily anything to do with the other setting... no?

Probably both, but the point I'm making here is that the need to bound the memory footprint of two separate in-memory structures that track very similar things demonstrates a clear area for simplification.

---

pkg/kv/txn_interceptor_pipeliner.go, line 152 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

pls comment the field

Done.

---

pkg/kv/txn_interceptor_pipeliner.go, line 212 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

why declare these variable with such a broad scope? They're only needed in a deeply nested place.

We modify owSizeBytes in each iteration though, and I don't want the atomic access in the loop.

---

pkg/kv/txn_interceptor_pipeliner.go, line 533 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Is there a convenient places for an assertion somewhere? The silly example I can think of is EndTransaction where Key is set kind of late, though that isn't something the pipeliner would track.

Done.

[nvanbenschoten]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)