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.

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: rollback of create table hits slow path for table deletion #79123

Closed
fqazi opened this issue Mar 31, 2022 · 1 comment · Fixed by #79543
Closed

sql: rollback of create table hits slow path for table deletion #79123

fqazi opened this issue Mar 31, 2022 · 1 comment · Fixed by #79543
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@fqazi
Copy link
Collaborator

fqazi commented Mar 31, 2022

Describe the problem

Please describe the issue you observed, and any steps we can take to reproduce it:

When rolling back a create table AS operation, we can end up not populating the drop time within the table descriptor, which can lead to us using the wrong code path inside the GC job:

// If DropTime isn't set, assume this drop request is from a version
// 1.1 server and invoke legacy code that uses DeleteRange and range GC.
if table.GetDropTime() == 0 {
    log.Infof(ctx, "clearing data in chunks for table %d", table.GetID())
    return sql.ClearTableDataInChunks(ctx, db, codec, sv, table, false /* traceKV */)
}

This scenario can be easily hit by taking a CREATE TABLE AS operation and cancelling it in the middle, which will cause us to drop the table. When the GC job is created it will incorrectly end up using this slower code path, instead of clearing the entire range.

We should populate the drop time and completely remove the 1.1 legacy behaviour.

To Reproduce

What did you do? Describe in your own words.
Execute a large CREATE TABLE AS operation, and cancel it as we are backfilling the table.

Expected behavior
We should be able to quickly delete the data and drop the table, but end up using a slower code path because of backwards compatbility.

Additional data / screenshots
If the problem is SQL-related, include a copy of the SQL query and the schema
of the supporting tables.

If a node in your cluster encountered a fatal error, supply the contents of the
log directories (at minimum of the affected node(s), but preferably all nodes).

Note that log files can contain confidential information. Please continue
creating this issue, but contact support@cockroachlabs.com to submit the log
files in private.

Jira issue: CRDB-14585

@fqazi fqazi added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-schema-deprecated Use T-sql-foundations instead labels Mar 31, 2022
@fqazi fqazi self-assigned this Mar 31, 2022
@blathers-crl blathers-crl bot added this to Triage in SQL Foundations Mar 31, 2022
@ajwerner
Copy link
Contributor

ajwerner commented Apr 5, 2022

Good find!

@ajwerner ajwerner moved this from Triage to 22.2 General in SQL Foundations Apr 5, 2022
fqazi added a commit to fqazi/cockroach that referenced this issue Apr 6, 2022
Fixes: cockroachdb#79123

Previously, when dropping a table during rollback of a create
we would clean up table data in chunks instead of doing a
clear range. This was inadequate because he code path in
question existed for compatibility Cockroach 1.1 and had terrible
performance characteristics for large tables. To address,
this patch will always use delete data using clear range when
dropping tables during rollback which will have better performance.

Release note (performance improvement): Rollback of CREATE
TABLE AS with large quantities of data has similar performance
to regular DROP TABLE.
craig bot pushed a commit that referenced this issue Apr 7, 2022
79543: sql: use clear range to clean tables during rollback of create r=fqazi a=fqazi

Fixes: #79123

Previously, when dropping a table during rollback of a create
we would clean up table data in chunks instead of doing a
clear range. This was inadequate because he code path in
question existed for compatibility Cockroach 1.1 and had terrible
performance characteristics for large tables. To address,
this patch will always use delete data using clear range when
dropping tables during rollback which will have better performance.

Release note (performance improvement): Rollback of CREATE
TABLE AS with large quantities of data has similar performance
to regular DROP TABLE.

Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>
@craig craig bot closed this as completed in 742d6fe Apr 7, 2022
SQL Foundations automation moved this from 22.2 General to Closed Apr 7, 2022
blathers-crl bot pushed a commit that referenced this issue Apr 7, 2022
Fixes: #79123

Previously, when dropping a table during rollback of a create
we would clean up table data in chunks instead of doing a
clear range. This was inadequate because he code path in
question existed for compatibility Cockroach 1.1 and had terrible
performance characteristics for large tables. To address,
this patch will always use delete data using clear range when
dropping tables during rollback which will have better performance.

Release note (performance improvement): Rollback of CREATE
TABLE AS with large quantities of data has similar performance
to regular DROP TABLE.
fqazi added a commit to fqazi/cockroach that referenced this issue Apr 7, 2022
Fixes: cockroachdb#79123

Previously, when dropping a table during rollback of a create
we would clean up table data in chunks instead of doing a
clear range. This was inadequate because he code path in
question existed for compatibility Cockroach 1.1 and had terrible
performance characteristics for large tables. To address,
this patch will always use delete data using clear range when
dropping tables during rollback which will have better performance.

Release note (performance improvement): Rollback of CREATE
TABLE AS with large quantities of data has similar performance
to regular DROP TABLE.
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
@blathers-crl blathers-crl bot added this to Triage in SQL Foundations May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
SQL Foundations
  
Done [after migration]
Development

Successfully merging a pull request may close this issue.

3 participants