schemachanger,scbuild: fix assertion bug #106175
Merged
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 |
postamar
commented
Jul 5, 2023
| @@ -198,16 +198,11 @@ func (p *planner) waitForDescriptorSchemaChanges( | |||
|
|
|||
| // Wait for the descriptor to no longer be claimed by a schema change. | |||
| start := timeutil.Now() | |||
| logEvery := log.Every(30 * time.Second) | |||
| logEvery := log.Every(10 * time.Second) | |||
There was a problem hiding this comment.
10s is more convenient than 30s when running tests and tailing the logs
postamar
commented
Jul 5, 2023
| "schema change waiting for concurrent schema changes on descriptor %d,"+ | ||
| " waited %v so far", descID, timeutil.Since(start), | ||
| ) | ||
| } |
There was a problem hiding this comment.
moving this to the end of the for-loop makes more sense, so that no message is logged if we're not actually waiting
postamar
added
backport-22.2.x
backport-23.1.x
postamar
force-pushed
the
fix-45510
branch
3 times, most recently
from
320d051 to
0d6b962
Compare
The declarative schema changer builder contains assertions on the state of elements and their targets and these could sometimes fail because the check which verifies that the corresponding descriptors are not already undergoing a schema change was performed later. This commit fixes this by moving the check earlier so the assertion is never reached in those cases. This commit removes a skipped test which is superseded by a new test in the schemachanger package. Fixes: cockroachdb#45510 Fixes: cockroachdb#102927 Fixes: cockroachdb#105754 Release note (bug fix): fixed a bug which manifested itself in error messages containing "failed to drop all of the relevant elements" when executing DDL statements with the declarative schema changer. What this really means is that there's a concurrent schema change that's ongoing. Instead we now behave as expected and wait for it to finish.
|
I will backport this manually to 23.1 and 22.2. |
postamar
requested review from
michae2
and removed request for
a team and
michae2
chengxiong-ruan
approved these changes
Jul 6, 2023
|
Thanks for the review! bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
postamar
pushed a commit
to postamar/cockroach
that referenced
this pull request
Jul 17, 2023
Previously, we didn't check for concurrent schema changes on targets set on elements that the builder state didn't know about yet. This patch fixes this oversight. This regression was recently introduced by cockroachdb#106175. Fixes: cockroachdb#106487 Release note: None
postamar
pushed a commit
to postamar/cockroach
that referenced
this pull request
Jul 17, 2023
Previously, we didn't check for concurrent schema changes on targets set on elements that the builder state didn't know about yet. This patch fixes this oversight. This regression was recently introduced by cockroachdb#106175. Fixes: cockroachdb#106487 Release note: None
postamar
pushed a commit
to postamar/cockroach
that referenced
this pull request
Jul 18, 2023
Previously, we didn't check for concurrent schema changes on targets set on elements that the builder state didn't know about yet. This patch fixes this oversight. This regression was recently introduced by cockroachdb#106175. Fixes: cockroachdb#106487 Release note: None
craig bot
pushed a commit
that referenced
this pull request
Jul 19, 2023
106869: sql: fix CREATE OR REPLACE VIEW cross-db type reference errors r=mgartner a=mgartner Fixes #106602 Release note (bug fix): A bug has been fixed that allowed views created with `CREATE OR REPLACE VIEW` to reference user-defined types in other databases, even with `sql.cross_db_views.enabled` set to `false`. This bug was present since user-defined types were introduced in version 20.1. 106933: scbuild: fix concurrent schema change verification bug r=postamar a=postamar Previously, we didn't check for concurrent schema changes on targets set on elements that the builder state didn't know about yet. This patch fixes this oversight. This regression was recently introduced by #106175. Fixes: #106487 Release note: None 107184: changefeedccl: remove disableDeclarativeSchemaChangesForTest from two tests r=miretskiy a=jayshrivastava This change removes the `disableDeclarativeSchemaChangesForTest` flag from two changefeed tests and modifies them to match the new behavior. There are two ways in which the tests are modified: 1. The timestamp associated with the rows we expect to see during a backfill are different with the declarative schema changer. Ex. for ADD COLUMN, we expect rows with the `ModificationTime` of the descriptor at version 7 instead of 5. 2. With the new schema changer, we expect to see backfill rows only once. With the old schema changer, we would see KV's be re-written in-place while the changefeed was running (one "backfill"). Then, when schema change becomes "visible", we would stop the changefeed to perform another backfill at the schema change timestamp before moving on. With the new schema changer, the KVs are not re-written in place, so they are not seen by the changefeed until the schema change becomes "visible". When the schema change becomes visible, the changefeed stops to emit all the backfill rows before continuing. Informs: #106906 Epic: None Release note: None Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Marius Posta <[email protected]> Co-authored-by: Jayant Shrivastava <[email protected]>
postamar
pushed a commit
to postamar/cockroach
that referenced
this pull request
Jul 21, 2023
Previously, we didn't check for concurrent schema changes on targets set on elements that the builder state didn't know about yet. This patch fixes this oversight. This regression was recently introduced by cockroachdb#106175. Fixes: cockroachdb#106487 Release note: None
postamar
pushed a commit
to postamar/cockroach
that referenced
this pull request
Jul 21, 2023
Previously, we didn't check for concurrent schema changes on targets set on elements that the builder state didn't know about yet. This patch fixes this oversight. This regression was recently introduced by cockroachdb#106175. Fixes: cockroachdb#106487 Release note: None
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.
The declarative schema changer builder contains assertions on the state of elements and their targets and these could sometimes fail because the check which verifies that the corresponding descriptors are not already undergoing a schema change was performed later. This commit fixes this by moving the check earlier so the assertion is never reached in those cases.
This commit removes a skipped test which is superseded by a new test in the schemachanger package.
Fixes: #45510
Fixes: #102927
Fixes: #105754
Release note (bug fix): fixed a bug which manifested itself in error messages containing "failed to drop all of the relevant elements" when executing DDL statements with the declarative schema changer. What this really means is that there's a concurrent schema change that's ongoing. Instead we now behave as expected and wait for it to finish.