upgrades: fix upgrade to add statement_diagnostics_requests.completed… #93487
Conversation
This file contains 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
|
This change is |
ajwerner
commented
Dec 13, 2022
There was a problem hiding this comment.
Oh, this roachtest change doesn't work because the snapshots are for a 3-node cluster
Reviewable status:
complete! 0 of 0 LGTMs obtained
ajwerner
force-pushed
the
ajwerner/fix-bad-migration
branch
from
0ecb06c
to
6ff6ea6
Compare
|
The roachtest will fail for the backport. |
…_idx The migration used a different column ordering than the descriptor in the bootstrap schema. The value in the bootstrap schema is the value used to determine whether the migration succeeded successfully. In general, you can hit this bug if you upgrade from 22.1->22.2 and then you create the index with the migration but crash before the index is fully created. In that case, the code will think that it's the wrong index. This should be rare, but would be problematic. Now we've made them match. This change also fixes the roachtest which checks that the system schema looks correct to check on what happens when you upgrade from a previous snapshot. The problem with the test is that it read the strings before they were assigned. Fixes cockroachdb#93133 Release note (bug fix): Fixed a rare bug which could cause upgrades from 22.1 to 22.2 to fail if the job coordinator node crashes in the middle of a specific upgrade migration.
ajwerner
force-pushed
the
ajwerner/fix-bad-migration
branch
from
6ff6ea6
to
2a118aa
Compare
|
@Xiang-Gu any interest in reviewing this one? |
Xiang-Gu
approved these changes
Dec 13, 2022
|
TFTR! bors r+ |
|
Build succeeded: |
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
Dec 14, 2022
Before this change, the test fails with:
```
Diff:
@@ -29,11 +29,11 @@
"lastUpdated" TIMESTAMP NOT NULL DEFAULT now():::TIMESTAMP,
"valueType" STRING NULL,
CONSTRAINT "primary" PRIMARY KEY (name ASC),
FAMILY "fam_0_name_value_lastUpdated_valueType" (name, value, "lastUpdated", "valueType")
);
-CREATE SEQUENCE public.descriptor_id_seq MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 1;
+CREATE SEQUENCE public.descriptor_id_seq MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 104;
CREATE TABLE public.tenants (
id INT8 NOT NULL,
active BOOL NOT NULL DEFAULT true,
info BYTES NULL,
name STRING NULL AS (crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo':::STRING, info)->>'name':::STRING) VIRTUAL,
----
```
This was revealed by cockroachdb#93487 which fixed the broken roachtest. Expect a few more
of these.
Fixes: cockroachdb#93602
Release note: None
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
Dec 14, 2022
Before this change, the test fails with:
```
Diff:
@@ -29,11 +29,11 @@
"lastUpdated" TIMESTAMP NOT NULL DEFAULT now():::TIMESTAMP,
"valueType" STRING NULL,
CONSTRAINT "primary" PRIMARY KEY (name ASC),
FAMILY "fam_0_name_value_lastUpdated_valueType" (name, value, "lastUpdated", "valueType")
);
-CREATE SEQUENCE public.descriptor_id_seq MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 1;
+CREATE SEQUENCE public.descriptor_id_seq MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 104;
CREATE TABLE public.tenants (
id INT8 NOT NULL,
active BOOL NOT NULL DEFAULT true,
info BYTES NULL,
name STRING NULL AS (crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo':::STRING, info)->>'name':::STRING) VIRTUAL,
----
```
This was revealed by cockroachdb#93487 which fixed the broken roachtest. Expect a few more
of these.
Fixes: cockroachdb#93602
Release note: None
craig bot
pushed a commit
that referenced
this pull request
Dec 14, 2022
93610: upgrades: fix desc ID sequence definition to match expectation r=ajwerner a=ajwerner
Before this change, the test fails with:
```
Diff:
`@@` -29,11 +29,11 `@@`
"lastUpdated" TIMESTAMP NOT NULL DEFAULT now():::TIMESTAMP,
"valueType" STRING NULL,
CONSTRAINT "primary" PRIMARY KEY (name ASC),
FAMILY "fam_0_name_value_lastUpdated_valueType" (name, value, "lastUpdated", "valueType")
);
-CREATE SEQUENCE public.descriptor_id_seq MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 1;
+CREATE SEQUENCE public.descriptor_id_seq MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 104;
CREATE TABLE public.tenants (
id INT8 NOT NULL,
active BOOL NOT NULL DEFAULT true,
info BYTES NULL,
name STRING NULL AS (crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo':::STRING, info)->>'name':::STRING) VIRTUAL,
----
```
This was revealed by #93487 which fixed the broken roachtest. Expect a few more of these.
Fixes: #93602
Release note: None
93619: ui: preserve backwards compatibility for managed-service r=matthewtodd a=matthewtodd
These components were renamed in #91860, but the support in managed-service for loading multiple versions of cluster-ui doesn't like it when we do that; it expects changes to be purely additive.
We choose here to provide aliases to the old names, rather than undoing the rename completely, so that we can eventually switch managed-service over to the new names once the old versions have fallen out of the support window.
Epic: none
Release note: None
93622: bulkpb: move backuppb.IngestionPerformanceStats out of ccl r=ajwerner a=ajwerner
This message was relied on by kv code. Nobody used the message at its old path. It's just used for trace events. This PR just moves its code to `kv/bulk/bulkpb` from `ccl/backupccl/backuppb`.
Part of #91714
Epic: none
Release note: None
Co-authored-by: Andrew Werner <awerner32@gmail.com>
Co-authored-by: Matthew Todd <todd@cockroachlabs.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…_idx
The migration used a different column ordering than the descriptor in the bootstrap schema. The value in the bootstrap schema is the value used to determine whether the migration succeeded successfully. In general, you can hit this bug if you upgrade from 22.1->22.2 and then you create the index with the migration but crash before the index is fully created. In that case, the code will think that it's the wrong index. This should be rare, but would be problematic. Now we've made them match.
This change also augments the roachtest which checks that the system schema looks correct to check on what happens when you upgrade from a previous snapshot. That matters here because the migration in question still exists on master, and is not idempotent. We should have found that, but didn't because we need multiple steps in the upgrade. We can get that pretty cheaply.
Fixes #93133
Release note (bug fix): Fixed a rare bug which could cause upgrades from 22.1 to 22.2 to fail if the job coordinator node crashes in the middle of a specific upgrade migration.