Skip to content

sql/schemachanger: remove oprules from declarative schema changer#96769

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
fqazi:removeOpRules
Feb 9, 2023
Merged

sql/schemachanger: remove oprules from declarative schema changer#96769
craig[bot] merged 2 commits intocockroachdb:masterfrom
fqazi:removeOpRules

Conversation

@fqazi
Copy link
Copy Markdown
Collaborator

@fqazi fqazi commented Feb 8, 2023

This PR will do the following:

  1. Enforce assertions to block not implemented operations outside of special drop scenarios
  2. Eliminate oprules for both dropping descriptors and columns, so that operations are still generated as no-ops.

Previously, when the declarative schema changer invoked,
not implemented operations they would be no-ops. This patch,
adds assertion that fail these operations, except for certain
cases where these operations should be no-ops for dropped
descriptors.

Epic: none
Release note: None
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@fqazi fqazi force-pushed the removeOpRules branch 2 times, most recently from f3d885a to e8a1d7c Compare February 8, 2023 04:15
@fqazi fqazi marked this pull request as ready for review February 8, 2023 04:56
@fqazi fqazi requested a review from a team February 8, 2023 04:56
@fqazi fqazi requested a review from a team as a code owner February 8, 2023 04:56
@fqazi fqazi requested a review from mgartner February 8, 2023 04:56
Copy link
Copy Markdown
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.

The only smell to me is the need to remember to skip the side-effect if the table is dropped in scmutationexec. I can live with it. :lgtm:

@fqazi
Copy link
Copy Markdown
Collaborator Author

fqazi commented Feb 8, 2023

@ajwerner TFTR!

@fqazi fqazi force-pushed the removeOpRules branch 2 times, most recently from adaa601 to 1b2184a Compare February 8, 2023 21:53
Previously, oprules in the declarative schema changer
would be used to skip op edges on certain dropped
elements (columns and descriptors). This made it harder
to understand behaviour of the declarative schema
changer. To address this, this patch removes this
logic, and adds support for skipping operations
during runtime instead.

Epic: none

Release note: None
@fqazi
Copy link
Copy Markdown
Collaborator Author

fqazi commented Feb 8, 2023

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 8, 2023

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 8, 2023

Build failed (retrying...):

@craig craig bot merged commit 802a56a into cockroachdb:master Feb 9, 2023
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 9, 2023

Build succeeded:

postamar pushed a commit to postamar/cockroach that referenced this pull request May 23, 2023
This commit is a follow-up to cockroachdb#96769 which removes superfluous test
definitions.

Informs cockroachdb#96769.

Release note: None
craig bot pushed a commit that referenced this pull request May 23, 2023
103148: server: server identity returns nodeID for system tenant r=aadityasondhi a=dhartunian

Previously, the server identity object would hide the nodeID from identity objects that contained a set tenantID. This was done to create separation of concerns between a secondary tenant and the rest of the cluster. However, this identity is used in populating log payloads with contextual metadata. That led to #103112 due to the fact that the system tenant sets its tenantId to 1 which led to a hidden nodeID.

We now always emit the nodeID when the tenantID is set to the system tenant.

Fixes #103112

Release note (bug fix): 23.1.0 contained a bug where the `node_id` field would be omitted in logs. This fix restores that value.

103709: server,jobs: Better handle node drain r=miretskiy a=miretskiy

Rework job registry drain signal to terminate the drain as soon as the last job that was watching for drain signal completes its drain

Epic: CRDB-26978
Release note: None

103781: rules: add deprecation notices, remove op-rule tests r=postamar a=postamar

This commit is a follow-up to #96769 which removes superfluous test definitions.

Informs #96769.

Release note: None

103792: sql/opt: deflake TestExecBuild/autocommit r=knz a=nvanbenschoten

Fixes #103772.

The test became flaky after 3fc29ad, which causes intent resolution to end up in foreground requests' traces.

Release note: None

Co-authored-by: David Hartunian <davidh@cockroachlabs.com>
Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>
Co-authored-by: Marius Posta <marius@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
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