sql/schemachanger: Deprecate ALTER COLUMN TYPE rewrite in the legacy schema changer #136110
Conversation
spilchen
requested review from
nameisbhaskar,
vidit-bhat and
asg0451
and removed request for
a team
|
This change is |
There was a problem hiding this comment.
Reviewed 18 of 19 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @asg0451, @nameisbhaskar, @spilchen, and @vidit-bhat)
pkg/sql/alter_column_type.go line 238 at r1 (raw file):
// to ensure consistent error messages for scenarios that overlap with the DSC. if params.p.execCfg.Settings.Version.IsActive(ctx, clusterversion.V25_1) { return pgerror.New(pgcode.FeatureNotSupported,
Can you confirm that we don't have any fallbacks for regional by row tables with the declarative schema changer? Trying to make sure we don't expose any regression here.
pkg/ccl/changefeedccl/cdctest/row.go line 35 at r1 (raw file):
t testing.TB, execCfgI interface{}, desc catalog.TableDescriptor, ) (func(t testing.TB) *kvpb.RangeFeedValue, func()) { reader, cleanup := MakeRangeFeedValueReaderExtended(t, execCfgI, desc)
This seems unrelated?
…schema changer This change deprecates the legacy schema changer for ALTER COLUMN TYPE operations that require a column rewrite. While the legacy schema changer remains available for mixed-version clusters, starting in version 25.1, these operations must use the declarative schema changer. Additionally, this update includes the following improvements: - Updated error messages for blocking conditions to clarify that they apply only when the ALTER COLUMN TYPE requires a column rewrite. Each error now includes a hint for resolving the issue. - Enforced that the sql_safe_updates setting must be disabled to perform operations requiring a column rewrite. This is necessary to prevent potential data loss when converting between DECIMAL or TIMESTAMP data types or when using a USING expression. Epic: CRDB-25314 Informs: cockroachdb#49329 Release note (sql change): The sql_safe_updates setting must be disabled to perform ALTER COLUMN TYPE operations that require a column rewrite.
spilchen
force-pushed
the
gh-49329/241125/0958/rev-prep
branch
from
b7ecf83 to
6ebb172
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/changefeedccl/cdctest/row.go line 35 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
This seems unrelated?
This change is related to the test update in event_test.go. With the legacy schema changer now blocked, the test needed adjustment. Since a column rewrite involves dropping the old column, it triggered a delete range event that the test wasn’t accounting for. This update enables the test to detect and handle delete range events by returning the relevant state.
pkg/sql/alter_column_type.go line 238 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Can you confirm that we don't have any fallbacks for regional by row tables with the declarative schema changer? Trying to make sure we don't expose any regression here.
I confirmed that there are no fallbacks. Since I couldn’t find any existing tests, I added a logic test.
There was a problem hiding this comment.
Reviewed 1 of 19 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:
|
TFTR! bors r+ |
136110: sql/schemachanger: Deprecate ALTER COLUMN TYPE rewrite in the legacy schema changer r=spilchen a=spilchen This change deprecates the legacy schema changer for ALTER COLUMN TYPE operations that require a column rewrite. While the legacy schema changer remains available for mixed-version clusters, starting in version 25.1, these operations must use the declarative schema changer. Additionally, this update includes the following improvements: - Updated error messages for blocking conditions to clarify that they apply only when the ALTER COLUMN TYPE requires a column rewrite. Each error now includes a hint for resolving the issue. - Enforce that the sql_safe_updates setting must be disabled to perform operations requiring a column rewrite. This is necessary to prevent potential data loss when converting between DECIMAL or TIMESTAMP data types or when applying a USING expression. Epic: CRDB-25314 Informs: #49329 Release note (sql change): The sql_safe_updates setting must be disabled to perform ALTER COLUMN TYPE operations that require a column rewrite. 136177: kvserver: bring TestRaftPreVote to a leader leases world r=arulajmani a=arulajmani See individual commits. 136179: kvserver: take advantage of fortification in TestQuotaPool r=iskettaneh a=arulajmani This test was using an extremely high number for RaftElectionTimeoutTicks to keep leadership sticky. This doesn't work with leader leases, as they require store liveness support to establish leadership, and requests for support are only kicked off on the first tick. Conveniently, fortification is yet another way to make leadership sticky -- so let's use that here instead. References #133763 Release note: None Co-authored-by: Matt Spilchen <[email protected]> Co-authored-by: Arul Ajmani <[email protected]>
|
Build failed (retrying...): |
This change deprecates the legacy schema changer for ALTER COLUMN TYPE operations that require a column rewrite. While the legacy schema changer remains available for mixed-version clusters, starting in version 25.1, these operations must use the declarative schema changer.
Additionally, this update includes the following improvements:
Epic: CRDB-25314
Informs: #49329
Release note (sql change): The sql_safe_updates setting must be disabled to perform ALTER COLUMN TYPE operations that require a column rewrite.