Opened 3 months ago

Closed 2 months ago

Last modified 2 months ago

#27356 closed enhancement (fixed)

Reduce database size and variance of query response times

Reported by: karsten Owned by: metrics-team
Priority: High Milestone:
Component: Metrics/ExoneraTor Version:
Severity: Normal Keywords:
Cc: Sebastian, metrics-team Actual Points:
Parent ID: Points:
Reviewer: irl Sponsor:

Description

The ExoneraTor database has grown to 262 GB, and sometimes queries can run for up to a minute or even longer (#17488). Both issues are known for years, and they're getting even worse over time as new data gets added.

Sebastian and I discussed possible mitigations over two years ago, and we came up with an improved database schema. Well, Sebastian did the bulk of the work there.

I picked up this work, rebased it to current master, made some fixes, and ran tests on a local database copy. Let's try to merge and deploy this in the next couple of weeks before the code gets old again like last time.

Child Tickets

Change History (18)

comment:1 Changed 3 months ago by karsten

Status: newneeds_review

Please review commit 8159855 in my task-27356 branch. I might tweak that branch a bit more while running tests, but I don't expect major changes at this point (unless the review brings up any major issues, of course).

comment:2 Changed 3 months ago by karsten

So, while testing, I found that the query for the first loop takes almost forever. I aborted it after 1.5 days and ran EXPLAIN ANALYZE on it:

exonerator=> EXPLAIN ANALYZE SELECT * FROM statusentry ORDER BY fingerprint, validafter;
                                                                 QUERY PLAN                                                                 
--------------------------------------------------------------------------------------------------------------------------------------------
 Sort  (cost=330924653.28..331942841.44 rows=407275264 width=335) (actual time=150799260.447..160427618.123 rows=407470751 loops=1)
   Sort Key: fingerprint, validafter
   Sort Method: external merge  Disk: 132839640kB
   ->  Seq Scan on statusentry  (cost=0.00..22111634.64 rows=407275264 width=335) (actual time=120.830..4751991.603 rows=407470751 loops=1)
 Planning time: 37.125 ms
 Execution time: 160482588.895 ms
(6 rows)

Admittedly, this looks okay. Just to be clear, that's over 1 day and 20 hours before we even start copying the first row!

I tweaked the logging to make that clearer. I also made another trivial code style change that shouldn't have any effect. Please find commits 2cb74cd and deec83d in the same branch mentioned above.

I'll hold off running more tests until the SSD that I ordered yesterday arrives. I figured that if the query alone takes almost 2 days, that the rest of the migration might take many more days or even weeks. Might have been a bit optimistic that an HDD would do the job. Hopefully that will be tomorrow, so I might start another test run this weekend or early next week at the latest.

Still in need for review. :)

comment:3 Changed 3 months ago by karsten

Update: Putting in an SSD reduced the runtime for that initial query from 1 day 20 hours to 1 hour! Migration finished over night in only 10 hours total! I'm now importing the past three weeks of data to catch up. If all goes well, I'd like to upload this database to the server rather than do the migration there again.

As part of testing, I pushed 25ea8a8 with a small but important fix to my branch.

comment:4 Changed 3 months ago by karsten

Priority: MediumHigh

Update: I successfully imported data since and have a migrated, up-to-date database here now. I'll do some more testing tomorrow.

I also talked to Sebastian about the original patch from 2 years ago, and we agreed on some modifications due to minor bugs or code simplifications. Other than that, Sebastian is a bit short on time these days and says that he cannot review the rebased patch as thoroughly as he'd like to in order to green-light it. Which is okay, we'll just have to do the review part ourselves. We shouldn't wait too long, though, because the migrated database copy that I have here is going to age, too. Maybe we can get this done this or next week.

comment:5 Changed 2 months ago by karsten

I pushed ten more commits to the same branch, ending with c9db91d. This concludes my testing, and I'm now waiting for the review. (Thanks!)

comment:6 Changed 2 months ago by karsten

(All commits are squash commits, so for a fresh review they are best reviewed all together.)

comment:7 Changed 2 months ago by irl

Reviewer: irl

Adding to the queue.

comment:8 Changed 2 months ago by irl

Status: needs_reviewneeds_revision

I've checked it over for any obvious errors and the tests pass, but I notice that none of the tests actually use the database.

I'm having difficulty reviewing these as there are many squash commits with lots of commit messages, but I haven't been following from the start. Could you rebase this on another branch with a single commit containing the changes (or just provide the commit message here in the ticket) so that I can understand a) what the changes are meant to be and b) the motivation/intent behind those changes.

comment:9 in reply to:  8 ; Changed 2 months ago by karsten

Status: needs_revisionneeds_review

Replying to irl:

I've checked it over for any obvious errors and the tests pass, but I notice that none of the tests actually use the database.

That's true. Having more useful tests in ExoneraTor is, unfortunately, a little project of its own. We already have #24365 for this, but it's not as high priority as it could/should be. Let's try to leave room for these things in the next roadmap.

I'm having difficulty reviewing these as there are many squash commits with lots of commit messages, but I haven't been following from the start. Could you rebase this on another branch with a single commit containing the changes (or just provide the commit message here in the ticket) so that I can understand a) what the changes are meant to be and b) the motivation/intent behind those changes.

It's a single commit, 8159855, with a couple squash commits. I'd rather not rebase in the middle of the review, as I wouldn't do that for any upcoming changes coming out of this review. I'll rebase before merging, though, but until then, can you check out this branch and run git diff da3eb1f (parent of 8159855) to see all changes together? The commit message of 8159855 explains what the changes are all about.

comment:10 in reply to:  9 ; Changed 2 months ago by irl

Status: needs_reviewmerge_ready

Replying to karsten:

Replying to irl:

I've checked it over for any obvious errors and the tests pass, but I notice that none of the tests actually use the database.

That's true. Having more useful tests in ExoneraTor is, unfortunately, a little project of its own. We already have #24365 for this, but it's not as high priority as it could/should be. Let's try to leave room for these things in the next roadmap.

Ok.

The commit message of 8159855 explains what the changes are all about.

Ok. The changes look to implement what is described and the strategy looks good too.

Thinking about handling schema changes, the comment says that exonerator.sql will go away and the new one will be modified to replace it. I've seen other software keep all the revisions and upgrade scripts since the beginning of the project (for example, observium) and installation starts with the original schema and then upgrades it. Perhaps this is useful to prevent bugs creeping in when the script is changed to replace the original script?

Other than that, I think this is the limit of what I can review without standing up an instance to test on.

comment:11 in reply to:  10 ; Changed 2 months ago by karsten

Replying to irl:

Replying to karsten:

Replying to irl:

I've checked it over for any obvious errors and the tests pass, but I notice that none of the tests actually use the database.

That's true. Having more useful tests in ExoneraTor is, unfortunately, a little project of its own. We already have #24365 for this, but it's not as high priority as it could/should be. Let's try to leave room for these things in the next roadmap.

Ok.

The commit message of 8159855 explains what the changes are all about.

Ok. The changes look to implement what is described and the strategy looks good too.

Perfect! I'll start the migration then by updating the local snapshot and uploading it back to the server. This is probably going to take a few days.

Thinking about handling schema changes, the comment says that exonerator.sql will go away and the new one will be modified to replace it. I've seen other software keep all the revisions and upgrade scripts since the beginning of the project (for example, observium) and installation starts with the original schema and then upgrades it. Perhaps this is useful to prevent bugs creeping in when the script is changed to replace the original script?

We can do that, too. I wonder if there's a way to provide the latest schema after applying all upgrade scripts. Maybe we can auto-generate that by running all scripts and then exporting the schema without data. Without something like this, new contributors will have a hard time going through all scripts just to learn the current state of things.

Other than that, I think this is the limit of what I can review without standing up an instance to test on.

Thanks! At least it's good to know that I did not make any obvious mistakes. And we'll still find out if it's 100% bug free after deploying and running it for a while. ;)

comment:12 in reply to:  11 ; Changed 2 months ago by irl

Replying to karsten:

Without something like this, new contributors will have a hard time going through all scripts just to learn the current state of things.

Observium has all the files in a directory with their version number and a script to run through them in sequence.

Ah, also the version number of the schema should be in the database to allow you to know where you are and how much upgrading you need to do. Running the script twice should produce the same result as running it once.

comment:13 in reply to:  12 ; Changed 2 months ago by karsten

Replying to irl:

Replying to karsten:

Without something like this, new contributors will have a hard time going through all scripts just to learn the current state of things.

Observium has all the files in a directory with their version number and a script to run through them in sequence.

Do they also have an automatically combined version with all changes under source control, or do they otherwise explain how to obtain the latest schema? If this is obvious for their contributors, maybe it's also obvious for our future contributors.

Ah, also the version number of the schema should be in the database to allow you to know where you are and how much upgrading you need to do. Running the script twice should produce the same result as running it once.

Good idea. I'll add such a version table and checks to the existing two scripts.

comment:14 in reply to:  13 ; Changed 2 months ago by irl

Replying to karsten:

Do they also have an automatically combined version with all changes under source control, or do they otherwise explain how to obtain the latest schema? If this is obvious for their contributors, maybe it's also obvious for our future contributors.

Well, you can run all the upgrade scripts in sequence without running the importer and then export the result. I don't think they have an annotated SQL file, but separate documentation.

comment:15 in reply to:  14 Changed 2 months ago by karsten

Replying to irl:

Replying to karsten:

Do they also have an automatically combined version with all changes under source control, or do they otherwise explain how to obtain the latest schema? If this is obvious for their contributors, maybe it's also obvious for our future contributors.

Well, you can run all the upgrade scripts in sequence without running the importer and then export the result. I don't think they have an annotated SQL file, but separate documentation.

Okay.

Regarding version information in the database, I took a less invasive way that keeps the current migration process as simple as possible: the exonerator2.sql script now exits on first error to avoid multiple executions, cf. commit b64f4d1. In the longer term we'll need something more elaborate, but maybe we can design something that then works for all databases, including those used in metrics-web.

Still working on migration/deployment.

comment:16 Changed 2 months ago by karsten

Squashed and merged to master. Will be released as part of 4.0.0 and deployed shortly after. See #27697 for details. Leaving this ticket open until after deployment.

comment:17 Changed 2 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

Released. Closing. Thanks, Sebastian and irl!

comment:18 Changed 2 months ago by karsten

irl asked me today how this change improved query times. Here are some quick statistics based on log statements over roughly one 1.5 weeks saying how much time passed between opening a connection and returning it:

Statistic Before change After change
Min. 37 2068
1st Qu. 170 2173
Median 191 2197
Mean 18912 2275
3rd Qu. 260 2235
Max. 3008467 17136

Despite the increase in minimum, first quartile, median, and even third quartile, I'd say that the decrease in mean and in particular maximum values make this change a huge improvement. While it's cool that some or even most responses in the old approach were really fast, it was the very high response times that made the service hard to predict. With the new approach, users can expect a request to return within a few seconds.

Note: See TracTickets for help on using tickets.