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: add split_time column to crdb_internal.ranges and crdb_internal.ranges_no_leases #37610

Merged
merged 1 commit into from May 21, 2019

Conversation

Projects
None yet
5 participants
@jeffrey-xiao
Copy link
Collaborator

commented May 20, 2019

Users can query the split_time column to determine which ranges were
manually split and when they were manually split.

Release note (sql change): Add split_time column to crdb_internal.ranges
and crdb_internal.ranges_no_leases

@jeffrey-xiao jeffrey-xiao requested review from ajwerner and nvanbenschoten May 20, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented May 20, 2019

This change is Reviewable

@nvanbenschoten
Copy link
Member

left a comment

Nice! Looks like this is missing corresponding changes to the logic tests though.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jeffrey-xiao, and @nvanbenschoten)


pkg/sql/crdb_internal.go, line 1713 at r1 (raw file):

  table_name      STRING NOT NULL,
  index_name      STRING NOT NULL,
	replicas     INT[] NOT NULL,

Weird spacing here.


pkg/sql/crdb_internal.go, line 1795 at r1 (raw file):

			splitTime := tree.DNull
			if desc.StickyBit != nil {
				splitTime = tree.MakeDTimestamp(desc.StickyBit.GoTime(), time.Microsecond)

We're pretty inconsistent with HLC timestamp throughout this file. This is the only place where we use GoTime() instead of timeutil.Unix(0, timestamp.WallTime). We're also inconsistent about microsecond vs. nanosecond resolution.

It would be nice to unify this in a single hlcToTimestampDatum function that we can use throughout this file. In fact, it would be nice if there was a single unified place for this conversion to live (maybe tree.MakeDTimestampFromHLC). I see other places that it could be used in backupccl/show.go. I think it's worth asking in the #sql slack channel if such a conversion does exist.

@jeffrey-xiao jeffrey-xiao force-pushed the jeffrey-xiao:add-sticky-bit-column branch 4 times, most recently from b6ec3b1 to 3d3bd77 May 20, 2019

@jeffrey-xiao jeffrey-xiao marked this pull request as ready for review May 20, 2019

@jeffrey-xiao jeffrey-xiao requested review from cockroachdb/sql-execution-prs as code owners May 20, 2019

@jeffrey-xiao
Copy link
Collaborator Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @jeffrey-xiao)


pkg/sql/crdb_internal.go, line 1713 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Weird spacing here.

There is some inconsistent spacing in this file with regards to spaces vs. tabs for indentation. I've changed this indentation to spaces.

@ajwerner
Copy link
Collaborator

left a comment

:lgtm:

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jeffrey-xiao)

@mjibson
Copy link
Member

left a comment

LGTM.

I agree with Nathan's comments. We should add two functions. Something like:

MakeInexactDTimestampFromHLC that does HLC -> timestamp with microsecond rounding
MakeExactDecimalFromHLC that does HLC -> decimal with zero loss (assuming there are use cases for this)

I think it's important to have the word "Inexact" in the function name so we don't break someone's expectations. This can be done (or not) in another PR.

@jeffrey-xiao jeffrey-xiao force-pushed the jeffrey-xiao:add-sticky-bit-column branch 2 times, most recently from 7732bec to 6806652 May 21, 2019

@nvanbenschoten
Copy link
Member

left a comment

:lgtm: mod one comment.

Reviewed 1 of 3 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner and @jeffrey-xiao)


pkg/sql/logictest/testdata/logic_test/crdb_internal, line 334 at r3 (raw file):


query I
SELECT count(manual_split_time) FROM crdb_internal.ranges WHERE manual_split_time IS NOT NULL;

Instead of these two aggregations, how about SELECT start_key, end_key, manual_split_time IS NOT NULL FROM crdb_internal.ranges so that we can check which range has the sticky bit set.

@jeffrey-xiao jeffrey-xiao force-pushed the jeffrey-xiao:add-sticky-bit-column branch from 6806652 to 3a38c7e May 21, 2019

@jeffrey-xiao

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2019

bors r+

ALTER TABLE foo SPLIT AT VALUES(2)

query TT colnames
SELECT start_pretty, end_pretty FROM crdb_internal.ranges WHERE manual_split_time IS NOT NULL;

This comment has been minimized.

Copy link
@nvanbenschoten

nvanbenschoten May 21, 2019

Member

This looks broken. Did you mean for these two queries to be different?

@jeffrey-xiao

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2019

bors r-

@craig

This comment has been minimized.

Copy link

commented May 21, 2019

Canceled

@jeffrey-xiao jeffrey-xiao force-pushed the jeffrey-xiao:add-sticky-bit-column branch from 3a38c7e to f8e656d May 21, 2019

@jeffrey-xiao
Copy link
Collaborator Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @ajwerner and @nvanbenschoten)


pkg/sql/logictest/testdata/logic_test/crdb_internal, line 334 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This looks broken. Did you mean for these two queries to be different?

As discussed offline, since the range for the table is split asynchronously, just checking for the split range is fine.

@jeffrey-xiao

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 21, 2019

Merge #37610
37610: sql: add split_time column to crdb_internal.ranges and crdb_internal.ranges_no_leases r=jeffrey-xiao a=jeffrey-xiao

Users can query the split_time column to determine which ranges were
manually split and when they were manually split.

Release note (sql change): Add split_time column to crdb_internal.ranges
                           and crdb_internal.ranges_no_leases

Co-authored-by: Jeffrey Xiao <jeffrey.xiao1998@gmail.com>
@jeffrey-xiao

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2019

bors r-

@craig

This comment has been minimized.

Copy link

commented May 21, 2019

Canceled

sql: add split_time column to crdb_internal.ranges and crdb_internal.…
…ranges_no_leases

Users can query the split_time column to determine which ranges were
manually split and when they were manually split.

Release note (sql change): Add split_time column to crdb_internal.ranges
                           and crdb_internal.ranges_no_leases

@jeffrey-xiao jeffrey-xiao force-pushed the jeffrey-xiao:add-sticky-bit-column branch from 2c3dce2 to 8c02639 May 21, 2019

@jeffrey-xiao

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 21, 2019

Merge #37610
37610: sql: add split_time column to crdb_internal.ranges and crdb_internal.ranges_no_leases r=jeffrey-xiao a=jeffrey-xiao

Users can query the split_time column to determine which ranges were
manually split and when they were manually split.

Release note (sql change): Add split_time column to crdb_internal.ranges
                           and crdb_internal.ranges_no_leases

Co-authored-by: Jeffrey Xiao <jeffrey.xiao1998@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 21, 2019

Build failed

@jeffrey-xiao

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2019

Seems to be a flake #37698.

bors r+

craig bot pushed a commit that referenced this pull request May 21, 2019

Merge #37274 #37537 #37610
37274: sql/row: avoid various allocations during mutations r=nvanbenschoten a=nvanbenschoten

This PR fell out of some poking around in #37059. It removes a number of
allocations throughout the `sql/row` package. All were found using `goescape`.

37537: *: introduce an abstraction for a set of ReplicaDescriptors r=tbg a=danhhz

Closes #37499

Release note: None

37610: sql: add split_time column to crdb_internal.ranges and crdb_internal.ranges_no_leases r=jeffrey-xiao a=jeffrey-xiao

Users can query the split_time column to determine which ranges were
manually split and when they were manually split.

Release note (sql change): Add split_time column to crdb_internal.ranges
                           and crdb_internal.ranges_no_leases

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>
Co-authored-by: Jeffrey Xiao <jeffrey.xiao1998@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 21, 2019

Build failed (retrying...)

craig bot pushed a commit that referenced this pull request May 21, 2019

Merge #37537 #37610
37537: *: introduce an abstraction for a set of ReplicaDescriptors r=tbg a=danhhz

Closes #37499

Release note: None

37610: sql: add split_time column to crdb_internal.ranges and crdb_internal.ranges_no_leases r=jeffrey-xiao a=jeffrey-xiao

Users can query the split_time column to determine which ranges were
manually split and when they were manually split.

Release note (sql change): Add split_time column to crdb_internal.ranges
                           and crdb_internal.ranges_no_leases

Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>
Co-authored-by: Jeffrey Xiao <jeffrey.xiao1998@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 21, 2019

Build failed (retrying...)

craig bot pushed a commit that referenced this pull request May 21, 2019

Merge #37610
37610: sql: add split_time column to crdb_internal.ranges and crdb_internal.ranges_no_leases r=jeffrey-xiao a=jeffrey-xiao

Users can query the split_time column to determine which ranges were
manually split and when they were manually split.

Release note (sql change): Add split_time column to crdb_internal.ranges
                           and crdb_internal.ranges_no_leases

Co-authored-by: Jeffrey Xiao <jeffrey.xiao1998@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 21, 2019

Build succeeded

@craig craig bot merged commit 8c02639 into cockroachdb:master May 21, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@jeffrey-xiao jeffrey-xiao deleted the jeffrey-xiao:add-sticky-bit-column branch May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.