migrations: add target column to system.pts_records#74281
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Jan 5, 2022
Merged
migrations: add target column to system.pts_records#74281craig[bot] merged 1 commit intocockroachdb:masterfrom
craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Member
|
This change is |
Contributor
Author
|
First commit is from #74211. |
b963f58 to
a5057c6
Compare
This was referenced Dec 28, 2021
a5057c6 to
91e7bad
Compare
91e7bad to
ce46759
Compare
Contributor
Author
|
@arulajmani @irfansharif rebased and RFAL! |
ajwerner
approved these changes
Jan 4, 2022
Contributor
ajwerner
left a comment
There was a problem hiding this comment.
LGTM
Note also in the PR message that the target column will store an encoded protocol buffer?
pkg/migration/migrations/alter_table_protected_timestamp_records.go
Outdated
Show resolved
Hide resolved
irfansharif
approved these changes
Jan 4, 2022
Contributor
irfansharif
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @arulajmani)
pkg/migration/migrations/alter_table_protected_timestamp_records_test.go, line 64 at r1 (raw file):
// Inject the old copy of the descriptor. migrations.InjectLegacyTable(ctx, t, s, systemschema.ProtectedTimestampsRecordsTable, getDeprecatedProtectedTimestampRecordsDescriptor)
[nit] Line wrap.
pkg/migration/migrations/alter_table_protected_timestamp_records_test.go, line 66 at r1 (raw file):
migrations.InjectLegacyTable(ctx, t, s, systemschema.ProtectedTimestampsRecordsTable, getDeprecatedProtectedTimestampRecordsDescriptor) // Validate that the protected timestamp records table has the old schema. migrations.ValidateSchemaExists(
[nit] Wrap this in a tiny helper func:
validateSchemaExists := func (exp bool) { ...}
// ...
validateSchemaExists(false)
migrations.Migrate(...)
validateSchemaExists(true)
pkg/migration/migrations/alter_table_protected_timestamp_records_test.go, line 99 at r1 (raw file):
} // getDeprecatedTableStatisticsDescriptor returns the system.table_statistics
Comment needs to be updated.
ce46759 to
9c2e0c8
Compare
adityamaru
commented
Jan 5, 2022
Contributor
Author
adityamaru
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/migration/migrations/alter_table_protected_timestamp_records_test.go, line 64 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
[nit] Line wrap.
Done.
pkg/migration/migrations/alter_table_protected_timestamp_records_test.go, line 66 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
[nit] Wrap this in a tiny helper func:
validateSchemaExists := func (exp bool) { ...} // ... validateSchemaExists(false) migrations.Migrate(...) validateSchemaExists(true)
Done.
pkg/migration/migrations/alter_table_protected_timestamp_records_test.go, line 99 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Comment needs to be updated.
Done.
This change adds a `BYTES` column to the system.pts_records table, that will store an encoded protocol buffer representing the `Target` on a protected timestamp record. We do not populate the column in this PR, that will be done in a followup. Additionally, we add a tenant migration to `ALTER TABLE ... ADD COLUMN` to run on the upgrade path for older clusters. Informs: cockroachdb#73727 Release note (sql change): `system.protected_timestamp_records` table now has an additional `target` column that will store an encoded protobuf that represents the target a record protects. This target can either be the entire cluster, tenants, or schema objects (databases/tables).
9c2e0c8 to
cdb829c
Compare
Contributor
Author
|
TFTR! bors r=ajwerner,irfansharif |
craig bot
pushed a commit
that referenced
this pull request
Jan 5, 2022
74281: migrations: add target column to system.pts_records r=ajwerner,irfansharif a=adityamaru This change adds a `BYTES` column to the system.pts_records table, that will store an encoded protocol buffer representing the `Target` on a protected timestamp record. We do not populate the column in this PR, that will be done in a followup. Additionally, we add a tenant migration to `ALTER TABLE ... ADD COLUMN` to run on the upgrade path for older clusters. Informs: #73727 Release note (sql change): `system.protected_timestamp_records` table now has an additional `target` column that will store an encoded protobuf that represents the target a record protects. This target can either be the entire cluster, tenants, or schema objects (databases/tables). Co-authored-by: Aditya Maru <adityamaru@gmail.com>
Contributor
|
Build failed: |
Contributor
Author
|
TestComposeGSS flake bors r=ajwerner,irfansharif |
craig bot
pushed a commit
that referenced
this pull request
Jan 5, 2022
74281: migrations: add target column to system.pts_records r=ajwerner,irfansharif a=adityamaru This change adds a `BYTES` column to the system.pts_records table, that will store an encoded protocol buffer representing the `Target` on a protected timestamp record. We do not populate the column in this PR, that will be done in a followup. Additionally, we add a tenant migration to `ALTER TABLE ... ADD COLUMN` to run on the upgrade path for older clusters. Informs: #73727 Release note (sql change): `system.protected_timestamp_records` table now has an additional `target` column that will store an encoded protobuf that represents the target a record protects. This target can either be the entire cluster, tenants, or schema objects (databases/tables). 74418: authors: add sherman-grewal to authors r=sherman-grewal a=sherman-grewal Release note: None Co-authored-by: Aditya Maru <adityamaru@gmail.com> Co-authored-by: Sherman Grewal <sherman.grewal@cockroachlabs.com>
Contributor
|
Build failed (retrying...): |
Contributor
|
Build succeeded: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change adds a
BYTEScolumn to the system.pts_recordstable, that will store an encoded protocol buffer
representing the
Targeton a protected timestamprecord. We do not populate the column in this PR, that will
be done in a followup.
Additionally, we add a tenant migration to
ALTER TABLE ... ADD COLUMNto run on the upgrade path for older clusters.
Informs: #73727
Release note (sql change):
system.protected_timestamp_recordstablenow has an additional
targetcolumn that will store an encodedprotobuf that represents the target a record protects. This target
can either be the entire cluster, tenants, or schema objects
(databases/tables).