changefeedccl: error when a watched table backfills

[danhhz]

When a table is currently being backfilled for a schema change (e.g.
adding a column with a default value), it's unclear what the expectation
is for any rows that are changed during the backfill. Our current
invariant is that rows are emitted with an updated timestamp and a later
SELECT ... AS OF SYSTEM TIME for that row would exactly match the
emitted data. During the backfill, there is nothing we can emit that
would definitely meet that invariant (because the backfill can be
aborted and rolled back).

In the meantime, this commit makes sure that we error whenever a
backfill happens, even if it's fast enough that we never get it from
leasing.

This also paves the way for switching to RangeFeed, which doesn't have
the convenient fetchSpansForTargets hook that the ExportRequest based
poller was (ab)using.

Closes #28643

Release note (bug fix): CHANGEFEEDs now error when a watched table
backfills (instead of undefined behavior)

[cockroach-teamcity]

This change is 

[danhhz]

@vivekmenezes can you review the second commit, please? It's got some assumptions about leasing and schema change backfills that I want to make sure I got right. Feel free to ignore the first commit, @mrtracy will review that one.

[danhhz]

@mrtracy @vivekmenezes thoughts on this?

[vivekmenezes]

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/ccl/changefeedccl/changefeed_processors.go, line 171 at r2 (raw file):

	}(ctx)
	ca.tableHistUpdaterDoneCh = make(chan struct{})
	go func(ctx context.Context) {

Any reason for not using poller.RunTask() ?

---

pkg/ccl/changefeedccl/changefeed_stmt.go, line 328 at r2 (raw file):

			return errors.Errorf(`CHANGEFEEDs cannot operate on tables being backfilled`)
		}
	}

it will be cool if you can move the above logic surrounding Mutations to sit in sqlbase/structured.go . I've consciously tried to keep it there so that future refactoring work will be easier.

---

pkg/ccl/changefeedccl/changefeed_test.go, line 434 at r2 (raw file):

			`unique: [2]->{"a": 2, "b": "2"}`,
		})
	})

Really nice job with the testing!

[danhhz]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/ccl/changefeedccl/changefeed_processors.go, line 171 at r2 (raw file):

Previously, vivekmenezes wrote…

Any reason for not using poller.RunTask() ?

Good idea! I hadn't thought of it. Tried it just now, but looks like it's really awkward for distsql to handle an error during Start, so I think I'll keep this the way it is

---

pkg/ccl/changefeedccl/changefeed_stmt.go, line 328 at r2 (raw file):

Previously, vivekmenezes wrote…

it will be cool if you can move the above logic surrounding Mutations to sit in sqlbase/structured.go . I've consciously tried to keep it there so that future refactoring work will be easier.

Absolutely! Done

---

pkg/ccl/changefeedccl/changefeed_test.go, line 434 at r2 (raw file):

Previously, vivekmenezes wrote…

Really nice job with the testing!

It caught TONS of bugs while I was writing the PR. Totally worth the effort

[mrtracy]

Commit 1 is with a couple of nits, nice tests

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/ccl/changefeedccl/table_history.go, line 87 at r1 (raw file):

// WaitForTS blocks until the given timestamp is greater or equal to the
// high-water or error timestamp. In the latter case, the error is returned.

I think this is the other way around; it waits until the given timestamp is less than or equal to the high-water or error timestamp.

---

pkg/ccl/changefeedccl/table_history.go, line 168 at r1 (raw file):

	}

	m.mu.Lock()

I think you should move this above the validateErr check, and remove the Lock/Unlock inside of the validateErr check block. This whole area works inside of the mutex.

[danhhz]

@mrtracy RFAL

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/ccl/changefeedccl/table_history.go, line 87 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

I think this is the other way around; it waits until the given timestamp is less than or equal to the high-water or error timestamp.

Done.

---

pkg/ccl/changefeedccl/table_history.go, line 168 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

I think you should move this above the validateErr check, and remove the Lock/Unlock inside of the validateErr check block. This whole area works inside of the mutex.

I was hoping this way would make it easier to reason about, but sounds like my intuition was wrong. I moved this whole thing into a new method, which I think is even better (one lock followed immediately by a defer at the very start of the new method)

[mrtracy]

Still LGTM, thanks!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/ccl/changefeedccl/table_history.go, line 168 at r1 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

I was hoping this way would make it easier to reason about, but sounds like my intuition was wrong. I moved this whole thing into a new method, which I think is even better (one lock followed immediately by a defer at the very start of the new method)

Yes, looks nicer.

---

pkg/ccl/changefeedccl/table_history.go, line 47 at r3 (raw file):

// The `ValidateThroughTS` method allows a user to block until some given
// timestamp is greater (or equal) to either the high-water or the error
// timestamp. In the latter case, it returns the error.

Sorry, missed this. Same comment problem that you just fixed below. This one is unexported, but now its inconsistent.

[danhhz]

Thanks for the reviews!

bors r=mrtracy,vivekmenezes

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/ccl/changefeedccl/table_history.go, line 47 at r3 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Sorry, missed this. Same comment problem that you just fixed below. This one is unexported, but now its inconsistent.

Done.

[craig]

Build succeeded

• GitHub CI (Cockroach)