github-actions[bot] commented on code in PR #69998:
URL: https://github.com/apache/doris/pull/69998#discussion_r4447976170
##########
be/src/service/point_query_executor.cpp:
##########
@@ -231,22 +284,28 @@ RowCache* RowCache::instance() {
return ExecEnv::GetInstance()->get_row_cache();
}
-bool RowCache::lookup(const RowCacheKey& key, CacheHandle* handle) {
+bool RowCache::lookup(const RowCacheKey& key, const TabletSchemaSPtr& schema,
CacheHandle* handle) {
const std::string& encoded_key = key.encode();
auto* lru_handle = LRUCachePolicy::lookup(encoded_key);
if (!lru_handle) {
// cache miss
return false;
}
+ auto* cached_value = static_cast<RowCacheValue*>(value(lru_handle));
+ if (cached_value->schema != schema) {
+ release(lru_handle);
+ return false;
+ }
*handle = CacheHandle(this, lru_handle);
return true;
}
-void RowCache::insert(const RowCacheKey& key, const Slice& value) {
+void RowCache::insert(const RowCacheKey& key, const TabletSchemaSPtr& schema,
const Slice& value) {
char* cache_value = static_cast<char*>(malloc(value.size));
memcpy(cache_value, value.data, value.size);
auto* row_cache_value = new RowCacheValue;
row_cache_value->cache_value = cache_value;
+ row_cache_value->schema = schema;
Review Comment:
[P2] Account for schema snapshots retained by row-cache entries.
`RowCacheValue` now owns `TabletSchemaSPtr` for every JSONB payload, but this
insertion still charges/tracks only `value.size` (the payload bytes). After a
light-schema change, warm entries can keep old `TabletSchema` objects alive
after `TabletSchemaCache` releases its `mem_size()` charge, so the row-cache
usage and memory tracker under-report retained metadata and repeated schema
changes can exceed the configured cache budget. Store a stable
generation/fingerprint without owning the full schema, or explicitly charge and
release the retained schema memory.
##########
be/src/service/point_query_executor.cpp:
##########
@@ -295,9 +356,11 @@ Status PointQueryExecutor::init(const
PTabletKeyLookupRequest* request,
__int128_t uuid =
static_cast<__int128_t>(request->uuid().uuid_high()) << 64 |
request->uuid().uuid_low();
SCOPED_ATTACH_TASK(ExecEnv::GetInstance()->point_query_executor_mem_tracker());
- auto cache_handle = LookupConnectionCache::instance()->get(uuid);
_binary_row_format = request->is_binary_row();
_tablet = DORIS_TRY(ExecEnv::get_tablet(request->tablet_id()));
+ _tablet_schema = _tablet->tablet_schema();
Review Comment:
[P1] Revalidate the schema generation before decoding the rowset.
`_tablet_schema` is captured here and used to select/cache the `Reusable`, but
`_lookup_row_key()` later snapshots rowsets under the tablet metadata lock and
`BaseTablet::lookup_row_data()` uses the producing rowset's schema. If a schema
refresh publishes a new rowset between those steps, this request can decode
that payload with the old SerDes/UID maps and return defaults or wrong complex
values. Take a schema/rowset snapshot under one lock or recheck the generation
before JSONB/column-store decoding and rebuild/reject on mismatch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
