From e8cfc8d8e0f597965f4c3dcf4f0b2b4f04d0672d Mon Sep 17 00:00:00 2001 From: Minghui Yang Date: Mon, 13 May 2024 22:00:33 +0000 Subject: [PATCH] [BACKPORT 2024.1][#22262] YSQL: Fix more memory leaks in catalog cache refresh Summary: This diff fixes the remaining memory leaks in `CacheMemoryContext` that were identified to allow a stable cache memory after each catalog cache refresh. Two more identified memory leaks: * `YbCompleteAttrProcessingImpl`: `relation->rd_att->constr` needs to be freed. * `YbPreloadPgInheritsCache`: tighter scope of `MemoryContextSwitchTo(CacheMemoryContext)` For the first leak, we do not cleanup all the relations from rel cache on a catalog cache refresh. Some are considered `YbIsNonAlterableRelation` and will remain. But the overall processing flow of `YbCompleteAttrProcessingImpl` processes them anyway and will set their `relation->rd_att->constr` to the newly allocated `constr`. For the second leak in `YbPreloadPgInheritsCache`, PG code already appropriately allocates/copies into hcxt (CacheMemoryContext in this case) whenever required. If we still allocate any temporary memory into `CacheMemoryContext`, then they are not freed after being copied into `CacheMemoryContext`. Related code are: ``` static HTAB *YbPgInheritsCache; ``` and ``` void YbInitPgInheritsCache() { Assert(YbPgInheritsCache == NULL); HASHCTL ctl = {0}; ctl.keysize = sizeof(Oid); ctl.entrysize = sizeof(YbPgInheritsCacheEntryData); ctl.hcxt = CacheMemoryContext; ``` Note that `ctl.hcxt = CacheMemoryContext` ensures the copying into `CacheMemoryContext`. By moving `MemoryContextSwitchTo(CacheMemoryContext)` to the minimum scope where it is truly required to ensure the right memory context, those temporary memory will be allocated from transaction memory that will be auto-freed after the transaction (created just for doing cache refresh) is over. With these two leaks fixed, I am able to add a new unit test that checks that the cache memory size stay the same after cache refresh. Jira: DB-11181 Original commit: a9b9a2bd59d0435e7fdf62ec8c005fc99c343ffa / D35017 Test Plan: ./yb_build.sh release --cxx-test pg_libpq-test --gtest_filter PgLibPqTest.CatalogCacheMemoryLeak -n 20 Reviewers: fizaa, kfranz Reviewed By: kfranz Subscribers: yql Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D35199 --- .../src/backend/utils/cache/relcache.c | 2 ++ .../backend/utils/cache/yb_inheritscache.c | 5 ++-- src/yb/yql/pgwrapper/pg_libpq-test.cc | 24 +++++++++++++++++++ 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/postgres/src/backend/utils/cache/relcache.c b/src/postgres/src/backend/utils/cache/relcache.c index 781e8f9ea3b3..adfefa12f529 100644 --- a/src/postgres/src/backend/utils/cache/relcache.c +++ b/src/postgres/src/backend/utils/cache/relcache.c @@ -1890,6 +1890,8 @@ YbCompleteAttrProcessingImpl(const YbAttrProcessorState *state) /* Set up constraint/default info */ if (constr->has_not_null || ndef > 0 || attrmiss || relation->rd_rel->relchecks) { + if (relation->rd_att->constr) + pfree(relation->rd_att->constr); relation->rd_att->constr = constr; if (ndef > 0) /* DEFAULTs */ diff --git a/src/postgres/src/backend/utils/cache/yb_inheritscache.c b/src/postgres/src/backend/utils/cache/yb_inheritscache.c index 5494623a393f..ba663e0f3cf1 100644 --- a/src/postgres/src/backend/utils/cache/yb_inheritscache.c +++ b/src/postgres/src/backend/utils/cache/yb_inheritscache.c @@ -226,7 +226,6 @@ void YbPreloadPgInheritsCache() { Assert(YbPgInheritsCache); - MemoryContext oldcxt = MemoryContextSwitchTo(CacheMemoryContext); Relation relation = heap_open(InheritsRelationId, AccessShareLock); HeapTuple inheritsTuple; @@ -253,13 +252,13 @@ YbPreloadPgInheritsCache() entry->refcount = 1; entry->parentOid = parentOid; } - + MemoryContext oldcxt = MemoryContextSwitchTo(CacheMemoryContext); HeapTuple copy_inheritsTuple = heap_copytuple(inheritsTuple); entry->childTuples = lappend(entry->childTuples, copy_inheritsTuple); + MemoryContextSwitchTo(oldcxt); } systable_endscan(scan); heap_close(relation, AccessShareLock); - MemoryContextSwitchTo(oldcxt); } YbPgInheritsCacheEntry diff --git a/src/yb/yql/pgwrapper/pg_libpq-test.cc b/src/yb/yql/pgwrapper/pg_libpq-test.cc index a60e97aeae15..7979f26b8912 100644 --- a/src/yb/yql/pgwrapper/pg_libpq-test.cc +++ b/src/yb/yql/pgwrapper/pg_libpq-test.cc @@ -3881,6 +3881,30 @@ TEST_F(PgLibPqTest, TempTableMultiNodeNamespaceConflict) { ASSERT_EQ(rows, (decltype(rows){4, 5, 6})); } +TEST_F(PgLibPqTest, CatalogCacheMemoryLeak) { + auto conn1 = ASSERT_RESULT(Connect()); + auto conn2 = ASSERT_RESULT(Connect()); + auto query = "SELECT total_bytes, used_bytes FROM " + "pg_get_backend_memory_contexts() " + "WHERE name = 'CacheMemoryContext'"s; + string stable_result; + for (int i = 0; i < 20; i++) { + BumpCatalogVersion(1, &conn1); + // Wait for heartbeat to propagate the new catalog version to trigger + // catalog cache refresh on conn2. + SleepFor(2s); + auto result = ASSERT_RESULT(conn2.FetchAllAsString(query)); + LOG(INFO) << "result: " << result; + if (stable_result.empty()) { + stable_result = result; + } else { + // If each catalog cache refresh had a memory leak in cache memory, + // then this assertion would fail. + ASSERT_EQ(result, stable_result); + } + } +} + class PgBackendsSessionExpireTest : public LibPqTestBase { public: void UpdateMiniClusterOptions(ExternalMiniClusterOptions* options) override {