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

schemachanger: address index validation errors during pk swap #118843

Merged
merged 1 commit into from Feb 8, 2024

Conversation

fqazi
Copy link
Collaborator

@fqazi fqazi commented Feb 6, 2024

Previously, when mutations were made public for validation queries, the declarative schema changer primary key swaps were not appropriately handled. This could lead to scenarios where queries would be unable to find columns in recreated secondary indexes that should ideally exist (i.e. columns from the old primary key). This would cause validation to fail on these newly created indexes. To address, this patch adds logic for making declarative primary key swaps mutations properly by detecting them.

Fixes: #118626

Release note (bug fix): ALTER PRIMARY KEY could fail with a "non-nullable column with no value! Index scanned .." when validating recreated secondary indexes.

@fqazi fqazi requested a review from a team as a code owner February 6, 2024 17:52
Copy link

blathers-crl bot commented Feb 6, 2024

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
Copy link
Member

This change is Reviewable

@fqazi fqazi added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. labels Feb 6, 2024
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

nice fix! just had some nits about comments

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


pkg/sql/catalog/descriptor.go line 764 at r1 (raw file):

	// IsPrimaryKeySwapMutation returns true if the mutation is a primary key
	// swap mutation or a secondary index used by the declarative schema changer
	// for an index swap.

nit: "for an index swap" -> "for a primary index swap"


pkg/sql/catalog/tabledesc/table_desc.go line 650 at r1 (raw file):

		// and does not use a PrimaryKeySwap mutation for this operation. Instead,
		// it will create a new secondary index with a primary index encoding and
		// make it public later. To detect a primary index swap scenario we are going

i didn't follow this sentence:

"To detect a primary index swap scenario we are going for the declarative case detect the encoding type and if the index is going public."

missing words?

Copy link
Collaborator Author

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

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


pkg/sql/catalog/tabledesc/table_desc.go line 650 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i didn't follow this sentence:

"To detect a primary index swap scenario we are going for the declarative case detect the encoding type and if the index is going public."

missing words?

Yeah this was badly worded, clarified the language here:

// The declarative schema changer handles primary key swaps differently and
// does not use a PrimaryKeySwap mutation for this operation. Instead, it
// will create a new secondary index with a primary index encoding as an index
// mutation. In the declarative schema changer case, to detect a primary index
// swap scenario, we are going to detect the encoding type of the index and
// check if a declarative scpb.PrimaryIndex element with a matching ID exists
// with a public target state. Since a table can only have a single primary
// index at a time, this would indicate this primary index is replacing the
// existing primary index.

Previously, when mutations were made public for validation queries,
the declarative schema changer primary key swaps were not appropriately
handled. This could lead to scenarios where queries would be unable to
find columns in recreated secondary indexes that should ideally exist
(i.e. columns from the old primary key). This would cause validation
to fail on these newly created indexes. To address, this patch adds
logic for making declarative primary key swaps mutations properly by
detecting them.

Fixes: cockroachdb#118626

Release note (bug fix): ALTER PRIMARY KEY could fail with a
"non-nullable column <x> with no value! Index scanned .." when
validating recreated secondary indexes.
@fqazi fqazi force-pushed the alterPKRegressionForSecondary branch from 5d89813 to 7a88441 Compare February 8, 2024 03:52
@fqazi
Copy link
Collaborator Author

fqazi commented Feb 8, 2024

@rafiss TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 8, 2024

Build succeeded:

@craig craig bot merged commit 7042601 into cockroachdb:master Feb 8, 2024
8 of 9 checks passed
Copy link

blathers-crl bot commented Feb 8, 2024

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

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

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


error creating merge commit from 7a88441 to blathers/backport-release-23.1-118843: 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 23.1.x failed. See errors above.


error creating merge commit from 7a88441 to blathers/backport-release-23.2-118843: 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 23.2.x failed. See errors above.


error creating merge commit from 7a88441 to blathers/backport-release-23.1.15-rc-118843: 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 23.1.15-rc failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roachtest: sqlsmith: non-nullable column with no value when altering the primary key
3 participants