opgen: make Column transitioning to public non-revertible by Xiang-Gu · Pull Request #88489 · cockroachdb/cockroach

Xiang-Gu · 2022-09-22T18:16:43Z

Delayed the transitioning of the Column element into PUBLIC to
the PostCommitNonRevertible phase. This helps with stmt like
ALTER TABLE t ADD COLUMN j INT DEFAULT 30 UNIQUE where we will
first do the pirmary index swap, then backfill the new unique secondary
index, and finally turn the column and the new unique secondary index
into public at the same stage. Previously, the column would incorrectly
turn into public before the new unique secondary index even begins
backfilling.

Fixes #82953

Release note: None

cockroach-teamcity · 2022-09-22T18:16:54Z

This change is

Xiang-Gu · 2022-10-03T16:08:56Z

I didn't fully understand the fix in the previous iteration and came up with a dep rule that didn't actually solve the problem. That dep rule delays the primary index swap and column turning public to happen in the same stage as the new unique secondary index turning public. This is not right because we would never be able to backfill the new unique secondary index to begin with, because that new column is not present in the (old) primary index. The reason we had such a requirement is definitely not obvious: it's related to an assumption that the optimizer holds deeply in the code and it's not easy to change any time soon. See this slack discussion for more details with an example included there.

The alternative @ajwerner suggested is to mark the transition of the column element into public non-revertible. What this entails is that we would respect the requirement by swapping the primary index first while keeping the column in WRITE_ONLY. We then backfill the new unique secondary index. Finally, in the non-revertible phase, we transition the column and the new unique secondary index into public in the same stage.

postamar · 2022-10-03T17:24:26Z

Whatever it takes. Regardless of this particular issue, I believe that making the column public a non-revertible op makes sense anyway. Go for it!

Xiang-Gu · 2022-10-03T17:52:11Z

The only failed test in CI is marked by TeamCity as "flaky", so I think it's RFAL!

postamar

Nicely done. LGTM conditional on cosmetic changes.

postamar · 2022-10-03T18:47:26Z

pkg/sql/schemachanger/scplan/internal/rules/dep_add_column.go

+	// The new primary index should reach VALIDATED, a ready state to be used as
+	// source to backfill the new secondary index, before the secondary index becomes existent.
+	registerDepRule(
+		"primary index with new columns should exist before secondary indexes",


This is an oversight on my part when I moved these rule definitions about, I think, but I just noticed that "new columns" should not be mentioned at all in this rule name. This rule can come into play regardless of columns. Instead, it might be worth mentioning why this rule is required for ADD COLUMN UNIQUE in the commentary above.

postamar · 2022-10-03T18:49:45Z

pkg/sql/schemachanger/scplan/internal/rules/dep_add_column.go

+				statusesToPublicOrTransient(from, scpb.Status_VALIDATED, to, scpb.Status_DELETE_ONLY),
+			}
+		})
+}


These rules should remain where they were in the dep_add_index.go file.

ajwerner

Is there something that enforces that the primary index moves to public before any indexes involving a new column moves to DELETE_ONLY? It seems like maybe we're under-constrained and getting lucky.

Xiang-Gu

@ajwerner Marius had these two rules:

cockroach/pkg/sql/schemachanger/scplan/internal/rules/dep_add_index.go

Lines 118 to 159 in 6993039

    
           // We want to say that all columns which are part of a secondary index need 
        
           // to be in a primary index which is validated 
        
           // To do that, we want to find a secondary index which has a source which 
        
           // is a primary index which is itself new. 
        
           func init() { 
        
           	registerDepRule( 
        
           		"primary index with new columns should exist before secondary indexes", 
        
           		scgraph.Precedence, 
        
           		"primary-index", "secondary-index", 
        
           		func(from, to nodeVars) rel.Clauses { 
        
           			return rel.Clauses{ 
        
           				from.Type((*scpb.PrimaryIndex)(nil)), 
        
           				to.Type((*scpb.SecondaryIndex)(nil)), 
        
           				joinOnDescID(from, to, "table-id"), 
        
           				joinOn( 
        
           					from, screl.IndexID, 
        
           					to, screl.SourceIndexID, 
        
           					"primary-index-id", 
        
           				), 
        
           				statusesToPublicOrTransient(from, scpb.Status_PUBLIC, to, scpb.Status_BACKFILL_ONLY), 
        
           			} 
        
           		}) 
        
           	registerDepRule( 
        
           		"primary index with new columns should exist before temp indexes", 
        
           		scgraph.Precedence, 
        
           		"primary-index", "temp-index", 
        
           		func(from, to nodeVars) rel.Clauses { 
        
           			return rel.Clauses{ 
        
           				from.Type((*scpb.PrimaryIndex)(nil)), 
        
           				to.Type((*scpb.TemporaryIndex)(nil)), 
        
           				joinOnDescID(from, to, "table-id"), 
        
           				joinOn( 
        
           					from, screl.IndexID, 
        
           					to, screl.SourceIndexID, 
        
           					"primary-index-id", 
        
           				), 
        
           				statusesToPublicOrTransient(from, scpb.Status_PUBLIC, to, scpb.Status_DELETE_ONLY), 
        
           			} 
        
           		}) 
        
           }

that made sure if we are adding both primary index and a secondary index, and the source index of the secondary index is the primary index, as in the case for ADD COLUMN UNIQUE, then we make sure we the primary index transitioned to PUBLIC before the secondary index transitioned to BACKFILL_ONLY.

Does this answer your questions?

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

Previously, postamar (Marius Posta) wrote…

I guess I am a bit confused about their original comments here

cockroach/pkg/sql/schemachanger/scplan/internal/rules/dep_add_index.go

Lines 118 to 121 in 6993039

    
           // We want to say that all columns which are part of a secondary index need 
        
           // to be in a primary index which is validated 
        
           // To do that, we want to find a secondary index which has a source which 
        
           // is a primary index which is itself new.

Are they stale? Do you want me to keep the updated comments but just move those two rules back to dep_add_index.go?

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

Previously, postamar (Marius Posta) wrote…

ack above.

ajwerner

I don't think it does, no. Didn't you delete that rule in this PR and change it to be about the primary index moving into validated prior to the temp index moving into DELETE_ONLY?

Xiang-Gu

Ahh, right. I realized that and since we are not adding any dep rules anymore, I reverted the changes on the existing rules. But I haven't pushed the code yet.

With this existing rule, do you think we have constrained it properly?

Xiang-Gu

Just pushed the reverts; now this PR is truly a one-line changer revertible(false); under opgen_column.go

ajwerner

Yeah, this looks right to me from the tests we have. Can you add an end-to-end test with a default value and a unique constraint?

This PR delays the transitioning of the Column element into PUBLIC to the PostCommitNonRevertible phase. This helps with stmt like `ALTER TABLE t ADD COLUMN j INT DEFAULT 30 UNIQUE` where we will first do the pirmary index swap, then backfill the new unique secondary index, and finally turn the column and the new unique secondary index into public at the same stage. Previously, the column would incorrectly turn into public before the new unique secondary index even begins backfilling, as detailed in cockroachdb#82953. Release note: None

Xiang-Gu

@ajwerner
I added an end-to-end test for ADD COLUMN DEFAULT UNIQUE and generated the expected output.

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

Previously, Xiang-Gu (Xiang Gu) wrote…

I reverted this change -- it turns out we don't and shouldn't need to modify the existing rules; they are needed to constrain the graph such that the new unique secondary index (as well its temp index) didn't move out of ABSENT until the new primary index is PUBLIC.

ajwerner

Xiang-Gu · 2022-10-04T21:03:53Z

TFTR!

bors r+

craig · 2022-10-04T22:02:35Z

Build succeeded:

Bazel Essential CI (Cockroach)

Xiang-Gu · 2022-10-05T17:06:03Z

blathers backport 22.2 22.2.0

blathers-crl · 2022-10-05T17:06:12Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 32b766a to blathers/backport-release-22.2-88489: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2 failed. See errors above.

error creating merge commit from 32b766a to blathers/backport-release-22.2.0-88489: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.0 failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

Xiang-Gu force-pushed the fix_add_column_unqiue_index_still_not_ready_when_primary_index_swap branch 2 times, most recently from ded1737 to 9c2b5fe Compare October 3, 2022 15:58

Xiang-Gu marked this pull request as ready for review October 3, 2022 17:52

Xiang-Gu requested a review from a team October 3, 2022 17:52

Xiang-Gu changed the title ~~rule: changed two dep rules~~ opgen: make Column transitioning to public non-revertible Oct 3, 2022

postamar approved these changes Oct 3, 2022

View reviewed changes

ajwerner reviewed Oct 3, 2022

View reviewed changes

Xiang-Gu commented Oct 3, 2022

View reviewed changes

ajwerner reviewed Oct 3, 2022

View reviewed changes

Xiang-Gu commented Oct 3, 2022

View reviewed changes

Xiang-Gu force-pushed the fix_add_column_unqiue_index_still_not_ready_when_primary_index_swap branch from 9c2b5fe to 0317cfc Compare October 3, 2022 21:48

Xiang-Gu commented Oct 3, 2022

View reviewed changes

ajwerner reviewed Oct 3, 2022

View reviewed changes

Xiang-Gu force-pushed the fix_add_column_unqiue_index_still_not_ready_when_primary_index_swap branch from 0317cfc to 32b766a Compare October 4, 2022 15:57

Xiang-Gu commented Oct 4, 2022

View reviewed changes

ajwerner approved these changes Oct 4, 2022

View reviewed changes

craig bot merged commit fc50f10 into cockroachdb:master Oct 4, 2022

postamar mentioned this pull request Oct 5, 2022

sql/schemachanger: column prematurely turns to PUBLIC in ADD COLUMN ... UNIQUE #82953

Closed

This was referenced Oct 5, 2022

release-22.2: opgen: make Column transitioning to public non-revertible #89414

Merged

release-22.2.0: opgen: make Column transitioning to public non-revertible #89415

Merged

	// We want to say that all columns which are part of a secondary index need
	// to be in a primary index which is validated
	// To do that, we want to find a secondary index which has a source which
	// is a primary index which is itself new.
	func init() {

	registerDepRule(
	"primary index with new columns should exist before secondary indexes",
	scgraph.Precedence,
	"primary-index", "secondary-index",
	func(from, to nodeVars) rel.Clauses {
	return rel.Clauses{
	from.Type((*scpb.PrimaryIndex)(nil)),
	to.Type((*scpb.SecondaryIndex)(nil)),
	joinOnDescID(from, to, "table-id"),
	joinOn(
	from, screl.IndexID,
	to, screl.SourceIndexID,
	"primary-index-id",
	),
	statusesToPublicOrTransient(from, scpb.Status_PUBLIC, to, scpb.Status_BACKFILL_ONLY),
	}
	})

	registerDepRule(
	"primary index with new columns should exist before temp indexes",
	scgraph.Precedence,
	"primary-index", "temp-index",
	func(from, to nodeVars) rel.Clauses {
	return rel.Clauses{
	from.Type((*scpb.PrimaryIndex)(nil)),
	to.Type((*scpb.TemporaryIndex)(nil)),
	joinOnDescID(from, to, "table-id"),
	joinOn(
	from, screl.IndexID,
	to, screl.SourceIndexID,
	"primary-index-id",
	),
	statusesToPublicOrTransient(from, scpb.Status_PUBLIC, to, scpb.Status_DELETE_ONLY),
	}
	})
	}

Conversation

Xiang-Gu commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Sep 22, 2022

Uh oh!

Xiang-Gu commented Oct 3, 2022

Uh oh!

postamar commented Oct 3, 2022

Uh oh!

Xiang-Gu commented Oct 3, 2022

Uh oh!

postamar left a comment

Choose a reason for hiding this comment

Uh oh!

postamar Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

postamar Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

Xiang-Gu left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

Xiang-Gu left a comment

Choose a reason for hiding this comment

Uh oh!

Xiang-Gu left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

Xiang-Gu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

Xiang-Gu commented Oct 4, 2022

Uh oh!

craig bot commented Oct 4, 2022

Uh oh!

Xiang-Gu commented Oct 5, 2022

Uh oh!

blathers-crl bot commented Oct 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Xiang-Gu commented Sep 22, 2022 •

edited

Loading

Xiang-Gu left a comment •

edited

Loading