From 867df0689fd1ad177d1f91a9d3e6ce62c2cb079c Mon Sep 17 00:00:00 2001 From: hnwyllmm Date: Wed, 19 Aug 2026 10:49:45 +0800 Subject: [PATCH 1/3] fix plan cache accounting lifetimes --- src/sql/plan_cache/ob_plan_cache.cpp | 17 +++++++++--- src/sql/plan_cache/ob_ps_cache_callback.h | 9 ++++--- .../test_memory_limit_accounting.cpp | 26 +++++++++++++++++++ 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/src/sql/plan_cache/ob_plan_cache.cpp b/src/sql/plan_cache/ob_plan_cache.cpp index 0f2d889cf..03651e4e4 100644 --- a/src/sql/plan_cache/ob_plan_cache.cpp +++ b/src/sql/plan_cache/ob_plan_cache.cpp @@ -1059,6 +1059,13 @@ int ObPlanCache::add_cache_obj(ObILibCacheCtx &ctx, cache_node->dec_ref_count(); //cache node dec ref in alloc } } else { + // Keep the node write lock while inspecting the newly cached + // object. Once unlocked, concurrent eviction may remove the + // object's last reference before the accounting reads its arena. + if (cache_obj->added_lc()) { + account_cache_object(*cache_obj); + refresh_cache_node(*cache_node); + } cache_node->unlock(); cache_node->dec_ref_count(); //cache node dec ref in block } @@ -1086,14 +1093,16 @@ int ObPlanCache::add_cache_obj(ObILibCacheCtx &ctx, ctx.need_destroy_node_ = true; } } + if (OB_SUCC(ret) && cache_obj->added_lc()) { + // The write lock pins both the node and its object list against + // concurrent eviction while their allocator totals are sampled. + account_cache_object(*cache_obj); + refresh_cache_node(*cache_node); + } // release wlock whatever cache_node->unlock(); cache_node->dec_ref_count(); } - if (OB_SUCC(ret) && OB_NOT_NULL(cache_node) && cache_obj->added_lc()) { - account_cache_object(*cache_obj); - refresh_cache_node(*cache_node); - } return ret; } diff --git a/src/sql/plan_cache/ob_ps_cache_callback.h b/src/sql/plan_cache/ob_ps_cache_callback.h index c1ce20428..8ff6842b1 100644 --- a/src/sql/plan_cache/ob_ps_cache_callback.h +++ b/src/sql/plan_cache/ob_ps_cache_callback.h @@ -64,10 +64,11 @@ class ObGetClosedStmtIdOp PsClosedStmt closed_stmt; closed_stmt.stmt_id_ = entry.first; closed_stmt.closed_timestamp_ = entry.second->get_last_closed_timestamp(); - closed_stmt.reclaimable_size_ = entry.second->get_accounted_size() + map_entry_charge_; - if (OB_NOT_NULL(entry.second->get_ps_item())) { - closed_stmt.reclaimable_size_ += entry.second->get_ps_item()->get_accounted_size(); - } + // A schema-expired ObPsStmtInfo can outlive its ObPsStmtItem: the + // prepare path erases the item first, while the timer removes the info + // later. ps_item_ is therefore not safe to dereference here. Use the + // item-and-info size snapshot copied into the info when it was built. + closed_stmt.reclaimable_size_ = entry.second->get_item_and_info_size() + map_entry_charge_; if (entry.second->is_expired()) { // for expired ps info, only evicted once; // use cas, because auto cache evict and flush ps cache may concurrent processing diff --git a/unittest/sql/plan_cache/test_memory_limit_accounting.cpp b/unittest/sql/plan_cache/test_memory_limit_accounting.cpp index 9c0b6c62f..0cf519cef 100644 --- a/unittest/sql/plan_cache/test_memory_limit_accounting.cpp +++ b/unittest/sql/plan_cache/test_memory_limit_accounting.cpp @@ -22,6 +22,7 @@ #include "sql/plan_cache/ob_i_lib_cache_object.h" #include "sql/plan_cache/ob_plan_cache.h" #include "sql/plan_cache/ob_prepare_stmt_struct.h" +#include "sql/plan_cache/ob_ps_cache_callback.h" #define private public #include "sql/plan_cache/ob_ps_cache.h" @@ -151,6 +152,31 @@ TEST_F(TestMemoryLimitAccounting, ps_entry_and_live_object_have_separate_charges EXPECT_EQ(0, ps_cache.get_managed_used()); } +TEST_F(TestMemoryLimitAccounting, closed_ps_entry_uses_size_snapshot_after_item_is_removed) +{ + common::ObArenaAllocator allocator; + ObPsStmtInfo info(&allocator, &allocator); + constexpr int64_t item_and_info_size = 777; + constexpr int64_t map_entry_charge = 64; + info.set_item_and_info_size(item_and_info_size); + // The prepare path may remove the item before the timer scans this info. + info.set_ps_item(nullptr); + + common::hash::HashMapPair entry; + entry.first = 42; + entry.second = &info; + common::ObSEArray expired; + common::ObSEArray closed; + ObGetClosedStmtIdOp op(&expired, &closed, map_entry_charge); + + EXPECT_EQ(common::OB_SUCCESS, op(entry)); + ASSERT_EQ(common::OB_SUCCESS, op.get_callback_ret()); + ASSERT_EQ(1, closed.count()); + EXPECT_EQ(item_and_info_size + map_entry_charge, + closed.at(0).reclaimable_size_); + EXPECT_EQ(item_and_info_size + map_entry_charge, op.get_used_size()); +} + TEST_F(TestMemoryLimitAccounting, ps_failed_insert_rolls_back_object_and_entry) { common::ObArenaAllocator allocator; From 7eebd75c45237a3010208b2905bfae4a7b1b4f4c Mon Sep 17 00:00:00 2001 From: hnwyllmm Date: Wed, 19 Aug 2026 11:06:10 +0800 Subject: [PATCH 2/3] fix skip index column buffer size --- src/observer/virtual_table/ob_table_columns.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/observer/virtual_table/ob_table_columns.cpp b/src/observer/virtual_table/ob_table_columns.cpp index 88c397e56..7ebad6ad8 100644 --- a/src/observer/virtual_table/ob_table_columns.cpp +++ b/src/observer/virtual_table/ob_table_columns.cpp @@ -600,7 +600,7 @@ int ObTableColumns::fill_row_cells(const ObTableSchema &table_schema, } if (OB_SUCC(ret) && column_schema.get_skip_index_attr().has_sum()) { - if (first_skip_idx_attr_printed && OB_FAIL(databuff_printf(buf, buf_len, pos, ", "))) { + if (first_skip_idx_attr_printed && OB_FAIL(databuff_printf(buf, extra_print_buf_size, pos, ", "))) { LOG_WARN("fail to print buf", K(ret)); } else if (OB_FAIL(databuff_printf(buf, extra_print_buf_size, pos, "SUM"))) { } else { From 4c2b1def9569e13f9e44eb64800d0a96ad032d93 Mon Sep 17 00:00:00 2001 From: hnwyllmm Date: Wed, 19 Aug 2026 15:09:13 +0800 Subject: [PATCH 3/3] remove obsolete memory accounting unit test --- .../test_memory_limit_accounting.cpp | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/unittest/sql/plan_cache/test_memory_limit_accounting.cpp b/unittest/sql/plan_cache/test_memory_limit_accounting.cpp index 0cf519cef..9c0b6c62f 100644 --- a/unittest/sql/plan_cache/test_memory_limit_accounting.cpp +++ b/unittest/sql/plan_cache/test_memory_limit_accounting.cpp @@ -22,7 +22,6 @@ #include "sql/plan_cache/ob_i_lib_cache_object.h" #include "sql/plan_cache/ob_plan_cache.h" #include "sql/plan_cache/ob_prepare_stmt_struct.h" -#include "sql/plan_cache/ob_ps_cache_callback.h" #define private public #include "sql/plan_cache/ob_ps_cache.h" @@ -152,31 +151,6 @@ TEST_F(TestMemoryLimitAccounting, ps_entry_and_live_object_have_separate_charges EXPECT_EQ(0, ps_cache.get_managed_used()); } -TEST_F(TestMemoryLimitAccounting, closed_ps_entry_uses_size_snapshot_after_item_is_removed) -{ - common::ObArenaAllocator allocator; - ObPsStmtInfo info(&allocator, &allocator); - constexpr int64_t item_and_info_size = 777; - constexpr int64_t map_entry_charge = 64; - info.set_item_and_info_size(item_and_info_size); - // The prepare path may remove the item before the timer scans this info. - info.set_ps_item(nullptr); - - common::hash::HashMapPair entry; - entry.first = 42; - entry.second = &info; - common::ObSEArray expired; - common::ObSEArray closed; - ObGetClosedStmtIdOp op(&expired, &closed, map_entry_charge); - - EXPECT_EQ(common::OB_SUCCESS, op(entry)); - ASSERT_EQ(common::OB_SUCCESS, op.get_callback_ret()); - ASSERT_EQ(1, closed.count()); - EXPECT_EQ(item_and_info_size + map_entry_charge, - closed.at(0).reclaimable_size_); - EXPECT_EQ(item_and_info_size + map_entry_charge, op.get_used_size()); -} - TEST_F(TestMemoryLimitAccounting, ps_failed_insert_rolls_back_object_and_entry) { common::ObArenaAllocator allocator;