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

Faster server variable handling #4567

Closed
wants to merge 2 commits into from
Closed

Conversation

TeoTwawki
Copy link
Contributor

@TeoTwawki TeoTwawki commented Sep 30, 2023

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

This definitely DOES speed it up:

  • Before: 277ms - 550ms
  • After: 7ms - 329ms

Buuuuut...

The thing is that range on these bothers me. those are quite large gaps between the smallest and largest times I saw. Different sources online make conflicting claims about which operations are faster. One person claiming REPLACE statements are 32x slower than ON DUPLICATE KEY UPDATE yet in my testing REPLACE was 200% faster, other claiming ON DUPLICATE KEY UPDATE was faster than a regular UPDATE statement, which I don't see how that can be physically possible - on duplicate attempt an insert and then when it fails, does an update. its literally the same update. REPLACE does a delete followed by an insert, and has minefields for situations involving foreign keys and auto increments.

So I did "real world testing" my dang self. You clearly can't trust what you read. And then I reached out to some experts I'm waiting to hear back from.

I'm not sure I want to move forward wit this yet, so its in draft mode pending response from persons more experienced than myself in SQL communities.

Here's some data in the meantime:

image

Steps to test these changes

Print those sql times

        else
        {
            ShowInfo(fmt::format("SQL query took {}ms: {}", dTime.count(), self->buf));
        }

and run some queries that were updated in this pr.

@github-actions
Copy link

✨ Thanks for the PR! ✨

This is a friendly automated reminder that the maintainers won't look at your PR until you've properly completed all of the checkboxes in the pre-filled template.

@TeoTwawki
Copy link
Contributor Author

Decided this is definitely NOT the direction I want to try and patch this up with. I still don't understand why the servervar queries are being this slow on having to update existing rows. There only one key here and neither the table nor the row should have a bad locking problem going on (we do 3 tries at reading it to confirm it was written, but that is going to take < 30ms in total).

@TeoTwawki TeoTwawki closed this Oct 12, 2023
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.

1 participant