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

jobs: protected timestamps refresh logic is not transactional #92692

Merged
merged 1 commit into from
Nov 30, 2022

Conversation

fqazi
Copy link
Collaborator

@fqazi fqazi commented Nov 29, 2022

Fixes: #92427

Previously, the refresh logic for protected timestamps would fetch jobs with transactions before attempting to update an existing one. Due to a recent change to allow this API to reflect any changes into individual job objects, we no longer do that correctly. This patch fixes the protected timestamp manager to update timestamps in a transactionally, safe manner since multiple updates can be happening concurrently for schema changes.

Release note: None

Fixes: cockroachdb#92427

Previously, the refresh logic for protected timestamps would fetch jobs with
transactions before attempting to update an existing one. Due to a recent change
to allow this API to reflect any changes into individual job objects, we no
longer do that correctly. This patch fixes the protected timestamp manager
to update timestamps in a transactionally, safe manner since multiple updates
can be happening concurrently for schema changes.

Release note: None
@fqazi fqazi requested a review from a team November 29, 2022 21:04
@fqazi fqazi marked this pull request as ready for review November 29, 2022 21:21
@fqazi fqazi requested a review from a team as a code owner November 29, 2022 21:21
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@msbutler
Copy link
Collaborator

ooof, thanks for catching this!

@fqazi
Copy link
Collaborator Author

fqazi commented Nov 29, 2022

@ajwerner @msbutler TFTR!

@fqazi
Copy link
Collaborator Author

fqazi commented Nov 29, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Nov 30, 2022

Build succeeded:

@craig craig bot merged commit b21fd88 into cockroachdb:master Nov 30, 2022
ajwerner added a commit to ajwerner/cockroach that referenced this pull request Nov 30, 2022
craig bot pushed a commit that referenced this pull request Nov 30, 2022
92632: opt: fix rare incorrect results due to sort between paired joins r=DrewKimball a=DrewKimball

Previously, it was possible for paired joins to produce incorrect results in the case when an ordering was required of their output, and a sort was added between the paired joins to enforce the ordering.

This patch prevents a sort from being added to the output of the first join in a set of paired joins. This is necessary because the continuation column that is used to indicate false positives matched by the first join relies on the ordering being maintained between the joins.

Fixes #89603

Release note: None

92669: roachtest/cdc: export stats for initial scan test to roachperf r=jayshrivastava a=jayshrivastava

This change updates the cdc/initial_scan_only test to produce a `stats.json` artifact to be consumed by roachprod. This file contains stats for p99 foreground latency, changefeed throughput, and CPU usage.

Release note: None
Epic: None

<img width="940" alt="image" src="https://user-images.githubusercontent.com/18633281/204564990-740e86e2-5c43-4d45-a715-4932428a5851.png">


92693: dev: add rewritable paths for ccl execbuilder tests r=rharding6373 a=rharding6373

There are some ccl tests that use test files in
`/pkg/sql/opt/exec/execbuilder`. This commit adds this as a rewritable path so that we can use the `--rewrite` flag with `dev`.

Release note: None
Epic: None

92695: sqlstats: record idle latency for transactions r=matthewtodd a=matthewtodd

Part of #86667
Follows #91098

Release note (sql change): A new NumericStat, idleLat, was introduced to the statistics column of crdb_internal.transaction_statistics, reporting the time spent waiting for the client to send statements while holding a transaction open.

92760: streamclient: replace usage of deprecated ioutil.ReadFile function r=stevendanna a=andyyang890

This patch fixes a lint error resulting from a usage of the
deprecated ioutil.ReadFile function.

Fixes #92761 

Release note: None

92763: jobsprotectedtsccl: unskip TestJobsProtectedTimestamp r=ajwerner a=ajwerner

It was fixed by #92692.

Fixes #91865.

Release note: None

Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
Co-authored-by: Jayant Shrivastava <jayants@cockroachlabs.com>
Co-authored-by: rharding6373 <rharding6373@users.noreply.github.com>
Co-authored-by: Matthew Todd <todd@cockroachlabs.com>
Co-authored-by: Andy Yang <yang@cockroachlabs.com>
Co-authored-by: Andrew Werner <awerner32@gmail.com>
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.

TestCCLLogic_regional_by_row flaked on master
3 participants