distsql: consult liveness during physical planning

[tbg]

The recent PR #22658 introduced a regression in
(*rpcContext).ConnHealth which caused DistSQL to continue planning on
unavailable nodes for about an hour (ttlNodeDescriptorGossip) if the
leaseholder cache happened to not be updated by other non-DistSQL
requests.

Instead, consult node liveness and avoid planning on dead nodes. This
reduces the problem to a <10s window. The defunct ConnHealth mechanism
still protects against planning in some of cases (supposedly due to a
once-per-second reconnection policy) and is retained for that reason,
with issue #23829 filed to decide its future.

NB: I'm not putting a release note since this was introduced after 1.1.
We released it in a beta, though, so it may be worth calling out there.

Touches #23601. (Not fixing it because this issue should only close
when there's a roachtest).

Release note (bug fix): NB: this fixes a regression introduced in
2.0-beta, and not present in 1.1: Avoid planning DistSQL errors against
unavailable nodes.

[cockroach-teamcity]

This change is 

[tbg]

@solongordon with this change I'm unable to repro any planning errors (sleeping 10s after the kill to make liveness time out). Could you give it a spin too to make sure I'm not messing this up (again)?

[solongordon]

Yup, I'm also unable to repro the error with this patch (except within 10s of killing the node).

[andreimatei]

Thanks Tobias. You also did DistSender improvements elsewhere, right?

---

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/sql/distsql_physical_planner.go, line 530 at r1 (raw file):

	//
	// TODO(tschottdorf): it's not clear that this adds anything to the liveness
	// check below. The node descriptor TTL is an hour at the time of writing.

nit nit: i prefer "as of 03/2018"

---

pkg/sql/distsql_physical_planner.go, line 561 at r1 (raw file):

	}

	// NB: not all tests populate a NodeLiveness. Everything using the

I'm not a fan of this comment. Better say on the field definition "can be nil for tests", and remove the panic from the ctor too.

---

Comments from Reviewable

[tbg]

PTAL, added rudimentary unit testing.

---

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/sql/distsql_physical_planner.go, line 530 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit nit: i prefer "as of 03/2018"

Done.

---

pkg/sql/distsql_physical_planner.go, line 561 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I'm not a fan of this comment. Better say on the field definition "can be nil for tests", and remove the panic from the ctor too.

No, that increases the risk that production code passes a nil node liveness as well.

---

Comments from Reviewable

[andreimatei]

but why no roachtest? Still working on it?

---

Review status: 0 of 4 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[tbg]

No, haven't even started working on it. But it should be straightforward given the shell snippet that we have.

[tbg]

TFTR!

[tbg]

PS DistSender PR is here: #23885