sql: stop observing the CommitTimestamp in TRUNCATE #42650
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
andreimatei
reviewed
Nov 21, 2019
There was a problem hiding this comment.
LGTM
Nit: I wouldn't talk about observing commit timestamps in the release note. And also I don't think phrasing like "this change" is appropriate in release notes - there's no "this" in that context.
Lucy, can we get an explanation of the Time field in the ReplacementOf struct please? It has no comment... Let's make it wall time instead of an hlc.Timestamp if it doesn't need to be HLC.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @lucy-zhang)
ajwerner
force-pushed
the
ajwerner/stop-observing-commit-timestamp-in-truncate
branch
from
1da85e9 to
fe800e5
Compare
|
I added some commentary and updated the release note. |
In cockroachdb#40581 we stopped observing the commit timestamp to write it into table descriptors. In this change I overlooked (rather forgot) about this additional place in the code where we observed the commit timestamp. As far as I can tell we don't read this field anywhere ever. Furthermore we know that the the table descriptor in question to which we are referring must be alive and equal to the provided value at the timestamp at which it was read due to serializability. In short, this minor change continues to populate the field with a sensible value and will permit TRUNCATE to be pushed. Fixes cockroachdb#41566. Release note (bug fix): Long running transactions which attempt to TRUNCATE can now be pushed and will commit in cases where they previously could fail or retry forever.
ajwerner
force-pushed
the
ajwerner/stop-observing-commit-timestamp-in-truncate
branch
from
fe800e5 to
3003a79
Compare
|
@lucy-zhang friendly ping |
thoszhang
approved these changes
Nov 25, 2019
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1.
Reviewable status:
|
TFTR! bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Nov 25, 2019
42650: sql: stop observing the CommitTimestamp in TRUNCATE r=ajwerner a=ajwerner In #40581 we stopped observing the commit timestamp to write it into table descriptors. In this change I overlooked (rather forgot) about this additional place in the code where we observed the commit timestamp. As far as I can tell we don't read this field anywhere ever. Furthermore we know that the the table descriptor in question to which we are referring must be alive and equal to the provided value at the timestamp at which it was read due to serializability. In short, this minor change continues to populate the field with a sensible value and will permit TRUNCATE to be pushed. Fixes #41566. Release note (bug fix): Long running transactions which attempt to TRUNCATE can now be pushed and will commit in cases where they previously could fail or retry forever. 42746: roachtest/cdc: fix cdc/bank and cdc/schemareg r=nvanbenschoten a=nvanbenschoten Fixes #41177. Fixes #42690. These were both broken by #41793 because prior versions of crdb didn't support the `WITH diff` option. Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]>
Build succeeded |
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
Feb 29, 2020
In cockroachdb#42650 I had intended to remove all places where the commit timestamp was observed in TRUNCATE. I missed this case. Modification times for the first write to table descriptors as of 19.2 should be the zero value (or the commit timestamp, so this isn't wrong). This observance of the commit timestamp prevents transactions which TRUNCATE tables from being pushed. Combined with cockroachdb#44090 we'll have removed all non-user observation of commit timestamps. Release note (bug fix): Long running transactions which attempt to TRUNCATE can now be pushed and will commit in cases where they previously could fail or retry forever.
craig bot
pushed a commit
that referenced
this pull request
Feb 29, 2020
44091: sql: stop observing the commit timestamp in TRUNCATE r=nvanbenschoten a=ajwerner In #42650 I had intended to remove all places where the commit timestamp was observed in TRUNCATE. I missed this case. Modification times for the first write to table descriptors as of 19.2 should be the zero value (or the commit timestamp, so this isn't wrong). This observance of the commit timestamp prevents transactions which TRUNCATE tables from being pushed. Combined with #44090 we'll have removed all non-user observation of commit timestamps. Release note (bug fix): Long running transactions which attempt to TRUNCATE can now be pushed and will commit in cases where they previously could fail or retry forever. Co-authored-by: Andrew Werner <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #40581 we stopped observing the commit timestamp to write it into table
descriptors. In this change I overlooked (rather forgot) about this additional
place in the code where we observed the commit timestamp. As far as I can tell
we don't read this field anywhere ever. Furthermore we know that the the table
descriptor in question to which we are referring must be alive and equal to
the provided value at the timestamp at which it was read due to serializability.
In short, this minor change continues to populate the field with a sensible
value and will permit TRUNCATE to be pushed.
Fixes #41566.
Release note (bug fix): Long running transactions which attempt to TRUNCATE
can now be pushed and will commit in cases where they previously could fail
or retry forever.