Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sqlliveness: encode region in session id #91019

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

jeffswenson
Copy link
Collaborator

Encode a region enum in the sqlliveness session id. The region will be used to support converting the sqlliveness and sql_instances table to regional by row tables.

This change creates a custom encoding for the session id. The encoding is convenient, as it allows adding a region to the session id without requiring modifications to the jobs table or the
crdb_internal.sql_liveness_is_alive built in.

The enum.One value is a work around for the fact the system database does not include a region enum by default. In the absence of a region enum, enum.One will be used in the session.

Part of #85736

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jeffswenson
Copy link
Collaborator Author

This was extracted from #90408 in order to unblock work on converting sql_instances to an RBR table.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have nits, but I think this is also going to skew with #90875 which does some decoding.

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


pkg/sql/sqlliveness/slstorage/sessionid.go line 19 at r1 (raw file):

)

const sessionIDVersion = 1

nit: define a type which is a uint8 and then make this const that.


pkg/sql/sqlliveness/slstorage/sessionid.go line 66 at r1 (raw file):

		return nil, b, nil
	}
	if len(b) < (uuid.Size + 3) {

nit: how do you feel about being pedantic with consts to explain it? Maybe something like:

const (
	legacyLen              = uuid.Size
	versionLen             = 1
	regionLengthLen        = 1
	minimumRegionLen       = 1
	minimumPrefixLen       = versionLen + regionLengthLen + minimumRegionLen
	minimumNonLegacyLen    = minimumPrefixLen + uuid.Size
)

pkg/sql/sqlliveness/slstorage/sessionid.go line 72 at r1 (raw file):

	}

	// Decode the version

nit: period


pkg/sql/sqlliveness/slstorage/sessionid.go line 79 at r1 (raw file):

	rest := b[2:]

	// Decode and validate the length of the region

nit: period

@jeffswenson jeffswenson force-pushed the jeffswenson-session-id branch from 16e555d to 6498ec7 Compare November 2, 2022 14:16
Encode a region enum in the sqlliveness session id. The region will be
used to support converting the sqlliveness and sql_instances table to
regional by row tables.

This change creates a custom encoding for the session id. The encoding
is convenient, as it allows adding a region to the session id without
requiring modifications to the jobs table or the
crdb_internal.sql_liveness_is_alive built in.

The enum.One value is a work around for the fact the system database
does not include a region enum by default. In the absence of a region
enum, enum.One will be used in the session.

Part of cockroachdb#85736

Release note: None
@jeffswenson jeffswenson force-pushed the jeffswenson-session-id branch from 6498ec7 to abd40ac Compare November 2, 2022 14:18
Copy link
Collaborator Author

@jeffswenson jeffswenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case there was no skew, because the decoding is introduced by #90408.

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


pkg/sql/sqlliveness/slstorage/sessionid.go line 19 at r1 (raw file):

Previously, ajwerner wrote…

nit: define a type which is a uint8 and then make this const that.

Done


pkg/sql/sqlliveness/slstorage/sessionid.go line 66 at r1 (raw file):

Previously, ajwerner wrote…

nit: how do you feel about being pedantic with consts to explain it? Maybe something like:

const (
	legacyLen              = uuid.Size
	versionLen             = 1
	regionLengthLen        = 1
	minimumRegionLen       = 1
	minimumPrefixLen       = versionLen + regionLengthLen + minimumRegionLen
	minimumNonLegacyLen    = minimumPrefixLen + uuid.Size
)

Done


pkg/sql/sqlliveness/slstorage/sessionid.go line 72 at r1 (raw file):

Previously, ajwerner wrote…

nit: period

Done


pkg/sql/sqlliveness/slstorage/sessionid.go line 79 at r1 (raw file):

Previously, ajwerner wrote…

nit: period

Done

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pr makes me feel like having session ID defined as opaque bytes wasn't such a bad idea.

@jeffswenson
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Nov 2, 2022

Build succeeded:

@craig craig bot merged commit 206fc07 into cockroachdb:master Nov 2, 2022
@jeffswenson jeffswenson deleted the jeffswenson-session-id branch November 2, 2022 15:58
jaylim-crl added a commit to jaylim-crl/cockroach that referenced this pull request Nov 10, 2022
…stance

Follow up on cockroachdb#90408 and cockroachdb#91019.

This commit introduces a new SetRegionalData method on the instance object to
allow callers set the current region. This region data will then be used to
construct the session ID.

Epic: None

Release note: None
jaylim-crl added a commit to jaylim-crl/cockroach that referenced this pull request Nov 12, 2022
…stance

Follow up on cockroachdb#90408 and cockroachdb#91019.

This commit introduces a new SetRegionalData method on the instance object to
allow callers set the current region. This region data will then be used to
construct the session ID.

Epic: None

Release note: None
jaylim-crl added a commit to jaylim-crl/cockroach that referenced this pull request Nov 14, 2022
Follow up on cockroachdb#90408 and cockroachdb#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
jaylim-crl added a commit to jaylim-crl/cockroach that referenced this pull request Nov 21, 2022
Follow up on cockroachdb#90408 and cockroachdb#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
jaylim-crl added a commit to jaylim-crl/cockroach that referenced this pull request Nov 22, 2022
Follow up on cockroachdb#90408 and cockroachdb#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
jaylim-crl added a commit to jaylim-crl/cockroach that referenced this pull request Nov 23, 2022
Follow up on cockroachdb#90408 and cockroachdb#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
craig bot pushed a commit that referenced this pull request Nov 23, 2022
91694: sqlliveness: embed current region into session ID r=JeffSwenson,ajwerner a=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

92320: sql: remove Txn() from JobExecContext r=adityamaru a=stevendanna

This removes Txn() from JobExecContext. This method would always
return a nil txn, making it a bit error prone to actually use in
jobs.

Epic: None

Release note: None

Co-authored-by: Jay <[email protected]>
Co-authored-by: Steven Danna <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants