-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql: move alter column type general support out of experimental #49329
Comments
Hi @RichardJCai, I've guessed the C-ategory of your issue and suitably labeled it. Please re-label if inaccurate. While you're here, please consider adding an A- label to help keep our repository tidy. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
When running Django's test suite, I'm seeing FeatureNotSupported errors pointing to this issue that weren't present before 20.2 alpha 2.
Is this a regression? I'll provide the exact SQL statements if needed. |
Yeah this is a regression, while adding experimental support for other cases of alter column type, I wrapped all uses of alter column type with the error, will fix this soon. |
Actually can you provide the statements please? Curious if you're providing an expression to alter the column type or not. |
ALTER TABLE "schema_author" ALTER COLUMN "name" TYPE text USING "name"::text;
ALTER TABLE "schema_uniquetest" ALTER COLUMN "year" TYPE bigint USING "year"::bigint;
ALTER TABLE "schema_smallintegerpk" ALTER COLUMN "i" TYPE smallint USING "i"::smallint; |
@timgraham actually this isn't necessarily a regression - previously the logic did not take into account the USING expression at all. Now if any expression is provided, we force a change to happen. The examples you gave, the expression is ignored and the ALTER TABLE is a no-op. How important is this? |
Maybe Django doesn't need to add a USING clause (that code was copied from the PostgreSQL backend). Removing that resolves the crashes. diff --git a/django_cockroachdb/schema.py b/django_cockroachdb/schema.py
index 11d90dc..ca2a9da 100644
--- a/django_cockroachdb/schema.py
+++ b/django_cockroachdb/schema.py
@@ -44,9 +44,6 @@ class DatabaseSchemaEditor(PostgresDatabaseSchemaEditor):
def _alter_column_type_sql(self, model, old_field, new_field, new_type):
self.sql_alter_column_type = 'ALTER COLUMN %(column)s TYPE %(type)s'
- # Cast when data type changed.
- if self._field_data_type(old_field) != self._field_data_type(new_field):
- self.sql_alter_column_type += ' USING %(column)s::%(type)s'
# Make ALTER TYPE with SERIAL make sense.
# table = strip_quotes(model._meta.db_table)
serial_fields_map = {'bigserial': 'bigint', 'serial': 'integer', 'smallserial': 'smallint'} Incidentally, I added "SET enable_experimental_alter_column_type_general = true" and enabled some type change tests without issue. The remaining type conversion test failures are covered by #47636. |
For what it's worth, here are the failures on PostgreSQL when I remove the USING clause. I guess CockroachDB does these casts implicitly.
|
Added the following two items to the bullet list at top, as per request from @dbist 👍
|
Any hope? |
Prior to this change, in order to alter a column’s type that needed a backfill the setting enable_experimental_alter_column_type_general had to be set. This removes that restriction now that we built out ALTER COLUMN TYPE support in the DSC. We still check the setting in mixed-version workloads since it depends on 25.1 dep rules. And when using the legacy schema changer. Epic: CRDB-25314 Informs cockroachdb#49329 Release note (general): Altering a column’s type no longer requires enabling the enable_experimental_alter_column_type_general setting. This change makes the feature generally available.
Previously, altering a column’s type requiring a backfill required the enable_experimental_alter_column_type_general setting to be enabled. This restriction has been removed, as full support for ALTER COLUMN TYPE has been implemented in the Declarative Schema Changer (DSC). The setting is still checked in mixed-version clusters, as the operation relies on 25.1 dependency rules, and when using the legacy schema changer. Epic: CRDB-25314 Informs cockroachdb#49329 Release note (general): Altering a column’s type no longer requires enabling the enable_experimental_alter_column_type_general setting. This change makes the feature generally available.
Previously, altering a column’s type requiring a backfill required the enable_experimental_alter_column_type_general setting to be enabled. This restriction has been removed, as full support for ALTER COLUMN TYPE has been implemented in the Declarative Schema Changer (DSC). The setting is still checked in mixed-version clusters, as the operation relies on 25.1 dependency rules, and when using the legacy schema changer. Epic: CRDB-25314 Informs cockroachdb#49329 Release note (general change): Altering a column’s type no longer requires enabling the enable_experimental_alter_column_type_general setting. This change makes the feature generally available.
Previously, altering a column’s type requiring a backfill required the enable_experimental_alter_column_type_general setting to be enabled. This restriction has been removed, as full support for ALTER COLUMN TYPE has been implemented in the Declarative Schema Changer (DSC). The setting is still checked in mixed-version clusters, as the operation relies on 25.1 dependency rules, and when using the legacy schema changer. Epic: CRDB-25314 Informs cockroachdb#49329 Release note (sql change): Altering a column’s type no longer requires enabling the enable_experimental_alter_column_type_general setting. This change makes the feature generally available.
Previously, altering a column’s type requiring a backfill required the enable_experimental_alter_column_type_general setting to be enabled. This restriction has been removed, as full support for ALTER COLUMN TYPE has been implemented in the Declarative Schema Changer (DSC). The setting is still checked in mixed-version clusters, as the operation relies on 25.1 dependency rules, and when using the legacy schema changer. Epic: CRDB-25314 Informs cockroachdb#49329 Release note (sql change): Altering a column’s type no longer requires enabling the enable_experimental_alter_column_type_general setting. This change makes the feature generally available.
135936: sql/schemachanger: Remove feature gate for ALTER COLUMN TYPE r=spilchen a=spilchen Previously, altering a column’s type requiring a backfill required the enable_experimental_alter_column_type_general setting to be enabled. This restriction has been removed, as full support for ALTER COLUMN TYPE has been implemented in the Declarative Schema Changer (DSC). The setting is still checked in mixed-version clusters, as the operation relies on 25.1 dependency rules, and when using the legacy schema changer. Epic: CRDB-25314 Informs #49329 Release note (sql change): Altering a column’s type no longer requires enabling the enable_experimental_alter_column_type_general setting. This change makes the feature generally available. Co-authored-by: Matt Spilchen <[email protected]>
…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.
…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.
This introduces a block that prevents running an ALTER COLUMN TYPE operation requiring a backfill within a transaction. To support the schemachanger end-to-end tests, which need to execute ALTERs in a transaction, I added a session variable to override this default behavior. Epic: CRDB-25314 Informs cockroachdb#49329 Release note: none
…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.
This introduces a block that prevents running an ALTER COLUMN TYPE operation requiring a backfill within a transaction. To support the schemachanger end-to-end tests, which need to execute ALTERs in a transaction, I added a session variable to override this default behavior. Epic: CRDB-25314 Informs cockroachdb#49329 Release note: none
This introduces a block that prevents running an ALTER COLUMN TYPE operation requiring a backfill within a transaction. To support the schemachanger end-to-end tests, which need to execute ALTERs in a transaction, I added a session variable to override this default behavior. Epic: CRDB-25314 Informs cockroachdb#49329 Release note: none
This introduces a block that prevents running an ALTER COLUMN TYPE operation requiring a backfill within a transaction. To support the schemachanger end-to-end tests, which need to execute ALTERs in a transaction, I added a session variable to override this default behavior. Epic: CRDB-25314 Informs cockroachdb#49329 Release note: none
136178: kvserver: run TestReplicateRestartAfterTruncation with leader leases r=arulajmani a=arulajmani This test was setting an extremely high RaftElectionTimeoutTicks value, but this doesn't work with raft fortification. That's because we won't campaign until StoreLiveness support is established, and the first time a replica is ticked we won't have this. The high RaftElectionTimeoutTicks in this test wasn't useful in practice, so let's just remove it. Note that this change also applies to the ReAdd version of the test. References #133763 Release note: None 136207: sql/schemachanger: Block explicit transactions for ALTER COLUMN TYPE r=spilchen a=spilchen This introduces a block that prevents running an ALTER COLUMN TYPE operation requiring a backfill within a transaction. To support the schemachanger end-to-end tests, which need to execute ALTERs in a transaction, I added a session variable to override this default behavior. Epic: CRDB-25314 Informs #49329 Release note: none Co-authored-by: Arul Ajmani <[email protected]> Co-authored-by: Matt Spilchen <[email protected]>
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]>
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. 136156: kvserver: move RaftTruncatedState from ReplicaState r=tbg a=pav-kv The `RaftTruncatedState` is in the unreplicated rangeID-local space. It logically belongs to the raft log storage, and is separate from the replicated state machine state. Before this PR, `RaftTruncatedState` was mixed into the `ReplicaState` struct which contains replicated state such as the range descriptor. After this PR, `Replica` tracks the `RaftTruncatedState` separately from `ReplicaState`: - `StateLoader.{Save,Load}` methods ignore this field. It is now saved and loaded using the `RaftTruncatedState` accessors. - `ReplicaState.TruncatedState` is always nil in memory. - The only place where the `ReplicaState.TruncatedState` remains used is the `ReplicatedEvalResult.State`. This is needed for compatibility with older versions, because the log may still contain historical proposals. - However, all new proposals evaluate to a newly introduced `ReplicatedEvalResult.RaftTruncatedState` field. This change is version-gated. The last remaining use of `ReplicaState.TruncatedState` needs to be phased out with a migration, before the field can be fully removed. All historical entries need to be applied/removed, before it is safe. Part of #97613 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: Pavel Kalinnikov <[email protected]> Co-authored-by: Arul Ajmani <[email protected]>
This serves as the final cleanup for the MVP implementation of ALTER COLUMN TYPE.
Jira issue: CRDB-4238
Epic CRDB-25314
The text was updated successfully, but these errors were encountered: