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

sqlstats: speed up flush operation #123476

Closed
xinhaoz opened this issue May 2, 2024 · 0 comments · Fixed by #123254
Closed

sqlstats: speed up flush operation #123476

xinhaoz opened this issue May 2, 2024 · 0 comments · Fixed by #123254
Assignees
Labels
branch-release-24.1 Used to mark GA and release blockers and technical advisories for 24.1 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability

Comments

@xinhaoz
Copy link
Member

xinhaoz commented May 2, 2024

The sql stats flush currently uses a 3 part txn to update a row in the sql stats tables on flush:
INSERT -> SELECT FOR UDPATE -> UPDATE.
This makes the update row operation much slower, increasing the overfall flush time.

This txn be shortened to a single statement, INSERT .. ON CONFLICT UPDATE ....

Jira issue: CRDB-38367

Epic CRDB-37544

@xinhaoz xinhaoz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker branch-release-24.1 Used to mark GA and release blockers and technical advisories for 24.1 T-observability labels May 2, 2024
@xinhaoz xinhaoz self-assigned this May 2, 2024
craig bot pushed a commit that referenced this issue May 3, 2024
123254: sqlstats: rewrite SELECT FOR UDPATE in flush as INSERT UPDATE ON CONFLICT r=xinhaoz a=xinhaoz

Please note only the latest commit is for review in this comment.

-------------------

Previously, the UPDATE logic for flushing sql stats
was split into a 3 part transaction due to using
SELECT FOR UPDATE -
1. Attempt to INSERT
2. SELECT row FOR UPDATE
3. UPDATE

We can make this faster by using INSERT .. ON CONFLICT UPDATE.

```
Previous:
BenchmarkSqlStatsMaxFlushTime/single-application/writes=update/10000-fingerprints-10    1     32487051083 ns/op

New:
BenchmarkSqlStatsMaxFlushTime/single-application/writes=update/10000-fingerprints-10    1     13356979917 ns/op
```

Epic: none
Fixes: #123476
Release note: None

Co-authored-by: Xin Hao Zhang <xzhang@cockroachlabs.com>
@craig craig bot closed this as completed in 98ae09a May 3, 2024
blathers-crl bot pushed a commit that referenced this issue May 3, 2024
…LICT

Previously, the UPDATE logic for flushing sql stats
was split into a 3 part transaction due to using
SELECT FOR UPDATE -
1. Attempt to INSERT
2. SELECT row FOR UPDATE
3. UPDATE

We can make this faster by using INSERT .. ON CONFLICT
UPDATE.

```
Previous:
BenchmarkSqlStatsMaxFlushTime/single-application/writes=update/10000-fingerprints-10    1     32487051083 ns/op

New:
BenchmarkSqlStatsMaxFlushTime/single-application/writes=update/10000-fingerprints-10    1     13356979917 ns/op
```

Epic: none
Fixes: #123476

Release note: None
@xinhaoz xinhaoz removed the GA-blocker label May 3, 2024
xinhaoz added a commit that referenced this issue May 4, 2024
…LICT

Previously, the UPDATE logic for flushing sql stats
was split into a 3 part transaction due to using
SELECT FOR UPDATE -
1. Attempt to INSERT
2. SELECT row FOR UPDATE
3. UPDATE

We can make this faster by using INSERT .. ON CONFLICT
UPDATE.

```
Previous:
BenchmarkSqlStatsMaxFlushTime/single-application/writes=update/10000-fingerprints-10    1     32487051083 ns/op

New:
BenchmarkSqlStatsMaxFlushTime/single-application/writes=update/10000-fingerprints-10    1     13356979917 ns/op
```

Epic: none
Fixes: #123476

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-24.1 Used to mark GA and release blockers and technical advisories for 24.1 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant