scplan: Fix deprules for dropping computed columns #120794

rimadeodhar · 2024-03-20T22:49:04Z

This PR fixes the new schema changer deprules for
dropping virtual computed columns which are also
used for hash and expression indexes.
Currently, the optimizer allows for virtual, computed
columns to be evaluated even when under mutation.
However, this causes concurrent DML issues when the
schemachanger job is running as the column that the
virtual computed column depends on moves into WRITE_ONLY
stage prior to the computed column being dropped.
As a result, the optimizer is unable to access the column
for evaluating the compute expression.
This PR updates the dep rules to ensure the virtual,
computed column is dropped before the dependent column
moves to WRITE_ONLY ensuring that the compute expression
can be enforced correctly for concurrent DML during all
stages of the schema change.

Epic: none
Fixes: #111608
Fixes: #111619
Release note: None

Note for reviewers: This PR is stacked on top of #120792.

blathers-crl · 2024-03-20T22:49:08Z

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.}

cockroach-teamcity · 2024-03-20T22:49:14Z

This change is

fqazi

Reviewed 4 of 23 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rimadeodhar)

pkg/sql/schemachanger/scplan/testdata/alter_table_drop_column line 1353 at r1 (raw file):

StatementPhase stage 1 of 1 with 42 MutationType ops
  transitions:
    [[Column:{DescID: 107, ColumnID: 3}, ABSENT], PUBLIC] -> WRITE_ONLY

Just thinking about this if we have a transaction what makes v2 invisible in this scenario? I think we are waiting this post commit non-revertible phase in this scenario?

i.e
BEGIN
ALTER TABLE defaultdb.foo DROP COLUMN v2 CASCADE;
SELECT * FROM defaultdb.foo;
...

rimadeodhar

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

pkg/sql/schemachanger/scplan/testdata/alter_table_drop_column line 1353 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Just thinking about this if we have a transaction what makes v2 invisible in this scenario? I think we are waiting this post commit non-revertible phase in this scenario?
i.e
BEGIN
ALTER TABLE defaultdb.foo DROP COLUMN v2 CASCADE;
SELECT * FROM defaultdb.foo;
...

Yeah, good point. I think for transactions we would need to introduce an additional stage where the columns being dropped move to inaccessible in the statement phase. I'll file a tracking issue for it. We would still need the changes contained within this PR as without them we will continue to get errors once the column moves to WRITE_ONLY.

rimadeodhar

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

pkg/sql/schemachanger/scplan/testdata/alter_table_drop_column line 1353 at r1 (raw file):

Previously, rimadeodhar (Rima Deodhar) wrote…

Yeah, good point. I think for transactions we would need to introduce an additional stage where the columns being dropped move to inaccessible in the statement phase. I'll file a tracking issue for it. We would still need the changes contained within this PR as without them we will continue to get errors once the column moves to WRITE_ONLY.

Created #120863

michae2 · 2024-03-24T05:37:01Z

Looks like you'll need to do ./dev testlogic base --config=5node --files=distsql_stats --ignore-cache --rewrite and then you should get this change:

diff --git a/pkg/sql/logictest/testdata/logic_test/distsql_stats b/pkg/sql/logictest/testdata/logic_test/distsql_stats
index e218e65a693..76ce58b7c6c 100644
--- a/pkg/sql/logictest/testdata/logic_test/distsql_stats
+++ b/pkg/sql/logictest/testdata/logic_test/distsql_stats
@@ -2699,8 +2699,10 @@ SELECT statistics_name, column_names, row_count, histogram_id IS NOT NULL AS has
 FROM [SHOW STATISTICS FOR TABLE t118537]
 ORDER BY statistics_name, column_names::STRING
 ----
-statistics_name  column_names  row_count  has_histogram
-mutation         {a}           10         true
+statistics_name  column_names                 row_count  has_histogram
+mutation         {a,crdb_internal_a_shard_3}  10         false
+mutation         {a}                          10         true
+mutation         {crdb_internal_a_shard_3}    10         true

 statement ok
 SET CLUSTER SETTING jobs.debug.pausepoints = ''

(I was literally just trying to figure out why I get this diff on 23.1 but not 23.2 or master, so congratulations on the fix! 😆)

fqazi

Great work, just a couple of comments to improve performance on the rule evaluation side

Reviewed 19 of 23 files at r1, 18 of 19 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rimadeodhar)

pkg/sql/schemachanger/scplan/internal/rules/helpers.go line 307 at r2 (raw file):

			return rel.Clauses{
				indexColumn.Type((*scpb.IndexColumn)(nil)),
				indexColumn.AttrEqVar(screl.DescID, rel.Blank),

This should be tableID?

pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_column.go line 151 at r2 (raw file):

						}
						for _, refColumns := range colType.ComputeExpr.ReferencedColumnIDs {
							if refColumns == column.ColumnID {

We can use: ReferencedColumnIDsContains and do it before the filter

rimadeodhar

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

pkg/sql/schemachanger/scplan/internal/rules/helpers.go line 307 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

This should be tableID?

Done.

pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_column.go line 151 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

We can use: ReferencedColumnIDsContains and do it before the filter

The referencedColumnIDs field is contained within the expression field in the ColumnType element, so I can't use this helper. Any other recommendations?

fqazi

All of this looks good. I do have one comment below, not sure if we want to add some other safety or something else as a follow on, since it seems like a potential pitfall if users aren't careful.

Reviewed 34 of 85 files at r3, 79 of 79 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rimadeodhar)

pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_column.go line 151 at r2 (raw file):

Previously, rimadeodhar (Rima Deodhar) wrote…

The referencedColumnIDs field is contained within the expression field in the ColumnType element, so I can't use this helper. Any other recommendations?

Ugh, no other options here then. Though its not too terrible since its limited to the columns on the table.

pkg/sql/schemachanger/testdata/end_to_end/drop_column_with_null_constraint/drop_column_with_null_constraint.definition line 9 at r4 (raw file):

# for j
stage-exec phase=PostCommitPhase stage=:
INSERT INTO t (i) VALUES($stageKey);

I think logically and correctness wise all of this makes sense, since we need to be able to rollback consistently. I think the only worry I have is someone accidentally doing a DROP COLUMN and not realizing that inserts will be blocked until the table is backfilled.

Not sure if we want a safety net of some sort 🤔 . Not sure what your thoughts are on that scenario.

rimadeodhar

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

pkg/sql/schemachanger/testdata/end_to_end/drop_column_with_null_constraint/drop_column_with_null_constraint.definition line 9 at r4 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

I think logically and correctness wise all of this makes sense, since we need to be able to rollback consistently. I think the only worry I have is someone accidentally doing a DROP COLUMN and not realizing that inserts will be blocked until the table is backfilled.

Not sure if we want a safety net of some sort 🤔 . Not sure what your thoughts are on that scenario.

Do you mean a scenario where someone drops a column accidentally, cancels the job and then the table is inaccessible for inserts until backfill part of the rollback is completed?

This PR fixes the new schema changer deprules for dropping virtual computed columns which are also used for hash and expression indexes. Currently, the optimizer allows for virtual, computed columns to be evaluated even when under mutation. However, this causes concurrent DML issues when the schemachanger job is running as the column that the virtual computed column depends on moves into WRITE_ONLY stage prior to the computed column being dropped. As a result, the optimizer is unable to access the column for evaluating the compute expression. This PR updates the dep rules to ensure the virtual, computed column is dropped before the dependent column moves to WRITE_ONLY ensuring that the compute expression can be enforced correctly for concurrent DML during all stages of the schema change. Epic: none Fixes: cockroachdb#111608 Fixes: cockroachdb#111619 Release note: None

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rimadeodhar)

pkg/sql/schemachanger/testdata/end_to_end/drop_column_with_null_constraint/drop_column_with_null_constraint.definition line 9 at r4 (raw file):

Previously, rimadeodhar (Rima Deodhar) wrote…

Do you mean a scenario where someone drops a column accidentally, cancels the job and then the table is inaccessible for inserts until backfill part of the rollback is completed?

Wait, I misread the plan this is a non-issue. Plus the rollback won't have any backfill only validation, which should be quick so it should be a small blip

rimadeodhar · 2024-05-02T19:42:03Z

TFTR!

bors r+

craig · 2024-05-02T20:16:55Z

Build succeeded:

rimadeodhar force-pushed the fix_dml_hash_index branch from 98bd1ce to 28d9cb7 Compare March 20, 2024 22:51

rimadeodhar marked this pull request as ready for review March 20, 2024 22:52

rimadeodhar requested a review from a team as a code owner March 20, 2024 22:52

fqazi requested changes Mar 21, 2024

View reviewed changes

rimadeodhar commented Mar 21, 2024

View reviewed changes

rimadeodhar requested a review from fqazi March 22, 2024 16:00

michae2 mentioned this pull request Mar 24, 2024

release-23.1: sql: collect table statistics on virtual computed columns #120928

Closed

fqazi requested changes Mar 25, 2024

View reviewed changes

rimadeodhar force-pushed the fix_dml_hash_index branch 2 times, most recently from 4c7e879 to 967eb34 Compare March 28, 2024 22:54

rimadeodhar force-pushed the fix_dml_hash_index branch 2 times, most recently from 3579df8 to edb8aa5 Compare April 26, 2024 17:33

rimadeodhar commented Apr 26, 2024

View reviewed changes

rimadeodhar requested a review from fqazi April 26, 2024 18:32

fqazi approved these changes May 2, 2024

View reviewed changes

rimadeodhar commented May 2, 2024

View reviewed changes

fqazi reviewed May 2, 2024

View reviewed changes

rimadeodhar force-pushed the fix_dml_hash_index branch from edb8aa5 to 47aa2d5 Compare May 2, 2024 18:49

craig bot merged commit 00dd860 into cockroachdb:master May 2, 2024
22 checks passed

blathers-crl bot mentioned this pull request May 3, 2024

release-24.1: scplan: Fix deprules for dropping computed columns #123580

Closed

rimadeodhar mentioned this pull request May 6, 2024

release-24.1: scplan: Fix deprules for dropping computed columns #123684

Merged

DrewKimball mentioned this pull request May 31, 2024

roachtest: sqlsmith/setup=rand-tables/setting=no-mutations failed #124684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scplan: Fix deprules for dropping computed columns #120794

scplan: Fix deprules for dropping computed columns #120794

rimadeodhar commented Mar 20, 2024 •

edited

Loading

blathers-crl bot commented Mar 20, 2024

cockroach-teamcity commented Mar 20, 2024

fqazi left a comment

rimadeodhar left a comment

rimadeodhar left a comment

michae2 commented Mar 24, 2024

fqazi left a comment

rimadeodhar left a comment

fqazi left a comment

rimadeodhar left a comment

fqazi left a comment

rimadeodhar commented May 2, 2024

craig bot commented May 2, 2024

scplan: Fix deprules for dropping computed columns #120794

scplan: Fix deprules for dropping computed columns #120794

Conversation

rimadeodhar commented Mar 20, 2024 • edited Loading

blathers-crl bot commented Mar 20, 2024

cockroach-teamcity commented Mar 20, 2024

fqazi left a comment

Choose a reason for hiding this comment

rimadeodhar left a comment

Choose a reason for hiding this comment

rimadeodhar left a comment

Choose a reason for hiding this comment

michae2 commented Mar 24, 2024

fqazi left a comment

Choose a reason for hiding this comment

rimadeodhar left a comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

rimadeodhar left a comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

rimadeodhar commented May 2, 2024

craig bot commented May 2, 2024

rimadeodhar commented Mar 20, 2024 •

edited

Loading