Re: Something is broken about connection startup

I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I assume you're going to back-patch this, and the consequences of
>> failing to reset it before going idle could easily be vastly worse
>> than the problem you're trying to fix.  So I'd rather not make
>> assumptions like "the client will probably never sleep between Bind
>> and Execute".  I bet that's actually false, and I certainly wouldn't
>> want to bet the farm on it being true.

> No, I'm not at all proposing to assume that.  But I may be willing to
> assume that "we don't hold a CatalogSnapshot between Bind and Execute
> unless we're also holding a transaction snapshot".  I need to do a bit
> more research to see if that's actually true, though.

Turns out it's not true.

I still don't much like having the main loop in PostgresMain know about
this hack, so I experimented with putting the InvalidateCatalogSnapshot()
calls into places in postgres.c that were already dealing with transaction
commit/abort or snapshot management.  I ended up needing five such calls
(as in the attached patch), which is just about equally ugly.  So at this
point I'm inclined to hold my nose and stick a call into step (1) of the
main loop instead.

Also, wherever we end up putting those calls, is it worth providing a
variant invalidation function that only kills the catalog snapshot when
it's the only one outstanding?  (If it isn't, the transaction snapshot
should be older, so there's no chance of advancing our xmin by killing
it.)  In principle this would save some catalog snapshot rebuilds for
inside-a-transaction-block cases, but I'm not sure it's worth sweating
that when we're doing client message exchange anyway.

Lastly, I find myself disliking the separate CatalogSnapshotStale flag
variable.  The other special snapshots in snapmgr.c are managed by setting
the pointer to NULL when it's not valid, so I wonder why CatalogSnapshot
wasn't done that way.  Since this patch is touching almost every use of
that flag already, it wouldn't take much to switch it over.

Comments?

            regards, tom lane

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 599874e..c9da8ed 100644
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*************** exec_parse_message(const char *query_str
*** 1352,1357 ****
--- 1352,1358 ----
          /* Done with the snapshot used for parsing */
          if (snapshot_set)
              PopActiveSnapshot();
+         InvalidateCatalogSnapshot();
      }
      else
      {
*************** exec_bind_message(StringInfo input_messa
*** 1778,1783 ****
--- 1779,1785 ----
      /* Done with the snapshot used for parameter I/O and parsing/planning */
      if (snapshot_set)
          PopActiveSnapshot();
+     InvalidateCatalogSnapshot();

      /*
       * And we're ready to start portal execution.
*************** exec_execute_message(const char *portal_
*** 2000,2005 ****
--- 2002,2012 ----
               * those that start or end a transaction block.
               */
              CommandCounterIncrement();
+
+             /*
+              * Flush catalog snapshot after every query, too.
+              */
+             InvalidateCatalogSnapshot();
          }

          /* Send appropriate CommandComplete to client */
*************** finish_xact_command(void)
*** 2456,2461 ****
--- 2463,2471 ----

          CommitTransactionCommand();

+         /* Flush catalog snapshot if any */
+         InvalidateCatalogSnapshot();
+
  #ifdef MEMORY_CONTEXT_CHECKING
          /* Check all memory contexts that weren't freed during commit */
          /* (those that were, were checked before being deleted) */
*************** PostgresMain(int argc, char *argv[],
*** 3871,3876 ****
--- 3881,3888 ----
           */
          AbortCurrentTransaction();

+         InvalidateCatalogSnapshot();
+
          if (am_walsender)
              WalSndErrorCleanup();

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 1ec9f70..842e135 100644
*** a/src/backend/utils/time/snapmgr.c
--- b/src/backend/utils/time/snapmgr.c
*************** GetTransactionSnapshot(void)
*** 316,321 ****
--- 316,327 ----
      /* First call in transaction? */
      if (!FirstSnapshotSet)
      {
+         /*
+          * Don't allow catalog snapshot to be older than xact snapshot.  Must
+          * do this first to allow the empty-heap Assert to succeed.
+          */
+         InvalidateCatalogSnapshot();
+
          Assert(pairingheap_is_empty(&RegisteredSnapshots));
          Assert(FirstXactSnapshot == NULL);

*************** GetTransactionSnapshot(void)
*** 347,355 ****
          else
              CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);

-         /* Don't allow catalog snapshot to be older than xact snapshot. */
-         CatalogSnapshotStale = true;
-
          FirstSnapshotSet = true;
          return CurrentSnapshot;
      }
--- 353,358 ----
*************** GetTransactionSnapshot(void)
*** 358,364 ****
          return CurrentSnapshot;

      /* Don't allow catalog snapshot to be older than xact snapshot. */
!     CatalogSnapshotStale = true;

      CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);

--- 361,367 ----
          return CurrentSnapshot;

      /* Don't allow catalog snapshot to be older than xact snapshot. */
!     InvalidateCatalogSnapshot();

      CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);

*************** GetNonHistoricCatalogSnapshot(Oid relid)
*** 465,471 ****
       */
      if (!CatalogSnapshotStale && !RelationInvalidatesSnapshotsOnly(relid) &&
          !RelationHasSysCache(relid))
!         CatalogSnapshotStale = true;

      if (CatalogSnapshotStale)
      {
--- 468,474 ----
       */
      if (!CatalogSnapshotStale && !RelationInvalidatesSnapshotsOnly(relid) &&
          !RelationHasSysCache(relid))
!         InvalidateCatalogSnapshot();

      if (CatalogSnapshotStale)
      {
*************** GetNonHistoricCatalogSnapshot(Oid relid)
*** 473,478 ****
--- 476,491 ----
          CatalogSnapshot = GetSnapshotData(&CatalogSnapshotData);

          /*
+          * Make sure the catalog snapshot will be accounted for in decisions
+          * about advancing PGXACT->xmin.  We could apply RegisterSnapshot, but
+          * that would result in making a physical copy, which is overkill; and
+          * it would also create a dependency on some resource owner.  Instead
+          * just shove the CatalogSnapshot into the pairing heap manually.
+          * This has to be reversed in InvalidateCatalogSnapshot, of course.
+          */
+         pairingheap_add(&RegisteredSnapshots, &CatalogSnapshot->ph_node);
+
+         /*
           * Mark new snapshost as valid.  We must do this last, in case an
           * ERROR occurs inside GetSnapshotData().
           */
*************** GetNonHistoricCatalogSnapshot(Oid relid)
*** 492,498 ****
  void
  InvalidateCatalogSnapshot(void)
  {
!     CatalogSnapshotStale = true;
  }

  /*
--- 505,516 ----
  void
  InvalidateCatalogSnapshot(void)
  {
!     if (!CatalogSnapshotStale)
!     {
!         CatalogSnapshotStale = true;
!         pairingheap_remove(&RegisteredSnapshots, &CatalogSnapshot->ph_node);
!         SnapshotResetXmin();
!     }
  }

  /*
*************** SnapshotResetXmin(void)
*** 930,935 ****
--- 948,954 ----

      if (pairingheap_is_empty(&RegisteredSnapshots))
      {
+         Assert(CatalogSnapshotStale);
          MyPgXact->xmin = InvalidTransactionId;
          return;
      }
*************** AtEOXact_Snapshot(bool isCommit)
*** 1058,1063 ****
--- 1077,1085 ----
          exportedSnapshots = NIL;
      }

+     /* Drop catalog snapshot if any */
+     InvalidateCatalogSnapshot();
+
      /* On commit, complain about leftover snapshots */
      if (isCommit)
      {