[BACKPORT 2024.1][#22262] YSQL: Fix more memory leaks in catalog cach…

…e 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: a9b9a2b / 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/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 */

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

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 {