[yugabyte#12490, yugabyte#7216, yugabyte#12692] YSQL: Avoid undefined…

… behavior on postgres shutdown

Summary:
The diff consists of 3 parts.

**Part #1:** Multiple call of the `PerformFuture::Get` method
In case of session close when temp tables exists the following stack is possible

```
std::__1::future<yb::pggate::PerformResult>::get()
yb::pggate::PerformFuture::Get()                   // Second call of the PerformFuture::Get on same object
yb::pggate::PgDocResponse::Get()
yb::pggate::PgDocOp::~PgDocOp()
yb::pggate::PgDocReadOp::~PgDocReadOp()
...
yb::pggate::PgDml::~PgDml()
yb::pggate::PgDmlRead::~PgDmlRead()
yb::pggate::PgSelect::~PgSelect()
...
yb::pggate::PgMemctx::Clear()
yb::pggate::PgMemctx::~PgMemctx()
...
yb::pggate::ClearGlobalPgMemctxMap()
YBCDestroyPgGate
YBOnPostgresBackendShutdown
quickdie                                           // Caused by interruption by the SIGQUIT signal
...
std::__1::future<yb::pggate::PerformResult>::get()
yb::pggate::PerformFuture::Get()                   // First call of the PerformFuture::Get
yb::pggate::PgDocResponse::Get()
yb::pggate::PgDocOp::GetResult(std::__1::list<yb::pggate::PgDocResult, std::__1::allocator<yb::pggate::PgDocResult> >*)
yb::pggate::PgDml::FetchDataFromServer()
yb::pggate::PgDml::Fetch(int, unsigned long*, bool*,
yb::pggate::PgApiImpl::DmlFetch(yb::pggate::PgStatement*, int, unsigned long*,
YBCPgDmlFetch
ybcFetchNextHeapTuple
ybc_getnext_heaptuple
ybc_systable_getnext
systable_getnext
findDependentObjects
performDeletion
RemoveTempRelations
RemoveTempRelationsCallback
shmem_exit
proc_exit_prepare
proc_exit
PostgresMain
BackendRun
BackendStartup
ServerLoop
PostmasterMain
PostgresServerProcessMain
main
```

In this stack the `PerformFuture::Get` method is called on same object twice. But the `PerformFuture` object is designed to call `Get` method only once. After the first call the `PerformFuture::Valid` method should return `false` and `Get` method should not be called. But in the above stack the second call of the `PerformFuture::Get`method is done before the first one is returned (due to interruption by the `SIGQUIT` signal).

To handle this situation `session_` field is set to `null` at the very beginning of the `PerformFuture::Get`. In case `session_` is `null` the `PerformFuture::Valid` returns `false`.

**Part #2:** Access deleted `PgApiImpl` object
The `YBCDestroyPgGate` function destroys the `PgApiImpl` object and then calls the `ClearGlobalPgMemctxMap` function. But this function may call the code which access the `PgApiImpl` object fields. Here is the stack trace

```
std::__1::unique_ptr<yb::pggate::PgClient::Impl, std::__1::default_delete<yb::pggate::PgClient::Impl> >::operator->() const
yb::pggate::PgClient::FinishTransaction(yb::StronglyTypedBool<yb::pggate::Commit_Tag>, yb::StronglyTypedBool<yb::pggate::DdlMode_Tag>)
   yb::pggate::PgTxnManager::ExitSeparateDdlTxnMode(yb::StronglyTypedBool<yb::pggate::Commit_Tag>)
yb::pggate::PgTxnManager::FinishTransaction(yb::StronglyTypedBool<yb::pggate::Commit_Tag>)
yb::pggate::PgTxnManager::AbortTransaction()
yb::pggate::PgTxnManager::~PgTxnManager()
...
yb::pggate::PgSession::~PgSession()
...
yb::pggate::PgStatement::~PgStatement()
yb::pggate::PgDml::~PgDml()
yb::pggate::PgDmlRead::~PgDmlRead()
yb::pggate::PgSelect::~PgSelect()
...
yb::pggate::PgMemctx::Clear()
yb::pggate::PgMemctx::~PgMemctx()
...
yb::pggate::ClearGlobalPgMemctxMap()
...
```

Solution is to move PgMemctx map into the `PgApiImpl` object (this also fixes yugabyte#7216)

**Part #3:** Access shut down `PgClient`object

The `~PgApiImpl()` explicitly shut downs `pg_client_` object but the `pg_session_` is still alive. And when it will be destroyed the pg_client_ object may be used.

The stack is

```
yb::pggate::PgClient::Impl::FinishTransaction(yb::StronglyTypedBool<yb::pggate::Commit_Tag>, yb::StronglyTypedBool<yb::pggate::DdlMode_Tag>)
yb::pggate::PgClient::FinishTransaction(yb::StronglyTypedBool<yb::pggate::Commit_Tag>, yb::StronglyTypedBool<yb::pggate::DdlMode_Tag>)
yb::pggate::PgTxnManager::ExitSeparateDdlTxnMode(yb::StronglyTypedBool<yb::pggate::Commit_Tag>)
yb::pggate::PgTxnManager::FinishTransaction(yb::StronglyTypedBool<yb::pggate::Commit_Tag>)
yb::pggate::PgTxnManager::AbortTransaction()
yb::pggate::PgTxnManager::~PgTxnManager()
...
yb::pggate::PgSession::~PgSession()
...
yb::pggate::PgApiImpl::~PgApiImpl()
...
YBCDestroyPgGate
YBOnPostgresBackendShutdown
quickdie
...
```

Solution is to explicitly destroy `pg_session_` before shutting down of `pg_client_`

Test Plan:
Jenkins

The following tests had crashed without the fix

```
./yb_build.sh asan --gtest_filter PgFKeyTest.AddFKCorrectnessOnTempTables
./yb_build.sh asan --gtest_filter PgCatalogPerfTest.CacheRefreshRPCCountWithPartitionTables
./yb_build.sh asan --gtest_filter PgMiniTest.BigInsertWithRestart
./yb_build.sh asan --gtest_filter AlterTableWithConcurrentTxnTest.TServerLeaderChange
```

Reviewers: dsrinivasan, jason, alex

Reviewed By: alex

Subscribers: mbautin, bogdan, yql

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

src/yb/yql/pggate/pg_memctx.cc

@@ -15,56 +15,16 @@
 
 #include "yb/yql/pggate/pg_memctx.h"
 
-#include "yb/util/status.h"
-
 #include "yb/yql/pggate/pg_tabledesc.h"
+#include "yb/yql/pggate/pggate.h"
 
 namespace yb {
 namespace pggate {
 
-PgMemctx::PgMemctx() {
-}
-
 PgMemctx::~PgMemctx() {
   Clear();
 }
 
-namespace {
-  // Table of memory contexts.
-  // - Although defined in Yugabyte, this table is owned and managed by Postgres process.
-  //   Other processes cannot control "pggate::PgMemctx" to avoid memory violations.
-  // - Table "postgres_process_memctxs" is to help releasing the references to PgStatement when
-  //   Postgres Process (a C program) is exiting.
-  // - Transaction layer and Postgres BOTH hold references to PgStatement objects, and those
-  //   PgGate objects wouldn't be destroyed unless both layers release their references or both
-  //   layers are terminated.
-  std::unordered_map<PgMemctx *, PgMemctx::SharedPtr> postgres_process_memctxs;
-} // namespace
-
-PgMemctx *PgMemctx::Create() {
-  auto memctx = std::make_shared<PgMemctx>();
-  postgres_process_memctxs[memctx.get()] = memctx;
-  return memctx.get();
-}
-
-Status PgMemctx::Destroy(PgMemctx *handle) {
-  if (handle) {
-    SCHECK(postgres_process_memctxs.find(handle) != postgres_process_memctxs.end(),
-           InternalError, "Invalid memory context handle");
-    postgres_process_memctxs.erase(handle);
-  }
-  return Status::OK();
-}
-
-Status PgMemctx::Reset(PgMemctx *handle) {
-  if (handle) {
-    SCHECK(postgres_process_memctxs.find(handle) != postgres_process_memctxs.end(),
-           InternalError, "Invalid memory context handle");
-    handle->Clear();
-  }
-  return Status::OK();
-}
-
 void PgMemctx::Register(Registrable *obj) {
   registered_objects_.push_back(*obj);
   obj->memctx_ = this;
@@ -95,9 +55,5 @@ 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

@@ -15,15 +15,12 @@
 #ifndef YB_YQL_PGGATE_PG_MEMCTX_H_
 #define YB_YQL_PGGATE_PG_MEMCTX_H_
 
-#include <vector>
 #include <unordered_map>
 
 #include <boost/intrusive/list.hpp>
 
 #include "yb/yql/pggate/pg_gate_fwd.h"
 
-#include "yb/util/status_fwd.h"
-
 namespace yb {
 namespace pggate {
 
@@ -39,8 +36,6 @@ namespace pggate {
 //   associated YB Memctx. The object is automatically destroyed when YB Memctx is destroyed.
 class PgMemctx {
  public:
-  typedef std::shared_ptr<PgMemctx> SharedPtr;
-
   class Registrable : public boost::intrusive::list_base_hook<> {
    public:
     virtual ~Registrable() = default;
@@ -49,9 +44,8 @@ class PgMemctx {
     friend class PgMemctx;
   };
 
-  // Constructor and destructor.
-  PgMemctx();
-  virtual ~PgMemctx();
+  PgMemctx() = default;
+  ~PgMemctx();
 
   // API: Create(), Destroy(), and Reset()
   // - Because Postgres process own YugaByte memory context, only Postgres processes should call
@@ -62,17 +56,6 @@ class PgMemctx {
   //   Reset() API uses a global variable for that purpose.  When Postgres processes exit, the
   //   global destructor will free all YugaByte memory contexts.
 
-  // Create yugabyte memory context that will be owned by Postgres process.
-  static PgMemctx *Create();
-
-  // Destroy yugabyte memory context that is owned by Postgres process.
-  static Status Destroy(PgMemctx *handle);
-
-  // Clear the content of yugabyte memory context that is owned by Postgres process.
-  // Postgres has Reset() option where it clears the allocated memory for the current context but
-  // keeps all the allocated memory for the child contexts.
-  static Status Reset(PgMemctx *handle);
-
   void Register(Registrable *obj);
   static void Destroy(Registrable *obj);
 
@@ -82,7 +65,6 @@ class PgMemctx {
   // Read the table descriptor from cache.
   void GetCache(size_t hash_id, PgTableDesc **handle);
 
- private:
   // NOTE:
   // - In Postgres, the objects in the outer context can references to the objects of the nested
   //   context but not vice versa, so it is safe to clear objects of outer context.
@@ -92,16 +74,15 @@ class PgMemctx {
   //   memctx, we can delay the PgStatement objects' destruction.
   void Clear();
 
-  // All talbe descriptors that are allocated with this memory context.
+ private:
+  // All table descriptors that are allocated with this memory context.
   std::unordered_map<size_t, PgTableDescPtr> tabledesc_map_;
 
   boost::intrusive::list<Registrable> registered_objects_;
 
   DISALLOW_COPY_AND_ASSIGN(PgMemctx);
 };
 
-void ClearGlobalPgMemctxMap();
-
 }  // namespace pggate
 }  // namespace yb
 

src/yb/yql/pggate/pg_perform_future.cc

@@ -37,9 +37,9 @@ bool PerformFuture::Ready() const {
 }
 
 Result<rpc::CallResponsePtr> PerformFuture::Get() {
+  PgSession* session = nullptr;
+  std::swap(session, session_);
   auto result = future_.get();
-  auto session = session_;
-  session_ = nullptr;
   session->TrySetCatalogReadPoint(result.catalog_read_time);
   RETURN_NOT_OK(session->PatchStatus(result.status, relations_));
   return result.response;

src/yb/yql/pggate/pggate.cc

@@ -325,6 +325,16 @@ using client::YBSession;
 
 //--------------------------------------------------------------------------------------------------
 
+size_t PgMemctxHasher::operator()(const std::unique_ptr<PgMemctx>& value) const {
+  return (*this)(value.get());
+}
+
+size_t PgMemctxHasher::operator()(PgMemctx* value) const {
+  return std::hash<PgMemctx*>()(value);
+}
+
+//--------------------------------------------------------------------------------------------------
+
 PgApiContext::MessengerHolder::MessengerHolder(
     std::unique_ptr<rpc::SecureContext> security_context_,
     std::unique_ptr<rpc::Messenger> messenger_)
@@ -384,6 +394,8 @@ PgApiImpl::PgApiImpl(
 }
 
 PgApiImpl::~PgApiImpl() {
+  mem_contexts_.clear();
+  pg_session_.reset();
   messenger_holder_.messenger->Shutdown();
   pg_txn_manager_.reset();
   pg_client_.Shutdown();
@@ -441,17 +453,23 @@ bool PgApiImpl::GetDisableTransparentCacheRefreshRetry() {
 
 PgMemctx *PgApiImpl::CreateMemctx() {
   // Postgres will create YB Memctx when it first use the Memctx to allocate YugaByte object.
-  return PgMemctx::Create();
+  return mem_contexts_.insert(std::make_unique<PgMemctx>()).first->get();
 }
 
 Status PgApiImpl::DestroyMemctx(PgMemctx *memctx) {
   // Postgres will destroy YB Memctx by releasing the pointer.
-  return PgMemctx::Destroy(memctx);
+  auto it = mem_contexts_.find(memctx);
+  SCHECK(it != mem_contexts_.end(), InternalError, "Invalid memory context handle");
+  mem_contexts_.erase(it);
+  return Status::OK();
 }
 
 Status PgApiImpl::ResetMemctx(PgMemctx *memctx) {
   // Postgres reset YB Memctx when clearing a context content without clearing its nested context.
-  return PgMemctx::Reset(memctx);
+  auto it = mem_contexts_.find(memctx);
+  SCHECK(it != mem_contexts_.end(), InternalError, "Invalid memory context handle");
+  (**it).Clear();
+  return Status::OK();
 }
 
 // TODO(neil) Use Arena in the future.

src/yb/yql/pggate/pggate.h

@@ -17,6 +17,7 @@
 #include <functional>
 #include <memory>
 #include <unordered_map>
+#include <unordered_set>
 
 #include "yb/client/async_initializer.h"
 #include "yb/client/client_fwd.h"
@@ -52,6 +53,35 @@ namespace pggate {
 class PgSysTablePrefetcher;
 class PgSession;
 
+struct PgMemctxComparator {
+  using is_transparent = void;
+
+  bool operator()(PgMemctx* l, PgMemctx* r) const {
+    return l == r;
+  }
+
+  template<class T1, class T2>
+  bool operator()(const T1& l, const T2& r) const {
+    return (*this)(GetPgMemctxPtr(l), GetPgMemctxPtr(r));
+  }
+
+ private:
+  static PgMemctx* GetPgMemctxPtr(PgMemctx* value) {
+    return value;
+  }
+
+  static PgMemctx* GetPgMemctxPtr(const std::unique_ptr<PgMemctx>& value) {
+    return value.get();
+  }
+};
+
+struct PgMemctxHasher {
+  using is_transparent = void;
+
+  size_t operator()(const std::unique_ptr<PgMemctx>& value) const;
+  size_t operator()(PgMemctx* value) const;
+};
+
 //--------------------------------------------------------------------------------------------------
 
 struct PgApiContext {
@@ -100,14 +130,10 @@ class PgApiImpl {
   // If database_name is empty, a session is created without connecting to any database.
   Status InitSession(const PgEnv *pg_env, const std::string& database_name);
 
-  // YB Memctx: Create, Destroy, and Reset must be "static" because a few contexts are created
-  //            before YugaByte environments including PgGate are created and initialized.
-  // Create YB Memctx. Each memctx will be associated with a Postgres's MemoryContext.
-  static PgMemctx *CreateMemctx();
-  // Destroy YB Memctx.
-  static Status DestroyMemctx(PgMemctx *memctx);
-  // Reset YB Memctx.
-  static Status ResetMemctx(PgMemctx *memctx);
+  PgMemctx *CreateMemctx();
+  Status DestroyMemctx(PgMemctx *memctx);
+  Status ResetMemctx(PgMemctx *memctx);
+
   // Cache statements in YB Memctx. When Memctx is destroyed, the statement is destructed.
   Status AddToCurrentPgMemctx(std::unique_ptr<PgStatement> stmt,
                                       PgStatement **handle);
@@ -597,6 +623,7 @@ class PgApiImpl {
 
   scoped_refptr<PgSession> pg_session_;
   std::unique_ptr<PgSysTablePrefetcher> pg_sys_table_prefetcher_;
+  std::unordered_set<std::unique_ptr<PgMemctx>, PgMemctxHasher, PgMemctxComparator> mem_contexts_;
 };
 
 }  // namespace pggate

src/yb/yql/pggate/ybc_pggate.cc

@@ -137,13 +137,13 @@ void YBCInitPgGate(const YBCPgTypeEntity *data_type_table, int count, PgCallback
 void YBCDestroyPgGate() {
   if (pgapi_shutdown_done.exchange(true)) {
     LOG(DFATAL) << __PRETTY_FUNCTION__ << " should only be called once";
-  } else {
-    pggate::PgApiImpl* local_pgapi = pgapi;
+    return;
+  }
+  {
+    std::unique_ptr<pggate::PgApiImpl> local_pgapi(pgapi);
     pgapi = nullptr; // YBCPgIsYugaByteEnabled() must return false from now on.
-    delete local_pgapi;
-    ClearGlobalPgMemctxMap();
-    VLOG(1) << __PRETTY_FUNCTION__ << " finished";
   }
+  VLOG(1) << __PRETTY_FUNCTION__ << " finished";
 }
 
 const YBCPgCallbacks *YBCGetPgCallbacks() {