*: enable declarative schema changer #76944
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
force-pushed
the
enable-declarative-schema-changer
branch
from
e43ce18 to
79c1178
Compare
postamar
requested review from
shermanCRL
and removed request for
a team and
shermanCRL
postamar
force-pushed
the
enable-declarative-schema-changer
branch
2 times, most recently
from
6237a46 to
7904823
Compare
fqazi
approved these changes
Feb 24, 2022
There was a problem hiding this comment.
Reviewed 11 of 23 files at r1, 56 of 56 files at r14, 2 of 2 files at r15, 6 of 6 files at r16, 1 of 1 files at r17, 10 of 10 files at r18, 2 of 2 files at r19, 1 of 1 files at r20, 8 of 8 files at r21, 11 of 11 files at r22, 5 of 5 files at r23, 17 of 17 files at r24, 1 of 1 files at r25, 1 of 1 files at r26, 11 of 11 files at r27, 38 of 38 files at r28, 3 of 3 files at r29, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @postamar)
postamar
force-pushed
the
enable-declarative-schema-changer
branch
from
7904823 to
f27d76a
Compare
|
Thanks for the review! I bumped the TESTIMEOUT variable in the Makefile to 60 minutes, evidently 50 minutes instead of the original 45 doesn't cut it either. We can worry about this later. bors r+ |
|
Build failed (retrying...): |
|
Build failed: |
Release note: None
Previously, the declarative schema changer fetched information about tables in the transaction for ALTER TABLE before checking if the commands were supported. This was problematic because it could change timing/retry behaviour in certain cases, since we would do potentially extra descriptor look ups. To address this, this patch checks if ALTER TABLE commands are supported first to avoid intermittent failures caused by extra round trips. Release note: None
Previously, the declarative schema changer was disabled by default. We are enabling it by default, so some logic test updates are needed. This patch will update logictest expected files. Release note: None
Previously, the declarative schema changer did not return the correct error when attempting to modify pg_catalog tables. To address this, this patch will return the correct error when trying to modify pg_catalog tables. Release note: None
Previously, the declarative schema changer was enabled by default, which could cause certain tests designed for the legacy schema changer to fail. To address this, this patch individual disables declarative schema changer on tests designed for legacy schema changer. Release note: None
Previously, the message generated for drop types that were not allowed due to dependencies was had a period. This patch will update the message to match the legacy schema changer. Release note: None
Previously, the declarative schema changer did not generate the correct messages when attempting to drop/alter multiregion enums. To address this patch will update the message generated for multiregion enums to include a hint to indicate that ALTER DATABASE should be used for such modifications. Release note: None
Previously, the declarative schema changer did not support KV tracing. This was inadequate because the legacy schema changer supported this functionality which is needed for supportability of the product. To address this, this patch will add support for KV tracing inside the declarative schema changer. Release note: None
Previously, when dropping a database descriptor, the decalrative schema changer never deleted database descriptors properly. This led to the descriptors being left behind, instead of fully being cleaned up. To address, this patch will delete database descriptors during DROP DATABASE. Release note: None
Previously, the declarative schema changer did not populate the drop time when dropping tables. This could be problematic when looking at crdb_internal tables and other scenarios that detect if a table is dropped based on this time. To address this, this patch starts populating the DropTime inside the declarative schema changer. Release note: None
This commit ensures that the declarative schema changer properly increments telemetry counters. Release note: None
Previously, there the enums logictest with the declarative schema changer code paths enabled could intermittently fail with retry errors. The design of the test is such that we execute SELECT queries and schema changes in the same transaction, so automatic retries are not possible once the result set has been sent to the client. To address this, this patch will workaround the issue by disabling declarative schema changer for this test. Release note: None
Previously, we were right at the timeout boundary for the TenantLogicTest. With the declarative schema changer enabled we are just past the 45 minute mark causing this test to fail. This patch bumps up the test timeout to 50 minutes to allow for breathing room. Release note: None
This commit adds more data-driven tests to cover CCL features support by the declarative schema changer. Release note: None
This commit replaces the scpb.TableLocality element with its four constituent subtypes: - global, - regional by row, - regional by table in the primary region, - regional by table in another region, The last of these is special in that it holds a reference to the multi-region enum type ID. Removing it therefore entails removing the back-reference to the table in the type descriptor. Release note: None
This test was failing due to the declarative schema changer's GC job name being different from that of the legacy schema changer's. Release note: None
This is now superfluous, as the declarative schema changer is enabled by default. Release note: None
postamar
force-pushed
the
enable-declarative-schema-changer
branch
from
f27d76a to
3a05e87
Compare
|
I added a commit which removes the |
|
Hmm the more I look into it, the more those bors failures seem like unrelated flakes. I feel like a baby trying to ram a square peg through a round hole and not understanding why it's not going through, but... bors r+ |
|
Build succeeded: |
|
Oh, what a relief. It's done. |
24 tasks
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.
This change enables the declarative schema changer by default. This PR originated as #76610 by @fqazi which I rebased following the merging of #76776. Only mild alterations were needed, these commits can therefore be considered as having been reviewed.