spanconfig/store: support applying a batch of updates

[irfansharif]

We first introduced spanconfig.StoreWriter in #70287. Here we extend the
interface to accept a batch of updates instead of just one.

type StoreWriter interface {
    Apply(ctx context.Context, dryrun bool, updates ...Update) (
        deleted []roachpb.Span, added []roachpb.SpanConfigEntry,
    )
}

The implementation is subtle -- we're not processing one update at a
time. The semantics we're after is applying a batch of updates
atomically on a consistent snapshot of the underlying store. This comes
up in the upcoming spanconfig.Reconciler (#71994) -- there, following a
zone config/descriptor change, we want to update KV state in a single
request/RTT instead of an RTT per descendent table. The intended
usage is better captured in the aforementioned PR; let's now talk
about what this entails for the datastructure.

To apply a single update, we want to find all overlapping spans and
clear out just the intersections. If the update is adding a new span
config, we'll also want to add the corresponding store entry after. We
do this by deleting all overlapping spans in their entirety and
re-adding the non-overlapping segments. Pseudo-code:

for entry in store.overlapping(update.span):
  union, intersection = union(update.span, entry), intersection(update.span, entry)
  pre  = span{union.start_key, intersection.start_key}
  post = span{intersection.end_key, union.end_key}

  delete {span=entry.span, conf=entry.conf}
  if entry.contains(update.span.start_key):
    # First entry overlapping with update.
    add {span=pre, conf=entry.conf} if non-empty
  if entry.contains(update.span.end_key):
    # Last entry overlapping with update.
    add {span=post, conf=entry.conf} if non-empty

if adding:
  add {span=update.span, conf=update.conf} # add ourselves

When extending to a set of updates, things are more involved. Let's
assume that the updates are non-overlapping and sorted by start key. As
before, we want to delete overlapping entries in their entirety and
re-add the non-overlapping segments. With multiple updates, it's
possible that a segment being re-added will overlap another update. If
processing one update at a time in sorted order, we want to only re-add
the gap between the consecutive updates.

keyspace         a  b  c  d  e  f  g  h  i  j
existing state      [--------X--------)
updates          [--A--)           [--B--)

When processing [a,c):A, after deleting [b,h):X, it would be incorrect
to re-add [c,h):X since we're also looking to apply [g,i):B. Instead of
re-adding the trailing segment right away, we carry it forward and
process it when iterating over the second, possibly overlapping update.
In our example, when iterating over [g,i):B we can subtract the overlap
from [c,h):X and only re-add [c,g):X.

It's also possible for the segment to extend past the second update. In
the example below, when processing [d,f):B and having [b,h):X carried
over, we want to re-add [c,d):X and carry forward [f,h):X to the update
after (i.e. [g,i):C)).

keyspace         a  b  c  d  e  f  g  h  i  j
existing state      [--------X--------)
updates          [--A--)  [--B--)  [--C--)

One final note: we're iterating through the updates without actually
applying any mutations. Going back to our first example, when processing
[g,i):B, retrieving the set of overlapping spans would (again) retrieve
[b,h):X -- an entry we've already encountered when processing [a,c):A.
Re-adding non-overlapping segments naively would re-add [b,g):X -- an
entry that overlaps with our last update [a,c):A. When retrieving
overlapping entries, we need to exclude any that overlap with the
segment that was carried over. Pseudo-code:

carry-over = <empty>
for update in updates:
  carried-over, carry-over = carry-over, <empty>
  if update.overlap(carried-over):
      # Fill in the gap between consecutive updates.
      add {span=span{carried-over.start_key, update.start_key}, conf=carried-over.conf}
      # Consider the trailing span after update; carry it forward if non-empty.
      carry-over = {span=span{update.end_key, carried-over.end_key}, conf=carried-over.conf}
  else:
      add {span=carried-over.span, conf=carried-over.conf} if non-empty

  for entry in store.overlapping(update.span):
     if entry.overlap(processed):
          continue # already processed

      union, intersection = union(update.span, entry), intersection(update.span, entry)
      pre  = span{union.start_key, intersection.start_key}
      post = span{intersection.end_key, union.end_key}

      delete {span=entry.span, conf=entry.conf}
      if entry.contains(update.span.start_key):
          # First entry overlapping with update.
          add {span=pre, conf=entry.conf} if non-empty
      if entry.contains(update.span.end_key):
          # Last entry overlapping with update.
          carry-over = {span=post, conf=entry.conf}

   if adding:
      add {span=update.span, conf=update.conf} # add ourselves

add {span=carry-over.span, conf=carry-over.conf} if non-empty

We've extended the randomized testing suite to generate batches of
updates at a time. We've also added a few illustrated datadriven tests.

Release note: None

[cockroach-teamcity]

This change is 

[arulajmani]

Pretty cool change! Let's extend the testing a bit more to include deletes in addition to sets in batches, like I commented in-line, but other than that this feels close. 💯

Reviewed 15 of 17 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @irfansharif, and @nvanbenschoten)

---

pkg/spanconfig/spanconfig.go, line 195 at r1 (raw file):

// StoreWriter is the write-only portion of the Store interface.
type StoreWriter interface {
	// Apply applies the given update[1]. It also returns the existing spans that

Let's update this text to talk about multiple updates. It's also worth calling out on the interface that the set of updates must be non-overlapping.

---

pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go, line 338 at r1 (raw file):

			s.mu.Lock()
			for _, ev := range events {
				s.mu.internal.Apply(ctx, false /* dryrun */, ev.(*bufferEvent).Update)

Add a TODO here to push all updated down into the Store instead of applying them one by one so that we can get rid of the locking here (from the conversation we had a couple of weeks ago).

---

pkg/spanconfig/spanconfigstore/store.go, line 139 at r1 (raw file):

	deleted, added, err := s.applyInternal(dryrun, updates...)
	if err != nil {
		log.Fatalf(ctx, "%v", err)

Any reason to fatal here instead of bubbling up the error to the caller?

---

pkg/spanconfig/spanconfigstore/store_test.go, line 60 at r1 (raw file):

// 		apply
// 		set [c,e):C
// 		delete [c,e)

This shouldn't be valid because the spans are overlapping, right? Let's update this comment with "real" inputs and outputs please.

---

pkg/spanconfig/spanconfigstore/store_test.go, line 216 at r1 (raw file):

				updates[i] = getRandomUpdate()
			}
			sort.Slice(updates, func(i, j int) bool {

Instead of sorting here and then doing a linear scan to figure out overlaps, you can compare each span to figure out overlaps so you don't pass sorted input to Apply.

---

pkg/spanconfig/spanconfigstore/testdata/batched/fractured, line 1 at r1 (raw file):

# Test semantics of batched updates that partially overlap with what's already

Should this be in the testdata/single folder instead? You're only using batched updates for the test setup.

---

pkg/spanconfig/spanconfigstore/testdata/batched/overlapping, line 1 at r1 (raw file):

# Test semantics of batched updates that partially overlap with what's already

This is a pretty cool test! We should extend it to include some deletes in the batched updates as well (maybe in a separate file, for readability).

---

pkg/spanconfig/spanconfigstore/testdata/batched/straddle, line 2 at r1 (raw file):

# Test semantics of batched updates that partially overlap with what's already
# present.

How're you viewing "fracturd" vs. "straddle"? Looks like they're setting up things differently, but I'm confused by the terminology.

---

pkg/spanconfig/spanconfigstore/testdata/batched/straddle, line 32 at r1 (raw file):

# keys    a  b  c  d  e  f  g  h  i  j
# state      [--A--)  [--B--)
# set           [---X-------)

Let's add another test where we set [c,h]: X to make this span extend beyond on the right.

---

pkg/spanconfig/spanconfigstore/testdata/batched/straddle, line 45 at r1 (raw file):

# keys    a  b  c  d  e  f  g  h  i  j
# state      [--A--)  [--B--)
# set        [---X-------)

Similar to above, let's add a test where we set [a,f]: X to make this span extend beyond on the left.

[irfansharif]

Thanks for the review! PTAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @nvanbenschoten)

---

pkg/spanconfig/spanconfig.go, line 195 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Let's update this text to talk about multiple updates. It's also worth calling out on the interface that the set of updates must be non-overlapping.

Done.

---

pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go, line 338 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Add a TODO here to push all updated down into the Store instead of applying them one by one so that we can get rid of the locking here (from the conversation we had a couple of weeks ago).

Done.

---

pkg/spanconfig/spanconfigstore/store.go, line 139 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Any reason to fatal here instead of bubbling up the error to the caller?

applyInternal is the bubble-up-err version of this method that we're using in tests. Any errors here are data structure bugs, there's nothing the caller can do -- what's stored has either been corrupted or is being used incorrectly. Fatal-ing feels like the right thing to do.

---

pkg/spanconfig/spanconfigstore/store_test.go, line 60 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

This shouldn't be valid because the spans are overlapping, right? Let's update this comment with "real" inputs and outputs please.

Done.

---

pkg/spanconfig/spanconfigstore/store_test.go, line 216 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Instead of sorting here and then doing a linear scan to figure out overlaps, you can compare each span to figure out overlaps so you don't pass sorted input to Apply.

Added a Shuffle instead.

---

pkg/spanconfig/spanconfigstore/testdata/batched/overlapping, line 1 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

This is a pretty cool test! We should extend it to include some deletes in the batched updates as well (maybe in a separate file, for readability).

Done (added here for contrast with non-deletes).

---

pkg/spanconfig/spanconfigstore/testdata/batched/straddle, line 2 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

How're you viewing "fracturd" vs. "straddle"? Looks like they're setting up things differently, but I'm confused by the terminology.

Renamed to "encompass". I was distinguishing between updates straddling two spans and updates that encompassed multiple spans wholly. It's a flimsy distinction, but presented as edge cases we needed to consider.

---

pkg/spanconfig/spanconfigstore/testdata/batched/straddle, line 32 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Let's add another test where we set [c,h]: X to make this span extend beyond on the right.

Done.

---

pkg/spanconfig/spanconfigstore/testdata/batched/straddle, line 45 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Similar to above, let's add a test where we set [a,f]: X to make this span extend beyond on the left.

Done.

---

pkg/spanconfig/spanconfigstore/testdata/batched/fractured, line 1 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Should this be in the testdata/single folder instead? You're only using batched updates for the test setup.

Added some more batched ops instead.

[arulajmani]

Reviewed 5 of 6 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @irfansharif, and @nvanbenschoten)

---

pkg/spanconfig/spanconfig.go, line 262 at r2 (raw file):

	// [1]: Unless dryrun is true. We'll still generate the same {deleted,added}
	//      lists.
	// [2]: We could alternatively expose a GetAllOverlapping() API to make

I was re-reading your comment about "provided updates aren't no-ops" again and that feels like an unfortunate implementation detail leaking to the interface. It also doesn't feel great to include the same span in both the added and deleted list either. This is just musings for the future, but maybe we should consider a GetAllOverlapping API at some point.

---

pkg/spanconfig/spanconfigstore/store.go, line 139 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

applyInternal is the bubble-up-err version of this method that we're using in tests. Any errors here are data structure bugs, there's nothing the caller can do -- what's stored has either been corrupted or is being used incorrectly. Fatal-ing feels like the right thing to do.

Yeah, but you would also fatal if the caller tries to Apply overlapping spans. No strong feelings though.

---

pkg/spanconfig/spanconfigstore/testdata/batched/straddle, line 2 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Renamed to "encompass". I was distinguishing between updates straddling two spans and updates that encompassed multiple spans wholly. It's a flimsy distinction, but presented as edge cases we needed to consider.

The distinction makes sense. Let's update the comment at the top of the file to be a bit more specific about this?

[irfansharif]

Thanks!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @arulajmani, and @nvanbenschoten)

---

pkg/spanconfig/spanconfig.go, line 262 at r2 (raw file):
Removed the no-op clause.

maybe we should consider a GetAllOverlapping API

There's that exact suggestion as a TODO below. But I don't think it ends up looking that bad even without it (#71994); I've come to prefer it actually. One peculiarity is that this read-only interface would intuitively sit on the StoreReader portion of the interface, but with no parallels in the sys-cfg span implementation. Feels awkward to include it in the StoreWriter? I dunno, bikeshed territory.

It also doesn't feel great to include the same span in both the added and deleted list either.

I'm not sure what you mean. The "added" list also includes fragments that were "re-added", because the updates only partially overlapped with existing spans -- the remainder need to be re-added. We actually need this property to interface with the KVAccessor.

---

pkg/spanconfig/spanconfigstore/store.go, line 139 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Yeah, but you would also fatal if the caller tries to Apply overlapping spans. No strong feelings though.

Yea, that would mean the caller's using it incorrectly, aka a bug. I'm not sure that error's "retryable" either, so I feel the fatal more appropriate.

---

pkg/spanconfig/spanconfigstore/testdata/batched/straddle, line 2 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

The distinction makes sense. Let's update the comment at the top of the file to be a bit more specific about this?

Done.

[craig]

Build succeeded:

• GitHub CI (Cockroach)