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

bench: lower attach concurrency #8737

Closed
wants to merge 1 commit into from
Closed

Conversation

koivunej
Copy link
Member

it has been observed that we get transaction serialization issues otherwise.

evidence: https://neon-github-public-dev.s3.amazonaws.com/reports/main/10402057346/index.html#suites/c62b105f3a4f00dd6be4ad88810e0e02/7c03011d9dbd5ec4/

Slack thread: https://neondb.slack.com/archives/C060CNA47S9/p1723722430992019

Alternatively we could:

  • first issue attach hooks sequentially then attach concurrently
  • just not do it at all (unsure why we even bother with overriding the generations)

@koivunej koivunej added the run-benchmarks Indicates to the CI that benchmarks should be run for PR marked with this label label Aug 15, 2024
@koivunej koivunej requested a review from problame August 15, 2024 12:28
@problame
Copy link
Contributor

problame commented Aug 15, 2024

I remember adding the concurrent attach because it was one of the slowest parts of the setup code at the time.

The serialization failures in storcon db: why? aren't these separate rows for each tenant?
Smells like a suboptimal SQL query in storcon to me, wouldn't we hit the same problem in prod?

Copy link
Contributor

@problame problame left a comment

Choose a reason for hiding this comment

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

Commented

@koivunej
Copy link
Member Author

koivunej commented Aug 15, 2024

The serialization failures in storcon db: why? aren't these separate rows for each tenant?

Yes, but it might be that uniqueness constraint on tenant gets to witness all or some subset of other rows, maybe?

Smells like a suboptimal SQL query in storcon to me, wouldn't we hit the same problem in prod?

I don't think we do large scale generation bumps in prod, but possibly yes. Though the attach-hook mentions it's testing-only hook.

@koivunej
Copy link
Member Author

The serialization failures in storcon db: why? aren't these separate rows for each tenant?

Yes, but it might be that uniqueness constraint on tenant gets to witness all or some subset of other rows, maybe?

refreshing on https://www.postgresql.org/docs/current/transaction-iso.html it would appear that we should first query instead of directly insert (and run into constraint violation). Let's fix it that way.

Copy link

2252 tests run: 2173 passed, 0 failed, 79 skipped (full report)


Code coverage* (full report)

  • functions: 32.4% (7220 of 22266 functions)
  • lines: 50.4% (58300 of 115727 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
b6a2c39 at 2024-08-15T14:21:20.589Z :recycle:

@koivunej
Copy link
Member Author

Discussed this PR, nothe better protocol of check + insert shouldn't be introduced with a test change.

@koivunej koivunej closed this Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-benchmarks Indicates to the CI that benchmarks should be run for PR marked with this label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants