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

hlc: remove the Synthetic field from Timestamp and LegacyTimestamp #116830

Merged

Conversation

nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented Dec 19, 2023

Closes #101938.

This PR completes the work to remove the Synthetic field from Timestamp and LegacyTimestamp. It removes the remaining uses, removes the fields from the proto definitions, and removes all access to the fields in methods.

Release note: None

Copy link

blathers-crl bot commented Dec 19, 2023

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

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/synTimestampRemRest branch from 0f10147 to b989349 Compare December 19, 2023 23:28
@nvanbenschoten
Copy link
Member Author

TestBelowRaftProtosDontChange/RangeAppliedState is failing, correctly detecting that we store synthetic timestamps in the RangeAppliedState. Specifically, RangeAppliedState.RaftClosedTimestamp.Synthetic can be set to true for GLOBAL tables. This is going to make the migration difficult, because while we don't include the RangeAppliedState in full range consistency checks, we do include it in stats-only consistency checks.

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/synTimestampRemRest branch 3 times, most recently from 61a81fe to 8a70b72 Compare December 20, 2023 18:45
@nvanbenschoten
Copy link
Member Author

Extracted 5 of these commits into #117015, #117016, and #117018.

@nvanbenschoten
Copy link
Member Author

Extracted another 6 of these commits into #117098, #117118, #117119, and #117120.

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/synTimestampRemRest branch 2 times, most recently from e2190ca to c70a9d4 Compare January 3, 2024 04:07
craig bot pushed a commit that referenced this pull request Jan 3, 2024
117262: storage: don't set synthetic timestamps in tests r=nvanbenschoten a=nvanbenschoten

Informs #101938.
Extracted from #116830.

We are about to remove this field from the proto, so stop assigning it in storage tests. `TestMVCCAndEngineKeyDecodeSyntheticTimestamp` still tests that we can properly decode keys that were encoded with synthetic timestamps.

Release note: None

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/synTimestampRemRest branch 2 times, most recently from 2aac63d to af0eef6 Compare January 9, 2024 00:06
@nvanbenschoten nvanbenschoten marked this pull request as ready for review January 9, 2024 00:08
@nvanbenschoten nvanbenschoten requested review from a team as code owners January 9, 2024 00:08
@nvanbenschoten nvanbenschoten requested a review from a team January 9, 2024 00:08
@nvanbenschoten nvanbenschoten requested a review from a team as a code owner January 9, 2024 00:08
@nvanbenschoten nvanbenschoten requested review from RaduBerinde and miretskiy and removed request for a team January 9, 2024 00:08
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/synTimestampRemRest branch from af0eef6 to c31785b Compare January 9, 2024 03:26
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

:lgtm: Nice cleanup!

Reviewed 3 of 3 files at r1, 3 of 3 files at r2, 2 of 2 files at r3, 9 of 9 files at r4, 2 of 2 files at r5, 1 of 1 files at r6, 15 of 15 files at r7, 8 of 8 files at r8, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @miretskiy, @nvanbenschoten, and @RaduBerinde)


pkg/sql/sem/asof/as_of.go line 249 at r6 (raw file):

		// Parse synthetic flag.
		syn := false
		if strings.HasSuffix(s, "?") {

This is strictly a breaking SQL change. Do we need a release note for it?


pkg/ccl/changefeedccl/helpers_test.go line 635 at r4 (raw file):

	}
	if !options.disableSyntheticTimestamps && rand.Intn(2) == 0 {
		// Offset all timestamps a random (but consistent per test) amount into the

Do we want to keep this test coverage of future timestamps around? I see that the actual offsetting is commented out anyway, so it's not currently in effect, but seems like it might be useful to fix that at some point and keep the deactivated code around.

@miretskiy Do you have an opinion on this? If not, let's axe it.

Informs cockroachdb#101938.

This commit is essentially a revert of 951add4. It removes the handling
of synthetic timestamps from the ptstorage package.

This flag has been deprecated since v22.2 and is no longer consulted in
uncertainty interval checks or by transaction commit-wait. It will never
makes its way into the ptstorage package once it is removed from the proto
definition.

Release note: None
Informs cockroachdb#101938.

This commit is essentially a revert of 5259c1d. It removes the handling
of synthetic timestamps from the changefeedccl package.

This flag has been deprecated since v22.2 and is no longer consulted in
uncertainty interval checks or by transaction commit-wait. It will never
makes its way into the changefeedccl package once it is removed from the
proto definition.

Release note: None
Informs cockroachdb#101938.

This is now unused.

Release note: None
Informs cockroachdb#101938.

This commit is essentially a revert of 34dc5ae. It removes the ability
to pass a synthetic timestamp to an AOST query.

Release note (backward-incompatible change): AS OF SYSTEM TIME queries
can no longer use a timestamp followed by a question mark to signifiy a
future-time value. This was an undocumented syntax.
Closes cockroachdb#101938.

This commit completes the work to remove the Synthetic field from
Timestamp and LegacyTimestamp. It removes the fields from the proto
definitions and removes all access to the fields in methods.

The commit also cleans up the remaining references in the code which
were just waiting for the field to be removed.

Release note: None
Copy link
Member Author

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r=erikgrinaker

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani, @erikgrinaker, @miretskiy, and @RaduBerinde)


pkg/sql/sem/asof/as_of.go line 249 at r6 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

This is strictly a breaking SQL change. Do we need a release note for it?

Good call. Done.


pkg/ccl/changefeedccl/helpers_test.go line 635 at r4 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

Do we want to keep this test coverage of future timestamps around? I see that the actual offsetting is commented out anyway, so it's not currently in effect, but seems like it might be useful to fix that at some point and keep the deactivated code around.

@miretskiy Do you have an opinion on this? If not, let's axe it.

I'll remove for now, as this is dead code without synthetic timestamps and removing it allows us to remove a decent amount of test infrastructure. I'll reintroduce it if @miretskiy thinks it's valuable.

@craig
Copy link
Contributor

craig bot commented Jan 10, 2024

Build succeeded:

@craig craig bot merged commit 3bcf088 into cockroachdb:master Jan 10, 2024
8 of 9 checks passed
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.

kv: remove synthetic timestamps
4 participants