Skip to content

sa: truncate times in type converter #7556

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.

Sign up for GitHub

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

Jump to bottom
Merged
jsha merged 3 commits into from
Jun 24, 2024
Merged

sa: truncate times in type converter #7556

jsha merged 3 commits into from
Jun 24, 2024

Conversation

@jsha
Copy link
Contributor

@jsha jsha commented Jun 20, 2024

We believe the MariaDB query planner generates inefficient query plans when a time index is queried using high precision (nanosecond) times. This uses the updated borp from letsencrypt/borp#11 to automatically truncate time.Time and *time.Time in query parameters.

Part of #5437

@jsha jsha requested a review from a team as a code owner June 20, 2024 19:54
@jsha jsha requested a review from pgporada June 20, 2024 19:54
@jsha
Copy link
Contributor Author

jsha commented Jun 20, 2024

BTW thanks to @aarongable and @beautifulentropy for encouraging me to find a more systemic fix in #7519 (review).

I did a version of this change where instead of truncating, it would panic if it received an untruncated time. It found a couple of call sites that I missed in #7519 which are probably pretty important to performance (SELECTs against time indexes that are called on new-order):

boulder/sa/rate_limits.go

Lines 75 to 77 in 8545ea8

"earliest": timeRange.Earliest.AsTime(),
"latest": latest,
})

boulder/sa/rate_limits.go

Lines 131 to 133 in 8545ea8

"earliest": req.Range.Earliest.AsTime(),
"latest": req.Range.Latest.AsTime(),
},

@jsha
Copy link
Contributor Author

jsha commented Jun 20, 2024

Also, once this change lands I'll submit a separate revert of #7519.

pgporada
pgporada approved these changes Jun 24, 2024
View reviewed changes
Copy link
Member

@pgporada pgporada left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I have a borp question.

Do queries wrapped in a (t *Transaction) get unwrapped for lack of a better term into their underlying (m *DbMap) so that both transactional and non-transactional queries run through m.convertArgs?

@pgporada pgporada requested review from a team, aarongable and beautifulentropy and removed request for a team June 24, 2024 15:00
@aarongable
Copy link
Contributor

aarongable commented Jun 24, 2024

Do queries wrapped in a (t *Transaction) get unwrapped for lack of a better term into their underlying (m *DbMap) so that both transactional and non-transactional queries run through m.convertArgs?

We create transactions using DbMap.BeginTx, which contains the original dbmap as a struct member and passes it into a set of helper functions which end up calling the methods (like QueryContext) which now do the transformation.

@jsha jsha merged commit c0ffbac into main Jun 24, 2024
@jsha jsha deleted the update-borp branch June 24, 2024 18:26
@jsha jsha mentioned this pull request Jun 24, 2024
Merged
jsha added a commit that referenced this pull request Jun 27, 2024
@jsha
Revert "sa: truncate all timestamps to seconds (#7519)" (#7559)
3baea43 
This reverts commit 2b5b623.

Following up on #7556, after we made a more systematic change to use
borp's TypeConverter, we no longer need to manually truncate timestamps.
@aarongable aarongable mentioned this pull request Aug 2, 2024
Closed
@jsha jsha mentioned this pull request Oct 22, 2024
Closed
@aarongable aarongable mentioned this pull request Apr 22, 2025
Merged
aarongable added a commit that referenced this pull request Apr 25, 2025
@aarongable
Update go-sql-driver/mysql from v1.5.0 to v1.7.1 (#8138)
bc899ac 
Version v1.5.0 was released in January 2020, over five years ago. We
have attempted to update this package several times since then -- first
to v1.6.0, later to v1.7.1 -- but have reverted the change due to
nigh-inexplicable performance regressions each time. Since our last
attempt, we believe we have addressed the underlying issue by truncating
timestamps when we talk to the database (see
#7556) so that our indices
don't try to track nanosecond precision.

We are now ready to reattempt updating this package to v1.7.1 again. If
that goes well, we will further update it to the newest version.

Fixes #5437
Part of #7872
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pgporada pgporada pgporada approved these changes

@aarongable aarongable aarongable approved these changes

@beautifulentropy beautifulentropy Awaiting requested review from beautifulentropy beautifulentropy is a code owner automatically assigned from letsencrypt/boulder-developers

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jsha @aarongable @pgporada