Do persistent cache writebacks asynchronously #7025
Conversation
msullivan
changed the title
edb/server/dbview/dbview.pyx
Outdated
| added_since_signal | ||
| and ( | ||
| self._cache_queue.empty() | ||
| or added_since_signal > 100 |
There was a problem hiding this comment.
Ideally we want to implement a "debounce" pattern here. Basically it would be:
- A task that exists solely to send the
query-cache-changessignal - Every time we call it it would extend the wait for some
T_ADDtime - When wait time elapses it would send the PG signal
- If the total time waiting is longer than T_MAX it should send a signal and be back to square one
This would:
- eliminate magic numbers like 100
- save the system from spamming everyone with PG events
- make the system reasonable with respect to the max lag it can take before notifying others
There was a problem hiding this comment.
Yeah I planned to do something along those lines, but I wanted to see if the aarch64 trouble recurred after these changes without it, but the aarch64 builds are too broken because of spot instance trouble to know yet really
There was a problem hiding this comment.
Alright I implemented something. Let me know if it basically matches what you were thinking
There was a problem hiding this comment.
I read the code a few times and I think it's good. I want @fantix to thoroughly read it too.
| @@ -174,6 +174,9 @@ cdef class Database: | |||
| metrics.background_errors.inc( | |||
There was a problem hiding this comment.
^^ Also we should probably stop using debug.dump directly in code and do proper logging.error or whatnot. If it's not printing when debugging that should be debugged
| # Empty the queue, for batching reasons. | ||
| ops = [await self._cache_queue.get()] | ||
| while not self._cache_queue.empty(): | ||
| ops.append(self._cache_queue.get_nowait()) |
There was a problem hiding this comment.
I'd add a comment that this is safe for asyncio queues (unlike thread queues, where the empty check /get_nowait combo is not guaranteed to succeed due to races)
There was a problem hiding this comment.
(I froze here for a few moments thinking, mistakingly, that your code is incorrect)
There was a problem hiding this comment.
Oh yeah; I felt naughty while doing this, but should have added a comment
edb/server/protocol/execute.pyx
Outdated
| if query_unit.tx_rollback: | ||
| be_conn.last_state = None |
There was a problem hiding this comment.
What was out of sync that led to this change? We should fix that instead.
Not updating last_state here will cripple this pgcon with a mismatching state, a concurrent frontend connection with a different state may pick it up and run into wrong data.
There was a problem hiding this comment.
Good catch.
The problem was that run_ddl didn't sync state in all cases.
edb/server/dbview/dbview.pyx
Outdated
| except TimeoutError: | ||
| t = time.time() | ||
| else: | ||
| pending_keys.append(str(key)) |
There was a problem hiding this comment.
I would also drain the queue up to MAX_KEYS pending keys here, because asyncio.wait_for() jumps at least 1 iteration in the event loop if timeout is not None, and we may end up with 100 tight asyncio event loops with our while True loop, which sounds like an expensive CPU moment.
There was a problem hiding this comment.
I may just try changing it to use timeout_at instead of wait_for, which looks like it won't create a task and so won't yield back to the event loop in the case of get succeeding immediately.
One reason I'm reluctant to switch to explicitly draining the queue is because the latest version has refactored it to just have an input function instead of operating on a queue (though I could change it back to having an input queue).
|
I pushed some changes but they aren't visible yet, and there appears to be a github incident: https://www.githubstatus.com/incidents/q3gl44chgxwv |
This is extracted from #7025, for history cleanliness and backportability.
This is extracted from #7025, for history cleanliness and backportability.
This reverts commit f04237c.
Whenever a entry is added to the in memory cache, enqueue it in a queue that a per-db worker task consumes. The worker task manages all cache inserts and evictions, which should fully eliminate the serialization error dangers. This also lets us persistently cache queries that were first compiled from within a rolled back transaction. I also re-enabled the query-cache-changes notifications, with some very minor rate limiting. We'll see how it goes. FUTURE NOTE: The situation for function caching once we want to reintroduce that will be a little more complicated. We'll need to put both the SQL text and the function call code in the in-memory cache, and use the text until the function is visible everywhere (at which point we can drop the text.) We'll also need to do some thinking about how to test it properly, because the downside of the approach is that in the typically path, the first execution of a query can't use the function. We may need some sort of testing path to allow us to exercise the functions easily in the test code.
Something was getting out of sync, and the extra connection traffic was exposing it.
The point of this is to make it testable though I haven't *actually* written tests
This reverts commit e2f1750.
| while True: | ||
| try: | ||
| async with asyncio.timeout_at(target_time): | ||
| v = await input() |
There was a problem hiding this comment.
Hm, so you will be waiting with timeout even for the first input() return, but that's not what we want to do. We don't want to keep looping through timeouts until input() returns for the first time.
We want to start debouncing only when it returns. The first await on it shouldn't be with a timeout at all.
There was a problem hiding this comment.
Oh sigh, this is why I don't like None for cases like this. Please add an if.
if target_time is not None:
async with ...
else:
await input()
Whenever a entry is added to the in memory cache, enqueue it in a queue that a per-db worker task consumes. The worker task manages all cache inserts and evictions, which should fully eliminate the serialization error dangers. This also lets us persistently cache queries that were first compiled from within a rolled back transaction. Also, reenable query-cache-notifications, but make them include the specific keys to query and implement a debouncing/batching algorithm to cut down on traffic. Unfortunately, even with query-cache-notifications off, the async caching starts triggering the bizarre failures to lookup transactions in postgres on arm64 linux. I'm going to put up a follow up that disables the cache there, so we can move ahead with testing and the release. FUTURE NOTE: The situation for function caching once we want to reintroduce that will be a little more complicated. We'll need to put both the SQL text and the function call code in the in-memory cache, and use the text until the function is visible everywhere (at which point we can drop the text.) We'll also need to do some thinking about how to test it properly, because the downside of the approach is that in the typically path, the first execution of a query can't use the function. We may need some sort of testing path to allow us to exercise the functions easily in the test code.
This is extracted from #7025, for history cleanliness and backportability.
Whenever a entry is added to the in memory cache, enqueue it in a queue that a per-db worker task consumes. The worker task manages all cache inserts and evictions, which should fully eliminate the serialization error dangers. This also lets us persistently cache queries that were first compiled from within a rolled back transaction. Also, reenable query-cache-notifications, but make them include the specific keys to query and implement a debouncing/batching algorithm to cut down on traffic. Unfortunately, even with query-cache-notifications off, the async caching starts triggering the bizarre failures to lookup transactions in postgres on arm64 linux. I'm going to put up a follow up that disables the cache there, so we can move ahead with testing and the release. FUTURE NOTE: The situation for function caching once we want to reintroduce that will be a little more complicated. We'll need to put both the SQL text and the function call code in the in-memory cache, and use the text until the function is visible everywhere (at which point we can drop the text.) We'll also need to do some thinking about how to test it properly, because the downside of the approach is that in the typically path, the first execution of a query can't use the function. We may need some sort of testing path to allow us to exercise the functions easily in the test code.
This is extracted from #7025, for history cleanliness and backportability.
This is extracted from #7025, for history cleanliness and backportability.
Whenever a entry is added to the in memory cache, enqueue it in a
queue that a per-db worker task consumes. The worker task manages all
cache inserts and evictions, which should fully eliminate the
serialization error dangers. This also lets us persistently cache
queries that were first compiled from within a rolled back
transaction.
I also re-enabled the query-cache-changes notifications, with some
very minor rate limiting. We'll see how it goes.
Thoughts?
FUTURE NOTE: The situation for function caching once we want to
reintroduce that will be a little more complicated. We'll need to put
both the SQL text and the function call code in the in-memory cache,
and use the text until the function is visible everywhere (at which
point we can drop the text.) We'll also need to do some thinking about
how to test it properly, because the downside of the approach is that
in the typically path, the first execution of a query can't use the
function. We may need some sort of testing path to allow us to
exercise the functions easily in the test code.