Skip to content

importccl: Set FK and CHECK constraints to unvalidated after IMPORT INTO #39183

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

Merged
merged 1 commit into from
Aug 1, 2019

Conversation

adityamaru27
Copy link
Contributor

This change sets foreign key and CHECK constraints to unvalidated
after the table is imported into, but before the descriptor is
published in a PUBLIC state.

@adityamaru27 adityamaru27 requested review from dt and a team July 31, 2019 01:34
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@dt dt 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 @adityamaru27 and @dt)


pkg/ccl/importccl/import_stmt.go, line 975 at r1 (raw file):

		if !tbl.IsNew {
			// Set FK constraints to unvalidated before publishing the table imported into.

there's a helper for this if you didn't want to find all the index yourself for _, idx := range tbl.AllNonDropIndexes()


pkg/ccl/importccl/import_stmt.go, line 990 at r1 (raw file):

			// Set CHECK constraints to unvalidated before publishing the table imported into.
			tableDesc.Checks = make([]*sqlbase.TableDescriptor_CheckConstraint, len(tbl.Desc.Checks))
			for i, c := range tbl.Desc.Checks {

this direct access vs using the accessor is okay because we asserted that Mutations is empty

Copy link
Contributor Author

@adityamaru27 adityamaru27 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 @dt)


pkg/ccl/importccl/import_stmt.go, line 975 at r1 (raw file):

Previously, dt (David Taylor) wrote…

there's a helper for this if you didn't want to find all the index yourself for _, idx := range tbl.AllNonDropIndexes()

Done.


pkg/ccl/importccl/import_stmt.go, line 990 at r1 (raw file):

Previously, dt (David Taylor) wrote…

this direct access vs using the accessor is okay because we asserted that Mutations is empty

feels cleaner to use the accessor, done.

@adityamaru27
Copy link
Contributor Author

bors r+

Copy link
Member

@dt dt 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 @adityamaru27 and @dt)


pkg/ccl/importccl/import_stmt.go, line 975 at r1 (raw file):

Previously, adityamaru27 (Aditya Maru) wrote…

Done.

not quite what I meant: the point of the helper returning pointers to index desc is that you can just mutate via the pointer. I actually think the direct field access was arguably bet


pkg/ccl/importccl/import_stmt.go, line 986 at r2 (raw file):

					tableDesc.PrimaryIndex = tableIdx
				} else {
					tableDesc.Indexes = append(tableDesc.Indexes, tableIdx)

Er, I don't think we want to be appending.

@dt
Copy link
Member

dt commented Aug 1, 2019

bors r-

@craig
Copy link
Contributor

craig bot commented Aug 1, 2019

Canceled

Copy link
Member

@dt dt 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 @adityamaru27)


pkg/ccl/importccl/import_stmt.go, line 986 at r2 (raw file):

Previously, dt (David Taylor) wrote…

Er, I don't think we want to be appending.

To expand on that: AllNonDropIndexes is returning pointers to things already in desc.Indexes (as well as to the PK and indexes in Mutations) so if we append like this, I think we're actually duplicating the existing indexes, right? Plus, if there were any in mutations (though I think in this case that is not a concern) this would be copying them to the public index list too outside the normal mutation completion path. Oh, I missed the blanking of desc.Indexes above, sorry. 🤦‍♂ that said, I'd think I'd still rather edit directly via the pointer, rather than in these special-case branches for the different descriptor fields, since apparently I'm prone to mis-reading it otherwise 😬.

Copy link
Contributor Author

@adityamaru27 adityamaru27 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 @dt)


pkg/ccl/importccl/import_stmt.go, line 986 at r2 (raw file):

Previously, dt (David Taylor) wrote…

To expand on that: AllNonDropIndexes is returning pointers to things already in desc.Indexes (as well as to the PK and indexes in Mutations) so if we append like this, I think we're actually duplicating the existing indexes, right? Plus, if there were any in mutations (though I think in this case that is not a concern) this would be copying them to the public index list too outside the normal mutation completion path.

Done.

This change sets foreign key and CHECK constraints to unvalidated
after the table is imported into, but before the descriptor is
published in a PUBLIC state.

Release note: None
@adityamaru27
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Aug 1, 2019
39183: importccl: Set FK and CHECK constraints to unvalidated after IMPORT INTO r=adityamaru27 a=adityamaru27

This change sets foreign key and CHECK constraints to unvalidated
after the table is imported into, but before the descriptor is
published in a PUBLIC state.

Co-authored-by: Aditya Maru <[email protected]>
@craig
Copy link
Contributor

craig bot commented Aug 1, 2019

Build succeeded

@craig craig bot merged commit 4ff31f2 into cockroachdb:master Aug 1, 2019
@adityamaru27 adityamaru27 deleted the unvalidate-fk branch August 1, 2019 18:36
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