sqlliveness: embed current region into session ID

[jaylim-crl]

Follow up on #90408 and #91019.

Previously, the sqlliveness subsystem was using enum.One when constructing the
session ID since it doesn't have the regional data yet. This commit implements
that missing part of it by ensuring that the regional representation is
plumbed all the way to the sqlliveness subsystem whenever the SQL pod is
started, and use it to construct the session ID. This will enable our REGIONAL
BY ROW work to put the data in the right region.

The multi-region enum for the system database will be read during startup for
that to work. Since doing this already requires loading the system DB's
descriptor (which indirectly tests whether the system DB has been bootstrapped)
for the tenant, we can remove the call to checkTenantExists.

Epic: None

Release note: None

[cockroach-teamcity]

This change is 

[jaylim-crl]

I would need to write a test for the DescsTxn work in server_sql.go, but the rest should be here.

Reviewable status: complete! 0 of 0 LGTMs obtained

[postamar]

I just have a couple of high-level comments here, which may be wrong. @ajwerner will no doubt to look at this more closely and have stronger opinions than me :)

[ajwerner]

Reviewed 5 of 7 files at r1, 6 of 6 files at r4, 3 of 3 files at r5, 3 of 3 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson and @postamar)

---

pkg/server/server_sql.go line 1320 at r7 (raw file):

Previously, postamar (Marius Posta) wrote…

We have a SynthesizeRegionConfig function in sql which does most of this already, would it be possible to use it? You'd have to extend multiregion.RegionConfig to include the physical representation, presumably, but that seems reasonable.

My vote is that we share the parts that are used also by SynthesizeRegionConfig and make this a function in regionutil also.

---

pkg/sql/sqlliveness/sqlliveness.go line 34 at r7 (raw file):

Previously, postamar (Marius Posta) wrote…

This dependency of catalog on eval is a shameful wart which the SQL Schema team has meant to remove for a while now but still hasn't. Sorry about that!

In any case even if that obstacle weren't in the way, the dependency structure of your change bothers me a bit. I may not have the full picture here but it feels like you're pushing too much high-level logic into SetRegionalData when in fact it would be better off being more "dumb" and instead just take the []bytes for the physical representation of the region, either nakedly or wrapped in some lightweight abstraction.

This could in turn originate from a new method you could add on multiregion.RegionConfig and which you call in the server package. This way no need to shadow catalog.TypeDescriptor at all, which is just as well.

FWIW, I think this narrowing of an interface is totally fine. I'm not sure I'm in love with falling back to the *descpb.TypeDescriptor. I think you could narrow it even further if you wanted. My 2c is you could just as well pass a struct that has the primary region, the process's locality, and the mapping of region to prefix bytes?

[postamar]

I'm not sure I'm in love with falling back to the *descpb.TypeDescriptor

I'd go stronger in that we definitely should not do that! If we want to go further on this path of defining a sqlliveness.TypeDescriptor shadow-interface, the thing to do would be: remove the TypeDesc method and add RegionNames, NumEnumMembers, GetMemberPhysicalRepresentation and GetMemberLogicalRepresentation and implement SetRegionalData using those. It's a bit contrived, but the type descriptor protobuf has no place there.

[jaylim-crl]

TFTRs! I cleaned up sqlliveness to only take in a []byte slice so no additional dependencies are introduced. The logic of choosing the current region was pushed to the caller as well.

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

---

pkg/server/server_sql.go line 1320 at r7 (raw file):

Previously, ajwerner wrote…

My vote is that we share the parts that are used also by SynthesizeRegionConfig and make this a function in regionutil also.

Done. Moved all the main logic to GetRegionEnumRepresentations. Did not extend multiregion.RegionConfig since we have multiple callers that were constructing regional configs, and don't have the physical representations available.

---

pkg/sql/region_util.go line 1554 at r8 (raw file):

			CommonLookupFlags: tree.CommonLookupFlags{
				AvoidLeased:    !useCache,
				Required:       true,

Note that one additional change here is to specify Required=true when looking up the multi-region enum descriptor, which wasn't done previously. I'd imagine this should be OK.

---

pkg/sql/sqlliveness/sqlliveness.go line 50 at r1 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

nit: Instead of introducing the SetRegionalData method, the locality information should be an argument to Start. SetRegionalData must be called before Start and the easiest way to ensure that is to fuse the methods into one.

Done.

---

pkg/sql/sqlliveness/sqlliveness.go line 34 at r7 (raw file):

Previously, ajwerner wrote…

FWIW, I think this narrowing of an interface is totally fine. I'm not sure I'm in love with falling back to the *descpb.TypeDescriptor. I think you could narrow it even further if you wanted. My 2c is you could just as well pass a struct that has the primary region, the process's locality, and the mapping of region to prefix bytes?

Done. Updated it to take in a []byte instead so no additional dependencies are required.

[ajwerner]

This is getting close.

Reviewed 1 of 2 files at r7, 11 of 12 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jaylim-crl, @jeffswenson, and @postamar)

---

pkg/sql/region_util.go line 1554 at r8 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

Note that one additional change here is to specify Required=true when looking up the multi-region enum descriptor, which wasn't done previously. I'd imagine this should be OK.

yeah, for bad reasons, it was implied.

cockroach/pkg/sql/catalog/descs/type.go

Lines 106 to 107 in 3038ea9

	// Required is ignored, and an error is always returned if no descriptor with
	// the ID exists.

---

pkg/sql/sqlliveness/sqlliveness.go line 37 at r8 (raw file):

// consumption.
type Provider interface {
	Start(ctx context.Context, regionPhysicalRep []byte)

Please provide commentary explaining what this is. I even think you could define a type to make it read more cleanly, even if it's a type alias like like type RegionEnumValue = []byte

---

pkg/sql/sqlliveness/slinstance/slinstance.go line 406 at r8 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

There was a started bool back then, but has been removed in e1d5835. We should be able to remove the mutex.

// Start runs the hearbeat loop.
func (l *Instance) Start(ctx context.Context) {
	l.mu.Lock()
	defer l.mu.Unlock()
	if l.mu.started {
		return
	}
	log.Infof(ctx, "starting SQL liveness instance")
	_ = l.stopper.RunAsyncTask(ctx, "slinstance", l.heartbeatLoop)
	l.mu.started = true
}

Yeah, I think the mutex is stale

[ajwerner]

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

---

pkg/sql/sqlliveness/slinstance/slinstance.go line 406 at r8 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

Will be removed in next push. Consider this done.

Did you forget to push?

[jaylim-crl]

TFTRs!

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

---

pkg/sql/region_util.go line 1500 at r8 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

nit: GetRegionEnumRepresentations is a good candidate for unit testing.

Done.

---

pkg/sql/region_util.go line 1554 at r8 (raw file):

Previously, ajwerner wrote…

yeah, for bad reasons, it was implied.

cockroach/pkg/sql/catalog/descs/type.go

Lines 106 to 107 in 3038ea9

	// Required is ignored, and an error is always returned if no descriptor with
	// the ID exists.

Done.

---

pkg/sql/sqlliveness/sqlliveness.go line 37 at r8 (raw file):

Previously, ajwerner wrote…

Please provide commentary explaining what this is. I even think you could define a type to make it read more cleanly, even if it's a type alias like like type RegionEnumValue = []byte

Done. I left it as []byte for now. Maybe when we start taking in the region physical representation in more places in the future, we can revisit this decision.

---

pkg/sql/sqlliveness/slinstance/slinstance.go line 406 at r8 (raw file):

Previously, ajwerner wrote…

Did you forget to push?

Done.

[jeffswenson]

LGTM

[ajwerner]

🚢🇮🇹

Reviewed 6 of 9 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jaylim-crl, @jeffswenson, and @postamar)

[jaylim-crl]

TFTRs!

bors r=JeffSwenson,ajwerner

[craig]

Timed out.

[jaylim-crl]

bors r=JeffSwenson,ajwerner

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)