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

rules: suppress a dep-rule in special case #87196

Conversation

Xiang-Gu
Copy link
Contributor

@Xiang-Gu Xiang-Gu commented Aug 31, 2022

Commit 1: very minor comment, function location, code changes;
Commit 2: suppress a dep-rule that causes forward incompatibility
for dropping a table with rowLevelTTL in the mixed version state.

Previously, in a mixed version state (v22.1 and v22.2), if a old node
drops a rowLevelTTL table and the new shcema changer job was adopted
by a new node, then there is a dep rule that cannot satisfied and hence
causes forward incompatibility. This PR fixes this by suppressing this
dep rule if this particular case happens (namely, it suppress the rule
if from is Table element, to is a RowLevelTTL element, and there is
no PUBLIC status of the table element anywhere in the graph).

Fixes: #86672

Release note: None

Release justification: fixed a release blocker bug.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

1. changed one comment;
2. changed one function locaiton in file to be more consistent;
3. added a code change that is consistent with the error message
but won't affect correctness.

Release justification:
@Xiang-Gu Xiang-Gu force-pushed the fix_dropping_row_level_ttl_tables_in_mixed_version_state_forward_compatibility_issue branch 2 times, most recently from 3f65dbb to c112476 Compare September 6, 2022 15:02
@Xiang-Gu Xiang-Gu marked this pull request as ready for review September 6, 2022 15:02
@Xiang-Gu Xiang-Gu requested a review from a team September 6, 2022 15:02
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: the CI failure is a flake

Reviewed 3 of 3 files at r1, 1 of 4 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @Xiang-Gu)


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

									public.Eq(scpb.Status_PUBLIC),
									n.AttrEqVar(screl.CurrentStatus, public),
								}

Can you statically define this rules in helpers? Also, can you make these rule names not have spaces?

Code quote:

							screl.Schema.DefNotJoin1("node does not have a PUBLIC status", "n", func(n rel.Var) rel.Clauses {
								public := rel.Var("public")
								return rel.Clauses{
									public.Eq(scpb.Status_PUBLIC),
									n.AttrEqVar(screl.CurrentStatus, public),
								}
							})(n),

@Xiang-Gu Xiang-Gu force-pushed the fix_dropping_row_level_ttl_tables_in_mixed_version_state_forward_compatibility_issue branch from c112476 to b38ff1c Compare September 7, 2022 14:54
Copy link
Contributor Author

@Xiang-Gu Xiang-Gu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)


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

Previously, ajwerner wrote…

Can you statically define this rules in helpers? Also, can you make these rule names not have spaces?

done!

@craig
Copy link
Contributor

craig bot commented Sep 7, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 7, 2022

Build failed (retrying...):

Release justification: fixed a release blocker bug.

Previously, in a mixed version state (v22.1 and v22.2), if a old node
drops a rowLevelTTL table and the new shcema changer job was adopted
by a new node, then there is a dep rule that cannot satisfied and hence
causes forward incompatibility. This PR fixes this by suppressing this
dep rule if this particular case happens (namely, it suppress the rule
if `from` is Table element, `to` is a RowLevelTTL element, and there is
no `PUBLIC` status of the table element anywhere in the graph).

Fixes: cockroachdb#86672

Release note: None
@Xiang-Gu Xiang-Gu force-pushed the fix_dropping_row_level_ttl_tables_in_mixed_version_state_forward_compatibility_issue branch from b38ff1c to 660efc7 Compare September 7, 2022 17:10
@craig
Copy link
Contributor

craig bot commented Sep 7, 2022

Canceled.

@Xiang-Gu
Copy link
Contributor Author

Xiang-Gu commented Sep 7, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 7, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 7, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 7, 2022

Build succeeded:

@craig craig bot merged commit 99a9587 into cockroachdb:master Sep 7, 2022
@Xiang-Gu Xiang-Gu deleted the fix_dropping_row_level_ttl_tables_in_mixed_version_state_forward_compatibility_issue branch September 7, 2022 23:07
@Xiang-Gu
Copy link
Contributor Author

Xiang-Gu commented Oct 3, 2022

blathers backport 22.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql/schemachanger: drop jobs from 22.1 can fail trying to plan on RowLevelTTL
3 participants