feat(server): next-page prefetch cache for paginated SELECT queries#391
feat(server): next-page prefetch cache for paginated SELECT queries#391
Conversation
Co-authored-by: rrobetti <[email protected]>
…yInternal Co-authored-by: rrobetti <[email protected]>
|
@copilot what can we do about these limitations? Ideally we should be able to cache any type of data including LOB types. Can we not keep them in memory as well ? It might ok to skip queries that receive a LOB as parameter, but as a return type in a query we should be able to cache it. |
Co-authored-by: rrobetti <[email protected]>
Done in commit The only remaining skip is for queries that pass LOB session references as input parameters (BLOB/CLOB Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot some questions in relation to this implementation: |
…-page prefetch cache Co-authored-by: rrobetti <[email protected]>
Both addressed in commit 1 - Cache isolation by datasource: Yes, it's now fully isolated. The cache key is 2 - Background cleanup job: A daemon Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot make sure there is only one background job that checks all cached objects, there is no need for multiple threads cleaning. |
…thread across all cache instances Co-authored-by: rrobetti <[email protected]>
|
@copilot create an integration test for Postgres with pagination. The test should create different amounts of records (99, 100, 101, 567(random), 1000 - make sure you use parameterized tests, not duplications) in a given table that contains multiple types and at least one LOB type. There should be tests paginating 100 records per page. You have to run a separate OJP server in the main CI workflow on a different port with pagination cache enabled and point the new integration test at this new server. You can use the same postgres db already present in the workflow, just name the table uniquely. Make sure all records are returned correctly when using cache and pagination, and assert every value. If a lob makes sure all bytes are returned correctly. |
Done in commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
… assertion Co-authored-by: rrobetti <[email protected]>
| */ | ||
| private static final ScheduledExecutorService CLEANUP_EXECUTOR = | ||
| Executors.newSingleThreadScheduledExecutor(r -> { | ||
| Thread t = new Thread(r, "ojp-prefetch-cache-cleanup"); |
There was a problem hiding this comment.
Fixed in commit 73a4d18. The thread factory now uses Thread.ofVirtual().name("ojp-prefetch-cache-cleanup").unstarted(r) instead of new Thread(r, ...). The explicit setDaemon(true) is no longer needed since virtual threads are always daemon threads. All 434 tests pass.
Done in commit New test class —
CI workflow —
Concern/note: PostgreSQL does not have a native |
…orts, condition negation Co-authored-by: rrobetti <[email protected]>
Fixed in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
… compilation, boolean pattern, catch(Exception) Co-authored-by: rrobetti <[email protected]>
Fixed in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| } | ||
|
|
||
| @Test | ||
| void detectOffsetFetch_sqlServer() { |
There was a problem hiding this comment.
@copilot Replace these 7 tests with a single Parameterized one.
There was a problem hiding this comment.
Done in 0293f8f. The 7 individual tests are replaced by a single @ParameterizedTest + @CsvSource covering the same 7 SQL patterns.
| } | ||
|
|
||
| @Test | ||
| void prefetchAsync_doesNothing_whenSqlIsNull() throws Exception { |
There was a problem hiding this comment.
@copilot Add at least one assertion to this test case.
There was a problem hiding this comment.
Done in 0293f8f. Added assertEquals(0, cache.cacheSize(), "Cache should remain empty when SQL is null").
| } | ||
|
|
||
| @AfterEach | ||
| void cleanupProperties() { |
There was a problem hiding this comment.
@update this method so that its implementation is not identical to "clearProperties" on line 23.
There was a problem hiding this comment.
Done in 0293f8f. cleanupProperties() now delegates to clearProperties() instead of duplicating its body.
…erEach Co-authored-by: rrobetti <[email protected]>
| } | ||
|
|
||
| @Test | ||
| void perDatasource_prefetchWaitTimeoutMs_fallsBackToGlobalDefault_whenNotSet() { |
There was a problem hiding this comment.
@copilot Replace these 3 tests with a single Parameterized one.
There was a problem hiding this comment.
Done in fab19cd. The 3 individual tests (whenNotSet, forNullName, forDefaultName) are replaced by a single @ParameterizedTest @NullSource @ValueSource(strings = {"unknown-ds", "default"}) — whenNoPerDatasourcePropertySet.
| cache.prefetchAsync(ds, "ds1", sql, List.of()); | ||
|
|
||
| // Wait a bit to ensure the prefetch completes and the entry is expired | ||
| Thread.sleep(50); |
There was a problem hiding this comment.
@copilot add line comment //NOSONAR to skip sonar scan
There was a problem hiding this comment.
Done in fab19cd. Added //NOSONAR to Thread.sleep(50) at line 202.
| @Test | ||
| void cachedPage_isExpired_withZeroTtl() throws Exception { | ||
| CachedPage page = new CachedPage(List.of("col"), List.of()); | ||
| Thread.sleep(10); // small delay so currentTime > createdAt |
There was a problem hiding this comment.
@copilot add line comment //NOSONAR to skip sonar scan
There was a problem hiding this comment.
Done in fab19cd. Added //NOSONAR to Thread.sleep(10) at line 239.
| // Wait (with polling) for the background cleanup to reduce the cache size to 0 | ||
| long deadline = System.currentTimeMillis() + 5_000; | ||
| while (cache.cacheSize() > 0 && System.currentTimeMillis() < deadline) { | ||
| Thread.sleep(50); |
There was a problem hiding this comment.
@copilot add line comment //NOSONAR to skip sonar scan
There was a problem hiding this comment.
Done in fab19cd. Added //NOSONAR to Thread.sleep(50) at line 442.
| // ---------------------------------------------------------------- | ||
|
|
||
| @Test | ||
| void shutdown_doesNotThrow_whenSchedulerNotStarted() { |
There was a problem hiding this comment.
@copilot Add at least one assertion to this test case.
There was a problem hiding this comment.
Done in fab19cd. Added assertEquals(0, cache.cacheSize(), "Cache should remain empty after shutdown").
| } | ||
|
|
||
| @Test | ||
| void shutdown_isIdempotent() { |
There was a problem hiding this comment.
@copilot Add at least one assertion to this test case.
There was a problem hiding this comment.
Done in fab19cd. Added assertEquals(0, cache.cacheSize(), "Cache should remain empty after double shutdown").
| // ---------------------------------------------------------------- | ||
|
|
||
| @Test | ||
| void registerDatasourcePrefetchWaitTimeout_ignoresNullId() { |
There was a problem hiding this comment.
@copilot Add at least one assertion to this test case.
There was a problem hiding this comment.
Done in fab19cd. Added assertEquals(0, cache.cacheSize(), "Cache should remain empty when datasourceId is null").
|
@copilot, before this change for pagination, we already had an intrinsic pagination which would stream records 100 by 100 by default to the client, what is the interaction of this current pagination preload and cache we are doing in this PR with the 100 by 100 stream currently implemented? |
Co-authored-by: rrobetti <[email protected]>
The two mechanisms operate at different levels and don't interfere with each other: Intrinsic gRPC streaming ( New prefetch cache (this PR): This is the coordination layer. It detects SQL with a recognised pagination clause ( When a cached page is served via In short: the intrinsic streaming controls how many rows fit in one gRPC message; the prefetch cache controls whether the next page query hits the database at all.
These two mechanisms operate at completely different levels and are fully complementary — there's no conflict. Existing intrinsic streaming (100 rows per gRPC block) New prefetch cache (this PR) How they interact on a typical paginated request:
So the 100-row gRPC chunking always applies (live or cached), and the prefetch cache only affects whether we go to the DB or serve from memory for subsequent pages. |
rrobetti
left a comment
rrobetti
left a comment
There was a problem hiding this comment.
@copilot update relevant documentation about this feature. Create new if appropriate. Make sure you update relevant sections of the eBook as well. Make sure you update documents related to configurations, adding the configurations added in this PR.
| // ---------------------------------------------------------------- | ||
|
|
||
| @Test | ||
| void buildNextPage_limitOffset_incrementsOffset() { |
There was a problem hiding this comment.
@copilot Replace these 7 tests with a single Parameterized one.
There was a problem hiding this comment.
Done in 76f8de7. The 7 individual buildNextPageSql() tests are replaced by a single @ParameterizedTest @CsvSource covering all 7 SQL patterns (LIMIT/OFFSET, OFFSET FETCH, MySQL LIMIT m,n, FETCH FIRST without OFFSET, and standalone LIMIT).
…ests to single @ParameterizedTest Co-authored-by: rrobetti <[email protected]>
Done in
|
|
prefetchWaitTimeoutMsconfigurationbuildNextPageSql()tests with single@ParameterizedTestdocuments/features/NEXT_PAGE_PREFETCH_CACHE.mddocuments/configuration/ojp-server-configuration.md— added Next-Page Prefetch Cache Settings sectiondocuments/ebook/part2-chapter6-server-configuration.md— added section 6.8 on the prefetch cachedocuments/README.md— added Features section andfeatures/to directory tree📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.