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: version gating pkey virtual column validation. #79659

Merged
merged 1 commit into from
Apr 8, 2022

Conversation

fqazi
Copy link
Collaborator

@fqazi fqazi commented Apr 8, 2022

Fixes: #79329

Previously, the schemachange workload on master was failing
because 21.2 does not support virtual columns inside primary
keys. So, the mixed version variant of the schemachange workload
can fail when a primary key with virtual columns exists, specifically
these tables become inaccessible from 21.2 nodes. To address, this
patch introduces a version gate to prevent creating prevent
creating primary indexes with virtual columns and updates the
schemachange workload to detect when this error will be generated
in mixed version workloads.

Release note: None

@fqazi fqazi requested a review from a team April 8, 2022 14:46
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@fqazi
Copy link
Collaborator Author

fqazi commented Apr 8, 2022

Porting this over to master, since we need the same fix to avoid issues for now. At some point the version bump will happen and we can rip this code out

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 assuming this is the same as #79609

@chengxiong-ruan
Copy link
Contributor

pkg/workload/schemachange/operation_generator.go, line 1138 at r1 (raw file):

				}
			} else if indexDef, ok := def.(*tree.IndexTableDef); ok {
				for _, indexCol := range indexDef.Columns {

sorry...my nasal congestion probably makes me not thinking clearly. But should we make this check more precise that check column is virtual instead of checking computed since computed column can be stored. Also should we check the index is primary?

@chengxiong-ruan
Copy link
Contributor

Thanks for picking this up :)

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 @chengxiong-ruan)


pkg/workload/schemachange/operation_generator.go, line 1138 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

sorry...my nasal congestion probably makes me not thinking clearly. But should we make this check more precise that check column is virtual instead of checking computed since computed column can be stored. Also should we check the index is primary?

Yes your right, this should be checking for virtual not computed

Fixes: cockroachdb#79329

Previously, the schemachange workload on master was failing
because 21.2 does not support virtual columns inside primary
keys. So, the mixed version variant of the schemachange workload
can fail when a primary key with virtual columns exists, specifically
these tables become inaccessible from 21.2 nodes. To address, this
patch introduces a version gate to prevent creating prevent
creating primary indexes with virtual columns and updates the
schemachange workload to detect when this error will be generated
in mixed version workloads.

Release note: None
@fqazi fqazi force-pushed the primaryKeyFixBF branch from 585f791 to 081e12d Compare April 8, 2022 15:19
Copy link
Contributor

@chengxiong-ruan chengxiong-ruan left a comment

Choose a reason for hiding this comment

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

:lgtm: 😴 🛌

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @chengxiong-ruan)

@fqazi
Copy link
Collaborator Author

fqazi commented Apr 8, 2022

@chengxiong-ruan @postamar TFTR!

@fqazi
Copy link
Collaborator Author

fqazi commented Apr 8, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 8, 2022

Build succeeded:

@craig craig bot merged commit d53a4c0 into cockroachdb:master Apr 8, 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.

sql: virtual columns on primary indexes should be blocked until cluster version is 22.1
4 participants