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/schemachanger/scplan: allow plan to move to NonRevertible earlier #84430

Merged

Conversation

ajwerner
Copy link
Contributor

This is critical for DROP COLUMN. In DROP COLUMN we cannot perform the
primary index swap until the dropping column reaches DELETE_ONLY, which is
not revertible. The primary index swap itself is revertible. Given this fact,
we need a mechanism to be able to "promote" revertible operations (operations
which don't destroy information or cause irrevocable visible side effects) to
be grouped with or after non-revertible operations. This commit makes that
happen naturally.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner marked this pull request as ready for review July 14, 2022 14:36
@ajwerner ajwerner requested a review from a team July 14, 2022 14:36
Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

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

LGTM but please that can-still-fail determining logic to opgen as suggested, it should be quite straightforward to do.

return aID < bID
}),
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I'm totally fine with these, they are well-motivated and easy to understand.

rec(n)
}
return m
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. This makes sense.

This logic belongs in the bowels of opgen, though, along with what determines the revertible boolean field on each op-edge. You'll find that it's quite similar.

In fact, perhaps this revertible boolean field should be replaced by an enum with three states:

  • revertible, failure possible;
  • revertible, failure not possible;
  • not revertible, failure not possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this into opgen in the newly added first commit. It was the right thing to do, but took a little bit of elbow grease.

I've left two booleans as I found it easier to deal with for now, do you feel strongly?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I don't feel strongly about that.

@ajwerner ajwerner force-pushed the ajwerner/loosen-non-revertible-planning branch from 38b09d6 to f54a672 Compare July 14, 2022 16:35
This change reworks opgen to require the emit functions to specify the type
of op they'll emit. This allows us to statically determine the op type and
then use that to determine whether there are any remaining ops which might
fail.

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/loosen-non-revertible-planning branch from f54a672 to 21a4021 Compare July 14, 2022 16:50
This is critical for DROP COLUMN. In `DROP COLUMN` we cannot perform the
primary index swap until the dropping column reaches `DELETE_ONLY`, which is
not revertible. The primary index swap itself is revertible. Given this fact,
we need a mechanism to be able to "promote" revertible operations (operations
which don't destroy information or cause irrevocable visible side effects) to
be grouped with or after non-revertible operations. This commit makes that
happen naturally.

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/loosen-non-revertible-planning branch from 21a4021 to 20acd12 Compare July 14, 2022 18:39
@ajwerner
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 15, 2022

Build succeeded:

@craig craig bot merged commit e8818d5 into cockroachdb:master Jul 15, 2022
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.

3 participants