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

sa: truncate times in type converter #7556

Merged
merged 3 commits into from
Jun 24, 2024
Merged

sa: truncate times in type converter #7556

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):

"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.

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, beautifulentropy and aarongable and removed request for a team June 24, 2024 15:00
@aarongable
Copy link
Contributor

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
12 checks passed
@jsha jsha deleted the update-borp branch June 24, 2024 18:26
jsha added a commit that referenced this pull request Jun 27, 2024
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.
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.

3 participants