Skip to content

Fix query plan cache race condition - #31859

Merged
FabianMeiswinkel merged 5 commits into
Azure:mainfrom
kushagraThapar:fix_query_plan_cache_race_condition
Nov 1, 2022
Merged

Fix query plan cache race condition#31859
FabianMeiswinkel merged 5 commits into
Azure:mainfrom
kushagraThapar:fix_query_plan_cache_race_condition

Conversation

@kushagraThapar

@kushagraThapar kushagraThapar commented Nov 1, 2022

Copy link
Copy Markdown
Member

Description

Query plan cache get size and put operation is not synchronized, which is causing a rare race condition when we check the size of the cache and try to clear it.

Race Condition: In a multi-threading environment, query plan cache size can grow in a non-synchronized manner. Because of this there is a race condition when another thread checks the below condition to clear query plan cache
If current queryPlanCache size is 999, and there are two threads executing the put operation, then a third thread which enters the if condition will see the query plan cache size as 1001, and will never clear the cache, and hence the cache keeps growing indefinitely.

This is how existing (bug-code) is:

if (queryPlanCache.size() == Constants.QUERYPLAN_CACHE_SIZE) {
                logger.warn("Clearing query plan cache as it has reached the maximum size : {}", queryPlanCache.size());
                queryPlanCache.clear();
            }
            queryPlanCache.put(query.getQueryText(), partitionedQueryExecutionInfo);

Fix

Synchronizing this above if condition block will make sure the cache is read and updated atomically when we try to put new entries in the cache.

#Test
Added test and ran queryPlanCacheSizeHit locally on main to repro and on this branch to make sure that cache never exceeds the allowed size. Currently the test is disabled on the CI (will try to automate it).

Perf

-consistencyLevel Eventual
-concurrency 10
-numberOfOperations 1000000
-operation QuerySingle
-connectionMode Direct
-documentDataFieldSize 1
-documentDataFieldCount 1
-numberOfPreCreatedDocuments 1000

main ->
mean rate = 5770 calls/second
P99 = 3.324 milliseconds
P99.9 = 7.374 milliseconds

fix ->
mean rate = 6059 calls/second
P99 = 3.026 milliseconds
P99.9 = 6.972 milliseconds

@azure-sdk

Copy link
Copy Markdown
Collaborator

API change check

API changes are not detected in this pull request.

@FabianMeiswinkel FabianMeiswinkel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand why this change would ever avoid an OutOfMemoryError - please elaborate in the PR description or code comments to make it easier for others to understand this later.

@FabianMeiswinkel FabianMeiswinkel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants