Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sql/schemachanger: column can be invisible temporarily when altering the column type #133996

Closed
spilchen opened this issue Oct 31, 2024 · 2 comments · Fixed by #134411
Closed

sql/schemachanger: column can be invisible temporarily when altering the column type #133996

spilchen opened this issue Oct 31, 2024 · 2 comments · Fixed by #134411
Assignees
@spilchen
Labels
A-schema-changes C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@spilchen
Copy link
Contributor

spilchen commented Oct 31, 2024 •
edited by exalate-issue-sync bot
Loading

While adding DML injection tests for ALTER COLUMN .. SET TYPE, I noticed that the column being altered can become temporarily invisible. This occurs when the alteration is complex, requiring a backfill. In such cases, we add a new column and drop the old one. Due to the visibility stages of the various column elements, there are points where querying the column results in a "column not found" error.

To address this, we likely need to add new dependency rules to ensure the old column name remains until the new column becomes public.

We can add tests for this in two ways:

  • In the DML injection test (pkg/sql/schemachanger/dml_injection_test.go), we can run a query like "SELECT new_col FROM tbl LIMIT 1" after each stage (new_col is the column being altered).
  • In the end-to-end test (pkg/sql/schemachanger/testdata/end_to_end/alter_table_alter_column_type_general_expr), we can add a SELECT query that uses the altered column by including a predicate on the column in the WHERE clause.

Jira issue: CRDB-43856

Epic CRDB-25314
@spilchen spilchen added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-schema-changes T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Oct 31, 2024
@spilchen spilchen self-assigned this Oct 31, 2024
@blathers-crl blathers[crl]
Copy link

blathers-crl bot commented Oct 31, 2024

Hi @spilchen, please add branch-* labels to identify which branch(es) this C-bug affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@spilchen spilchen added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) and removed C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels Oct 31, 2024
@exalate-issue-sync exalate-issue-sync bot added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. and removed C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels Oct 31, 2024
@blathers-crl blathers[crl]
Copy link

blathers-crl bot commented Oct 31, 2024

Hi @exalate-issue-sync[bot], please add branch-* labels to identify which branch(es) this C-bug affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@exalate-issue-sync exalate-issue-sync bot added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) and removed C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels Oct 31, 2024
@spilchen spilchen mentioned this issue Nov 1, 2024
Merged
spilchen added a commit to spilchen/cockroach that referenced this issue Nov 6, 2024
@spilchen
sql/schemachanger: Prevent Temporary Disappearance of Altered Columns
59a59b3 
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 spilchen mentioned this issue Nov 6, 2024
Merged
spilchen added a commit to spilchen/cockroach that referenced this issue Nov 12, 2024
@spilchen
sql/schemachanger: Prevent Temporary Disappearance of Altered Columns
f0cb08b 
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
craig bot pushed a commit that referenced this issue Nov 12, 2024
@spilchen
Merge #134411
fae784f 
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 <[email protected]>
craig bot pushed a commit that referenced this issue Nov 12, 2024
@nvanbenschoten @spilchen @rafiss
Merge #133216 #133217 #134411 #134875
c1a452d 
133216: roachtest: add kv/splits/nodes=3/quiesce=false/lease=leader r=nvanbenschoten a=nvanbenschoten

Part of #132762.

This commit adds a `kv/splits/nodes=3/quiesce=false/lease=leader` test. It should be able to support 80k ranges even without quiescence, because leader leases use store liveness for failure detection and lease extension.

To push higher than this, we will likely need to address #133885.

Release note: None

133217: roachtest: metamorphically enable leader leases r=nvanbenschoten a=nvanbenschoten

Part of #132762.

To increase test coverage of leader leases, this patch adds leader leases to the set of possible lease types when tests opt-in to metamorphic leases. As of fc68b0f, most roachtests do enable metamorphic leases, so this will provide good coverage of leader leases.

Before merging this, we should manually run all of these tests with leader leases to verify that they pass.

Release note: None

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

134875: license: unredact logs written by license enforcer r=rafiss a=rafiss

This makes it so log messages are not redacted unnecessarily.

- Use redact.StringBuilder instead of strings.Builder.
- Avoid using `.String()` arguments for log.Infof, since strings are always redacted.
- Mark license type as a redact.SafeValue.

Epic: None
Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Matt Spilchen <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
@craig craig bot closed this as completed in 88a9c8e Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@spilchen spilchen

Labels
A-schema-changes C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql/schemachanger: Prevent Temporary Disappearance of Altered Columns spilchen/cockroach
1 participant
@spilchen