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

Disable cache outside transactions #2067

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Conversation

tylerkaraszewski
Copy link
Contributor

Details

This disables caching query results when we're not in a transaction (particularly, in prePeek). Because there is no transaction, there's no way to know if the database has changed after any of these queries. This was discovered in the HC-Tree version of JoinAccessiblePolicyTest but it could happen in any command with prePeek.

Fixed Issues

https://github.com/Expensify/Expensify/issues/337537

Tests


Internal Testing Reminder: when changing bedrock, please compile auth against your new changes

Copy link
Contributor

@iwiznia iwiznia left a comment

Choose a reason for hiding this comment

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

So is it better to not have a query cache in prePeek or to start a transaction in prePeek so we can take advantage of the cache?

@cead22
Copy link
Contributor

cead22 commented Jan 21, 2025

Because there is no transaction, there's no way to know if the database has changed after any of these queries

Why is this an issue?

@iwiznia
Copy link
Contributor

iwiznia commented Jan 21, 2025

Why is this an issue?

I assume it is because the cache needs to be invalidated when there's a write

@cead22
Copy link
Contributor

cead22 commented Jan 23, 2025

Yeah but I was trying to understand where/how we do that and why it matters whether we're in a transaction.

Looking at the code, I still don't understand how the issue would happen. We clear _queryCache in beginTransaction, rollback and _writeIdempotent. If we run a query in prePeek and cache it, and another query runs in prePeek:

  • If we wrote something to the database before, we would've cleared the cache in one of the methods mentioned above
  • If we didn't write anything into the database, the cached query should be current

What am I missing?

As an aside, I checked logs to see how frequently we use this cache, and it looks like we use it a ton, so this optimization is probably a very good one

@tylerkaraszewski
Copy link
Contributor Author

tylerkaraszewski commented Jan 31, 2025

This happens when two different commands run simultaneously. The query cache is per DB handle.

So if you run a query in prePeek in one command, and then a different commands updates the DB, and then the first command runs the same query again, it will get the same result as the first time, even though it is no longer accurate.

The cache is command 1 is not cleared when command 2 performs a write. This never mattered inside a transaction, but does without transactions.

This doesn't only affect a single command, either.

If prePeek does a query and then throws an exception to finish the command, the queryCache for the DB handle contains the result of that query.

If another command is given the same DB handle, and attempts the same query, it will get the cached result from the previous command that ran it. It affects postProcess as well for the same reasons.

Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

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

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Jan 31, 2025

As an aside, I checked logs to see how frequently we use this cache, and it looks like we use it a ton, so this optimization is probably a very good one

I don't think this counts as an "optimization", it is fixing a bug where we read from the cache stale data. I understand that It can potentially make things slower, not faster because you are no longer caching queries in prePeek/postProcess.

@tylerkaraszewski tylerkaraszewski merged commit 76d5d4f into main Jan 31, 2025
1 check passed
@tylerkaraszewski tylerkaraszewski deleted the tyler-fix-bad-query branch January 31, 2025 23:36
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.

4 participants