sql/schemachanger: Prevent Temporary Disappearance of Altered Columns #134411
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
There was a problem hiding this comment.
Please add the deps command to: pkg/sql/schemachanger/scplan/testdata/alter_table_alter_column_type so that we can confirm which rules are firing as well.
Reviewed 57 of 57 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @spilchen)
pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_column.go line 230 at r1 (raw file):
// This rule is similar to the previous one but applies specifically to ALTER COLUMN ... TYPE operations. // It uses SameStagePrecedence to enable the swapping of dropped and added columns within the same stage.
Just as a sanity what happens to the plan without SameStagePrecedence?
pkg/sql/schemachanger/scplan/internal/rules/current/dep_add_column.go line 21 at r1 (raw file):
registerDepRule( "column existence precedes column dependents",
Nit: could we reduce duplication here some how by having a helper function?
registerDepRule(
"column existence precedes column dependents",
append(ColumnExistencePrecedes(isColumnDependentExcept),
IsNotAlterColumnTypeOp("table-id", "col-id"))
spilchen
force-pushed
the
issue-133996/final-rev-rebase
branch
from
59a59b3
to
f0cb08b
Compare
There was a problem hiding this comment.
Thanks for taking a look.
Please add the deps command to: pkg/sql/schemachanger/scplan/testdata/alter_table_alter_column_type so that we can confirm which rules are firing as well.
I did have to update pkg/sql/schemachanger/scplan/testdata/alter_table_alter_column_type. You can see that I had to make changes to it because the column name is being set in a later stage.
Reviewable status:
pkg/sql/schemachanger/scplan/internal/rules/current/dep_add_column.go line 21 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Nit: could we reduce duplication here some how by having a helper function?
registerDepRule(
"column existence precedes column dependents",
append(ColumnExistencePrecedes(isColumnDependentExcept),
IsNotAlterColumnTypeOp("table-id", "col-id"))
Done
pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_column.go line 230 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Just as a sanity what happens to the plan without SameStagePrecedence?
Without this change, the old column transitions out of the public state (i.e., becomes invisible to end users) in the previous stage, but the new column has not yet been made public. This causes any query for the column during that stage to fail with column "<col-name>" does not exist.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status:
|
TFTR @fqazi bors r+ |
134411: sql/schemachanger: Prevent Temporary Disappearance of Altered Columns r=spilchen a=spilchen Previously, during an ALTER COLUMN ... TYPE operation, the column being modified could briefly disappear, leading to "column does not exist" errors for any queries attempting to access it. This issue arose from the incorrect ordering of dependency rules, which has now been addressed. The changes to the dependency rules during the column type alteration are as follows: - The name of the new column is not set until we are ready to make it public. Previously, the column name was assigned too early in the pre-commit phase, which necessitated additional shadow transient column name logic. This new approach simplifies the process and eliminates the need for that extra rename logic. - The ColumnName and Column elements for both the old and new columns are swapped within the same stage. I have also maintained the existing rules for cases where the column type is not being altered by utilizing the IsAlterColumnTypeOp and IsNotAlterColumnTypeOp predicates. I updated existing tests to query the column during each stage of the alter. This allowed me to reproduce the issue. Epic: CRDB-25314 Closes #133996 Release note: none Co-authored-by: Matt Spilchen <matt.spilchen@cockroachlabs.com>
|
Build failed: |
Previously, during an ALTER COLUMN ... TYPE operation, the column being modified could briefly disappear, leading to "column does not exist" errors for any queries attempting to access it. This issue arose from the incorrect ordering of dependency rules, which has now been addressed. The changes to the dependency rules during the column type alteration are as follows: - The name of the new column is not set until we are ready to make it public. Previously, the column name was assigned too early in the pre-commit phase, which necessitated additional shadow transient column name logic. This new approach simplifies the process and eliminates the need for that extra rename logic. - The ColumnName and Column elements for both the old and new columns are swapped within the same stage. I have also maintained the existing rules for cases where the column type is not being altered by utilizing the IsAlterColumnTypeOp and IsNotAlterColumnTypeOp predicates. I updated existing tests to query the column during each stage of the alter. This allowed me to reproduce the issue. Epic: CRDB-25314 Closes cockroachdb#133996 Release note: none
spilchen
force-pushed
the
issue-133996/final-rev-rebase
branch
from
f0cb08b
to
88a9c8e
Compare
|
bors r+ |
Previously, during an ALTER COLUMN ... TYPE operation, the column being modified could briefly disappear, leading to "column does not exist" errors for any queries attempting to access it. This issue arose from the incorrect ordering of dependency rules, which has now been addressed.
The changes to the dependency rules during the column type alteration are as follows:
I have also maintained the existing rules for cases where the column type is not being altered by utilizing the IsAlterColumnTypeOp and IsNotAlterColumnTypeOp predicates.
I updated existing tests to query the column during each stage of the alter. This allowed me to reproduce the issue.
Epic: CRDB-25314
Closes #133996
Release note: none