server: clean up sql session and instance on shutdown

[jeffswenson]

Remove the sql server's system.sql_instance and sqlliveness.sqlliveness
rows on sql server shut down. The original motivation for this change is
it improves the behavior of DistSQL for serverless tenants. SQL servers
would attempt to schedule DistSQL flows on sql_instance rows that
belonged to shutdown servers.

Cleaning up the session and instance have a hand full of small benefits.

• The sql_instances pre-allocated in a region are less likely to run out
  and trigger the slow cold start path.
• Resources leased by the session, like jobs, may be re-claimed more
  quickly after the server shuts down.

Fixes: CC-9095

Release note: none

[cockroach-teamcity]

This change is 

[knz]

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

---

pkg/server/drain.go line 381 at r1 (raw file):

	s.sqlServer.leaseMgr.SetDraining(ctx, true /* drain */, reporter)

	session, err := s.sqlServer.sqlLivenessProvider.Release(ctx)

Note: these two method calls (this one and the one below) need to be idempotent -- the Drain call is performed multiple times until it doesn't fail any more.

[jeffswenson]

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

---

pkg/server/drain.go line 381 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Note: these two method calls (this one and the one below) need to be idempotent -- the Drain call is performed multiple times until it doesn't fail any more.

Done.

[knz]

Reviewed 12 of 12 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson)

---

pkg/sql/sqlliveness/slinstance/slinstance.go line 319 at r2 (raw file):

	select {
	case <-l.drain:
		log.Infof(ctx, "draining heartbeat loop")

Why not close l.mu.blockCh in this case?

---

pkg/sql/sqlliveness/slinstance/slinstance.go line 437 at r2 (raw file):

	l.wait.Add(1)
	err := l.stopper.RunAsyncTask(taskCtx, "slinstance", func(ctx context.Context) {

add defer l.wait.Done() at the beginning which is more idiomatic than Add(-1).

---

pkg/sql/sqlinstance/instancestorage/instancestorage_test.go line 192 at r2 (raw file):

		}

		// Release an instance and verify the instance is available

nit: period at end.

---

pkg/sql/sqlinstance/instancestorage/instancestorage_test.go line 196 at r2 (raw file):

			toRelease := initialInstances[0]

			// Call twice to ensure it is idempotent

ditto

---

pkg/sql/sqlinstance/instancestorage/instancestorage_test.go line 213 at r2 (raw file):

			}

			// re-allocate the instance

nit: capital at start and period at end.

---

pkg/sql/sqlinstance/instancestorage/instancestorage_test.go line 240 at r2 (raw file):

		}

		// TODO(jeffswenson): verify release is idempotent

I believe you've done this elsewhere already. Maybe remove this comment.

---

pkg/sql/sqlliveness/slinstance/slinstance_test.go line 132 at r2 (raw file):

	require.NotEqual(t, 0, stopper.NumTasks())

	// Make sure release is idempotent

nit: period at end. Ditto below.

[jeffswenson]

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

---

pkg/sql/sqlliveness/slinstance/slinstance.go line 319 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Why not close l.mu.blockCh in this case?

A side effect of clearSessionLocked is it allocates a new blockCh. blockCh is expected to always be nil when the server is draining. If it were not nil, then clearSessionLocked would panic.

I added a comment describing where the blockCh is coming from. The blockCh is used by the SqlInstance.Session method, so I updated the test to ensure the Session method does the right thing after the session is released.

---

pkg/sql/sqlliveness/slinstance/slinstance.go line 437 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

add defer l.wait.Done() at the beginning which is more idiomatic than Add(-1).

Done.

---

pkg/sql/sqlinstance/instancestorage/instancestorage_test.go line 196 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

ditto

Done.

---

pkg/sql/sqlinstance/instancestorage/instancestorage_test.go line 240 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I believe you've done this elsewhere already. Maybe remove this comment.

Done.

[knz]

very nice!

the remaining CI failure (maybe_stress) isn't related to your PR and shouldn't be a blocker.

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jeffswenson)

[jeffswenson]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)