[yugabyte#7105] Explicitly clear the global PgMemctx map in YBCDestro…

…yPgGate

Summary:
Clear the postgres_process_memctxs global PgMemctx object map in the YBCDestroyPgGate function, right after destroying the PgApiImpl object. If we don't do this, then we are leaving the destruction order of objects whose last refcount is held by the PgMemctx objects in the global map up to chance. Some of them contain Trace objects, which hold pointers to arenas allocated from a global thread-safe pool of arenas, but that thread-safe pool could have already been destroyed by the time the global PgMemctx map is being destructed. Global order of destructors in C++ is not well defined, and we should not rely on it.

Example crash stack trace: https://gist.githubusercontent.com/mbautin/37df1b73963b9e9cb92294be69560512/raw

Also see the stack traces in yugabyte#7105

It would be good to move the global PgMemctx map into the PgApiImpl object in the future. That is tracked in yugabyte#7216.

Test Plan: Jenkins: enable clang 11

Reviewers: neil, mihnea, dmitry

Reviewed By: dmitry

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D10589

src/yb/yql/pggate/pg_memctx.cc

@@ -91,5 +91,9 @@ void PgMemctx::GetCache(size_t hash_id, PgTableDesc **handle) {
   }
 }
 
+void ClearGlobalPgMemctxMap() {
+  postgres_process_memctxs.clear();
+}
+
 }  // namespace pggate
 }  // namespace yb

src/yb/yql/pggate/pg_memctx.h

@@ -98,6 +98,8 @@ class PgMemctx {
   DISALLOW_COPY_AND_ASSIGN(PgMemctx);
 };
 
+void ClearGlobalPgMemctxMap();
+
 }  // namespace pggate
 }  // namespace yb
 

src/yb/yql/pggate/test/pggate_test.cc

@@ -32,21 +32,30 @@ extern "C" void FetchUniqueConstraintName(PgOid relation_id, char* dest, size_t
   CHECK(false) << "Not implemented";
 }
 
-} // namespace
+YBCPgMemctx global_test_memctx = nullptr;
+
+YBCPgMemctx GetCurrentTestYbMemctx() {
+  if (!global_test_memctx) {
+    global_test_memctx = YBCPgCreateMemctx();
+  }
+  return global_test_memctx;
+}
 
-YBCPgMemctx test_memctx = nullptr;
-static YBCPgMemctx TestGetCurrentYbMemctx() {
-  if (!test_memctx) {
-    test_memctx = YBCPgCreateMemctx();
+void ClearCurrentTestYbMemctx() {
+  if (global_test_memctx != nullptr) {
+    CHECK_YBC_STATUS(YBCPgDestroyMemctx(global_test_memctx));
+
+    // We assume the memory context has actually already been deleted.
+    global_test_memctx = nullptr;
   }
-  return test_memctx;
 }
 
+} // namespace
+
 PggateTest::PggateTest() {
 }
 
 PggateTest::~PggateTest() {
-  CHECK_YBC_STATUS(YBCPgDestroyMemctx(test_memctx));
 }
 
 //--------------------------------------------------------------------------------------------------
@@ -82,6 +91,9 @@ void PggateTest::SetUp() {
 }
 
 void PggateTest::TearDown() {
+  // It is important to destroy the memory context before destroying PgGate.
+  ClearCurrentTestYbMemctx();
+
   // Destroy the client before shutting down servers.
   YBCDestroyPgGate();
 
@@ -105,7 +117,7 @@ Status PggateTest::Init(const char *test_name, int num_tablet_servers) {
   YBCTestGetTypeTable(&type_table, &count);
   YBCPgCallbacks callbacks;
   callbacks.FetchUniqueConstraintName = &FetchUniqueConstraintName;
-  callbacks.GetCurrentYbMemctx = &TestGetCurrentYbMemctx;
+  callbacks.GetCurrentYbMemctx = &GetCurrentTestYbMemctx;
   YBCInitPgGate(type_table, count, callbacks);
 
   // Don't try to connect to tserver shared memory in pggate tests.

src/yb/yql/pggate/test/pggate_test.h

@@ -38,7 +38,8 @@ namespace pggate {
 //--------------------------------------------------------------------------------------------------
 // Test base class.
 //--------------------------------------------------------------------------------------------------
-#define CHECK_YBC_STATUS(s) CheckYBCStatus((s), __FILE__, __LINE__)
+#define CHECK_YBC_STATUS(s) \
+    ::yb::pggate::PggateTest::CheckYBCStatus((s), __FILE__, __LINE__)
 
 class PggateTest : public YBTest {
  public:
@@ -51,7 +52,7 @@ class PggateTest : public YBTest {
   virtual ~PggateTest();
 
   //------------------------------------------------------------------------------------------------
-  void CheckYBCStatus(YBCStatus status, const char* file_name, int line_number);
+  static void CheckYBCStatus(YBCStatus status, const char* file_name, int line_number);
 
   //------------------------------------------------------------------------------------------------
   // Test start and cleanup functions.

src/yb/yql/pggate/ybc_pggate.cc

@@ -24,6 +24,7 @@
 #include "yb/yql/pggate/pggate_thread_local_vars.h"
 #include "yb/yql/pggate/pg_txn_manager.h"
 #include "yb/yql/pggate/pggate_flags.h"
+#include "yb/yql/pggate/pg_memctx.h"
 
 DECLARE_bool(client_suppress_created_logs);
 
@@ -93,6 +94,7 @@ void YBCDestroyPgGate() {
     pggate::PgApiImpl* local_pgapi = pgapi;
     pgapi = nullptr; // YBCPgIsYugaByteEnabled() must return false from now on.
     delete local_pgapi;
+    ClearGlobalPgMemctxMap();
     VLOG(1) << __PRETTY_FUNCTION__ << " finished";
   }
 }