slstorage: support regional by row system.sqlliveness

[jeffswenson]

The COCKROACH_MR_SYSTEM_DATABASE environment variable was introduced to
control features added as part of #85736. The variable will be removed
once migrations and version gates are implemented for the multi-region
system database optimizations.

The sqlliveness table now supports an index format compatible with
regional by row tables. Multi region serverless is the motivation for
this change. When a sql server starts up, it must write its session to
the sqlliveness table. The remote session write can add ~400ms to a
servers startup time.

$ COCKROACH_MR_SYSTEM_DATABASE=1 ./cockroach-short demo
[email protected]:26257/movr> SELECT * FROM system.sqlliveness;
  crdb_region |            session_uuid            |           expiration
--------------+------------------------------------+---------------------------------
  \x80        | \x5895d5d121984d86bcb81d6a89d5635d | 1667511355275484444.0000000000

$ COCKROACH_MR_SYSTEM_DATABASE=0 ./cockroach-short demo
[email protected]:26257/movr> select * from system.sqlliveness;
                 session_id                |           expiration
-------------------------------------------+---------------------------------
  \x0101805fc92bdb0e8d4cb4aec3e1527a3052b8 | 1667511392212781946.0000000000

Part of #85736

Release note: None

[cockroach-teamcity]

This change is 

[jeffswenson]

Marking this as a draft for now. I'll remove the draft label once I implement the upgrade job.

[jeffswenson]

@ajwerner it's not clear to me how I should fix the TestSystemTableLiterals test. I chose new column ids for the session_uuid and crdb_region columns. Also the new primary key index is using index id 2. I don't see a way to get a CREATE TABLE statement to use those ids and produce a deeply equal descriptor.

Here's my best idea so far:

1. Rewrite the test to use SHOW CREATE TABLE for the system descriptors.
2. Assert that the hard coded sql matches the SHOW CREATE TABLE output.

Anything else I should consider?

[ajwerner]

Nice work and nice tests. Sorry I didn't give this a look until now. I'll give it a deeper read when I find a gap later this morning. One thing we'll need to sort out after this is the story to plumb the region for the current process into this code.

Another thing to note is some of this code is shifting underneath you with #90875 and #90586.

Regarding the system table literal test. I think there's a few options. One would be to do as you suggest with SHOW CREATE TABLE. I think that's a good fine idea either way. Another thing we can do is attach some special cases to muck with things and then make sure it matches. Like just for this case, attach a closure in a map that might go in and adjust the IDs after calling CreateTestTableDescriptor.

Reviewed 7 of 7 files at r1, 5 of 5 files at r2, 4 of 19 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jaylim-crl and @jeffswenson)

---

pkg/sql/sqlliveness/slstorage/table.go line 49 at r3 (raw file):

// After the version gate is flipped, servers stop dual writing. The legacy RBT
// index is eventually cleaned up by deleteExpiredSessions.
type Table struct {

do the concepts in this file need to be exported?

---

pkg/sql/catalog/systemschema_test/testdata/bootstrap line 283 at r3 (raw file):

	FAMILY other (schedule_name, created, owner, schedule_expr, schedule_details, executor_type, execution_args)
);
CREATE TABLE public.sqlliveness (

one thing we could do is put a unique without index constraint on the primary index. That's implicitly what an RBR table does IIRC.

---

pkg/sql/sqlliveness/slstorage/sessionid_test.go line 25 at r1 (raw file):

)

func FuzzSessionIDEncoding(f *testing.F) {

Cool! This is the first use of this I've seen

[jeffswenson]

I've significantly changed the details of the implementation, but the end result is nearly the same. The big changes are:

1. I removed the version gates and added the COCKROACH_MR_SYSTEM_DATABASE environment variable.
2. I replaced Table with keyCodec. As I was refactoring to support both index formats, I decided Table wasn't really pulling it's weight as an abstraction, so I replaced it with the more minimum keyCodec API.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @jaylim-crl)

---

pkg/sql/catalog/systemschema_test/testdata/bootstrap line 283 at r3 (raw file):

Previously, ajwerner wrote…

one thing we could do is put a unique without index constraint on the primary index. That's implicitly what an RBR table does IIRC.

Can you elaborate? How would I define that in SQL and what is the advantage compared to the explicit primary key?

---

pkg/sql/sqlliveness/slstorage/table.go line 49 at r3 (raw file):

Previously, ajwerner wrote…

do the concepts in this file need to be exported?

Done

I was exporting Table so it could be used for tests. I replaced Table with keyCodec and now use slstorage.Storage in the multi region table test.

[ajwerner]

This is satisfyingly more simple! Just nits from me.

Reviewed 1 of 23 files at r5, 6 of 8 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jaylim-crl and @jeffswenson)

---

pkg/sql/catalog/systemschema/system.go line 2010 at r7 (raw file):

	// SqllivenessTable is the descriptor for the sqlliveness table.
	SqllivenessTable = func() catalog.TableDescriptor {
		if SupportMultiRegion() {

can you add another sentence for any wayward souls who find themselves in this file to answer what this ceremony is about and to inform them that this is not intended to be long for this world. Or just direct them to SupportsMultiRegion.

Code quote:

	SqllivenessTable = func() catalog.TableDescriptor {
		if SupportMultiRegion() {

---

pkg/sql/catalog/systemschema/system.go line 2689 at r7 (raw file):

// TODO(jeffswenson): remove SupportMultiRegion after implementing migrations
// and version gates to migrate to the new regional by row compatible schemas.
func SupportMultiRegion() bool {

How about calling this thing TestingSupportMultiRegion or something like that to make it double clear it's a hack

---

pkg/sql/catalog/systemschema/system.go line 2690 at r7 (raw file):

// and version gates to migrate to the new regional by row compatible schemas.
func SupportMultiRegion() bool {
	val, ok := envutil.EnvString("COCKROACH_MR_SYSTEM_DATABASE", 0)

Why not envutil.EnvOrDefaultBool?

---

pkg/sql/sqlliveness/slstorage/key_encoder.go line 30 at r7 (raw file):

	encode(sid sqlliveness.SessionID) (roachpb.Key, error)
	decode(key roachpb.Key) (sqlliveness.SessionID, error)
	prefix() roachpb.Key

use some commentary to describe the meaning of prefix and its relationship to what you pass to decode and what comes out of encode.

---

pkg/sql/sqlliveness/slstorage/key_encoder.go line 40 at r7 (raw file):

		return &rbrEncoder{codec.IndexPrefix(uint32(tableID), uint32(rbrIndex))}
	}
	return &rbtEncoder{codec.IndexPrefix(uint32(tableID), 1)}

nit: using consts for the index ID and the 0 family ID below will make the code feel a touch less magical.

---

pkg/ccl/multiregionccl/multiregion_system_table_test.go line 68 at r7 (raw file):

	defer leaktest.AfterTest(t)()
	defer log.Scope(t).Close(t)
	defer envutil.TestSetEnv(t, "COCKROACH_MR_SYSTEM_DATABASE", "1")()

This works because we aren't using the system database's table, right?

[jeffswenson]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @jaylim-crl)

---

pkg/sql/catalog/systemschema/system.go line 2010 at r7 (raw file):

Previously, ajwerner wrote…

can you add another sentence for any wayward souls who find themselves in this file to answer what this ceremony is about and to inform them that this is not intended to be long for this world. Or just direct them to SupportsMultiRegion.

Done

---

pkg/sql/catalog/systemschema/system.go line 2689 at r7 (raw file):

Previously, ajwerner wrote…

How about calling this thing TestingSupportMultiRegion or something like that to make it double clear it's a hack

Done

---

pkg/sql/catalog/systemschema/system.go line 2690 at r7 (raw file):

Previously, ajwerner wrote…

Why not envutil.EnvOrDefaultBool?

Done

Thanks! I did not know about EnvOrDefaultBool.

---

pkg/sql/sqlliveness/slstorage/key_encoder.go line 30 at r7 (raw file):

Previously, ajwerner wrote…

use some commentary to describe the meaning of prefix and its relationship to what you pass to decode and what comes out of encode.

Done

I also renamed the method to indexPrefix

---

pkg/sql/sqlliveness/slstorage/key_encoder.go line 40 at r7 (raw file):

Previously, ajwerner wrote…

nit: using consts for the index ID and the 0 family ID below will make the code feel a touch less magical.

Done

---

pkg/ccl/multiregionccl/multiregion_system_table_test.go line 68 at r7 (raw file):

Previously, ajwerner wrote…

This works because we aren't using the system database's table, right?

Exactly. The actual system database table is determined by the value of the environment variable set at process start time.