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: bugfixes around writing the old primary key in pk changes #44489

Merged
merged 1 commit into from
Jan 30, 2020

Conversation

rohany
Copy link
Contributor

@rohany rohany commented Jan 29, 2020

This PR fixes two bugs:

  • The logic for when to rewrite the old primary key was broken
    resulting in the old primary key not being rewritten in many cases.
  • The old primary key being created was not properly dealing with
    its dangling interleave information.

This PR makes the design decision to not re-interleave the copy of the old primary index if it was interleaved.

Release note: None

@rohany rohany requested review from solongordon and a team January 29, 2020 16:39
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@solongordon solongordon 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 @rohany and @solongordon)


pkg/sql/alter_table.go, line 402 at r1 (raw file):

			// TODO (rohany): is there an easier way of checking if the existing primary index was the
			//  automatically created one?
			if !(len(n.tableDesc.PrimaryIndex.ColumnNames) == 1 && n.tableDesc.PrimaryIndex.ColumnNames[0] == "rowid") {

Not ideal that this would also apply to a user who happened to create a table with a primary key called "rowid". Could you look for a more reliable way to detect this? Maybe also check that rowid is a hidden column?


pkg/sql/alter_table.go, line 407 at r1 (raw file):

					"old_primary_key",
					nameExists,
				)

This can be addressed in a separate PR, but I don't love the "old_primary_key" name. What if we just named it the same way we name other indexes where the user doesn't specify a name? Like if you add INDEX (x, y) on a table named t, we call the index t_x_y_idx.

@rohany rohany force-pushed the pk-change-bugfixes branch from 4c1cc39 to 765d816 Compare January 30, 2020 20:29
@rohany
Copy link
Contributor Author

rohany commented Jan 30, 2020

PTAL

@rohany rohany requested a review from solongordon January 30, 2020 20:30
Copy link
Contributor

@solongordon solongordon 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 @rohany and @solongordon)

This PR fixes two bugs:
* The logic for when to rewrite the old primary key was broken
  resulting in the old primary key not being rewritten in many cases.
* The old primary key being created was not properly dealing with
  its dangling interleave information.

Release note: None
@rohany rohany force-pushed the pk-change-bugfixes branch from 765d816 to fc51339 Compare January 30, 2020 20:49
@rohany
Copy link
Contributor Author

rohany commented Jan 30, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 30, 2020

Build failed (retrying...)

craig bot pushed a commit that referenced this pull request Jan 30, 2020
44489: sql: bugfixes around writing the old primary key in pk changes r=rohany a=rohany

This PR fixes two bugs:
* The logic for when to rewrite the old primary key was broken
  resulting in the old primary key not being rewritten in many cases.
* The old primary key being created was not properly dealing with
  its dangling interleave information.

This PR makes the design decision to not re-interleave the copy of the old primary index if it was interleaved.

Release note: None

44551: jobs: always set start time of a job r=spaskob a=spaskob

When starting a job via CreateAndStartJob if
making the job started fails the job will stil be
in system.jobs and can be adopted by another
node later but the started time will be 0 in this
case. We add a check and set it if necessary.

Release note: none.

44553: engine: redefine targetSize and add maxSize to ExportToSst r=itsbilal a=ajwerner

This PR is a follow-up of work from #44440 motivated by problems unearthed while typing #44482. The PR comes in two commits:

1) Re-define `targetSize` from being a target below which most requests would remain to being the size above which the export stops.
2) Add a `maxSize` parameter above which the `ExportToSst` call will fail.

See the individual commits for more details. 

Co-authored-by: Rohan Yadav <[email protected]>
Co-authored-by: Spas Bojanov <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
@craig
Copy link
Contributor

craig bot commented Jan 30, 2020

Build succeeded

@craig craig bot merged commit fc51339 into cockroachdb:master Jan 30, 2020
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