sql: fix mixed-version behaviour of create_tenant() #94774
Conversation
|
This change is |
postamar
force-pushed
the
data-driven-bootstrap
branch
from
d9ba4d3 to
ffc6d7e
Compare
postamar
changed the title
postamar
force-pushed
the
data-driven-bootstrap
branch
3 times, most recently
from
bfd95b3 to
37d302c
Compare
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
postamar
force-pushed
the
data-driven-bootstrap
branch
from
37d302c to
aac5192
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/jobs/registry.go line 533 at r4 (raw file):
ctx, "job-row-update", txn, updateStmt, jobID, jobType.String(), ); err != nil { if pgerror.GetPGCode(err) != pgcode.UndefinedColumn {
consider using select column_name from [show columns from system.jobs] where column_name = 'job_type' rather than executing the query and checking for an error. it should be possible to use pg_catalog also
or if we have the system.jobs descriptor, we could use the FindColumnWithName function
pkg/upgrade/upgrades/permanent_upgrades.go line 71 at r4 (raw file):
ctx, "addRootToAdminRole", nil /* txn */, updateMembership, username.AdminRole, username.RootUser, username.AdminRoleID, username.RootUserID) if err != nil { if pgerror.GetPGCode(err) == pgcode.UndefinedColumn {
similar to above, could use show columns here, or inspect the system.role_members descriptor
postamar
force-pushed
the
data-driven-bootstrap
branch
from
aac5192 to
3db94bb
Compare
|
Thanks for the suggestion @rafiss, I ended up using a pg_catalog table which should not incur any extra overhead. |
postamar
force-pushed
the
data-driven-bootstrap
branch
from
3db94bb to
8722605
Compare
| // | ||
| // This string literal was generated by copying over the expected results | ||
| // for the TestInitialValuesToString test in the release-22.2 branch. | ||
| nonSystem222 = `[{"key":""} |
There was a problem hiding this comment.
what's this first entry about?
There was a problem hiding this comment.
Keys without values are split keys and this one is the split we define at the tenant prefix, but in this datum the prefix is stripped away leaving the empty string.
postamar
force-pushed
the
data-driven-bootstrap
branch
from
8722605 to
6753acb
Compare
postamar
force-pushed
the
data-driven-bootstrap
branch
3 times, most recently
from
86e28eb to
9bddb8a
Compare
This commit adds InitialValuesToString and InitialValuesFromString which convert KV datums and KV split keys from a MetadataSchema to a string representation and vice-versa. The tenant prefix is omitted from the string representation. This functionality exists to allow bootstrapping clusters using the values and splits as generated by an earlier cockroach binary. Informs cockroachdb#94773. Release note: None
Previously, running create_tenant on the latest binary in a mixed-version cluster would bootstrap the system schema using the logic from the latest binary, in which all system database migration upgrade steps have been "baked in". This might not be compatible with the the tenant cluster's version which is (correctly) set to the active host cluster version, the idea there being that tenant clusters versions cannot be greater than host cluster versions. This commit fixes this by version-gating the tenant cluster system schema bootstrapping, and using hard-coded values when bootstrapping into the old version. Informs cockroachdb#94773. Release note: None
postamar
force-pushed
the
data-driven-bootstrap
branch
from
9bddb8a to
08f2dc4
Compare
|
Thanks for the reviews. I'm going to bors this now before it grows stale. bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
In cockroachdb#94774, we didn't intend to change the TLV and it is still supposed to be equal to the SLV. Changing the way we set it however has caused TLV and SLV to be different if a test uses two binaries, and only one of them was built with `crdb_test` due to how we add `1e6` to versions in test binaries. Closes cockroachdb#95648 Epic: none Release note: None
Previously, running create_tenant on the latest binary in
a mixed-version cluster would bootstrap the system schema using the
logic from the latest binary, in which all system database migration
upgrade steps have been "baked in".
This might not be compatible with the the tenant cluster's version which
is (correctly) set to the active host cluster version, the idea there
being that tenant clusters versions cannot be greater than host cluster
versions.
This commit fixes this by version-gating the tenant cluster system
schema bootstrapping, and using hard-coded values when bootstrapping
into the old version.
Informs #94773.
Release note: None