[SERVER-82295] Classic and SBE plan cache API definition and naming clarity Created: 18/Oct/23  Updated: 27/Oct/23

Status: In Code Review
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major - P3
Reporter: Kevin Cherkauer Assignee: Kevin Cherkauer
Resolution: Unresolved Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Participants:

 Description   

Improve the ease of working in the plan cache code, especially by creating a single, central API definition for each of the Classic and SBE plan caches; renaming classes, methods, functions, and data types to have consistent, unique, and non-generic names; and consolidating code into fewer places.



 Comments   
Comment by Kevin Cherkauer [ 27/Oct/23 ]

PR 16378 is ready for review and is intended to be delivered as production code.

The purpose of this PR is to make plan cache code easier to work with through several code hygiene practices. Most of the diff is from moving existing code around and renaming a number of types and methods, not fundamentally changing existing code or writing new code.

Friction points this PR addresses:

  1. There was no central API definition for the plan cache.
  2. Many actually different types, methods, and functions in different classes (usually Classic vs SBE) had the same names as each other.
  3. It was difficult to distinguish shared, Classic-specific, and SBE-specific plan cache code, and this code was often mixed together in a single file or even a single method.
  4. Some templatization existed that did not have benefits because the templatized types were not polymorphic with each other, so either the template just dispatched to separate methods or, in one case, to a shared method that mixed Classic-specific and SBE-specific code together and decided which blocks of code to execute inside that method based on checking whether the types that actually came in at runtime were Classic or SBE.
  5. Code was scattered around in different files each with their own custom namespace, making it difficult to find all the code that together constituted the plan cache implementation.
  6. Many functions and methods had very short, generic names (e.g. set(), get()) that made it impossible to easily grep to find all the uses of them because they were overwhelmed by thousands of extraneous hits.

There was a barrier to fully consolidating the APIs because some pieces (classic_plan_cache.cpp, sbe_plan_cache.cpp, and their shared parent plan_cache.h) are in a separate, small build target that other components depend on, while the APIs for adding things to the plan cache exist in and depend on code from in a different, very large build target that other components probably don't want to depend on, and attempting to depend on it would probably create dependency cycles anyway. Therefore I was not able to move all the relevant APIs from the small library to the large one or vice versa, but the large library depends on the small one, so I created APIs in the large library that, in some cases, wrap APIs in the small library and made the APIs in the small library private. (In several cases making the small library APIs private required writing public methodNameTest() wrappers for them that are clearly commented as being for unit test use only, while the new API classes are made friend classes so they can call the private APIs. This is to discourage more APIs from being added in the small library that would again break the goal of having a single central official API definition for each plan cache. Production code should not call any "xyzTest()" methods.)

The crux of this PR is the creation of central API definitions for each plan cache in one *.h file each:

  1. Classic - classic_plan_cache_api.h
  2. SBE - sbe_plan_cache_api.h

These consolidate APIs for

  • Cache key creation
  • Cache retrieval
  • Cache updating
  • In SBE's case, also parameter binding (which does not exist in Classic)

There was not sufficient time during Skunkworks to try to consolidate auto-parameterization APIs, which is the last main piece of the SBE plan cache.

Generated at Thu Feb 08 06:48:48 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.