Re: [patch] BUG #15005: ANALYZE can make pg_class.reltuples inaccurate. - Mailing list pgsql-hackers

I wrote:
> Re-reading that thread, it seems like we should have applied Jeff's
> initial trivial patch[1] (to not hold AutovacuumScheduleLock across
> table_recheck_autovac) rather than waiting around for a super duper
> improvement to get agreed on.  I'm a bit tempted to go do that;
> if nothing else, it seems simple enough to back-patch, unlike most
> of the rest of what was discussed.

Jeff mentioned that that patch wasn't entirely trivial to rebase over
15739393e, and I now see why: in the code structure as it stands,
we don't know soon enough whether the table is shared.  In the
attached rebase, I solved that with the brute-force method of just
reading the pg_class tuple an extra time.  I think this is probably
good enough, really.  I thought about having do_autovacuum pass down
the tuple to table_recheck_autovac so as to not increase the net
number of syscache fetches, but I'm slightly worried about whether
we could be passing a stale pg_class tuple to table_recheck_autovac
if we do it like that.  OTOH, that just raises the question of why
we are doing any of this while holding no lock whatever on the target
table :-(.  I'm content to leave that question to the major redesign
that was speculated about in the bug #13750 thread.

This also corrects the inconsistency that at the bottom, do_autovacuum
clears wi_tableoid while taking AutovacuumLock, not AutovacuumScheduleLock
as is the documented lock for that field.  There's no actual bug there,
but cleaning this up might provide a slight improvement in concurrency
for operations that need AutovacuumLock but aren't looking at other
processes' wi_tableoid.  (Alternatively, we could decide that there's
no real need anymore for the separate AutovacuumScheduleLock, but that's
more churn than I wanted to deal with here.)

I think this is OK to commit/backpatch --- any objections?

            regards, tom lane

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 21f5e2c..639bd71 100644
*** a/src/backend/postmaster/autovacuum.c
--- b/src/backend/postmaster/autovacuum.c
*************** typedef struct autovac_table
*** 212,220 ****
   * wi_launchtime Time at which this worker was launched
   * wi_cost_*    Vacuum cost-based delay parameters current in this worker
   *
!  * All fields are protected by AutovacuumLock, except for wi_tableoid which is
!  * protected by AutovacuumScheduleLock (which is read-only for everyone except
!  * that worker itself).
   *-------------
   */
  typedef struct WorkerInfoData
--- 212,220 ----
   * wi_launchtime Time at which this worker was launched
   * wi_cost_*    Vacuum cost-based delay parameters current in this worker
   *
!  * All fields are protected by AutovacuumLock, except for wi_tableoid and
!  * wi_sharedrel which are protected by AutovacuumScheduleLock (note these
!  * two fields are read-only for everyone except that worker itself).
   *-------------
   */
  typedef struct WorkerInfoData
*************** do_autovacuum(void)
*** 2317,2323 ****
--- 2317,2325 ----
      foreach(cell, table_oids)
      {
          Oid            relid = lfirst_oid(cell);
+         HeapTuple    classTup;
          autovac_table *tab;
+         bool        isshared;
          bool        skipit;
          int            stdVacuumCostDelay;
          int            stdVacuumCostLimit;
*************** do_autovacuum(void)
*** 2342,2350 ****
          }

          /*
!          * hold schedule lock from here until we're sure that this table still
!          * needs vacuuming.  We also need the AutovacuumLock to walk the
!          * worker array, but we'll let go of that one quickly.
           */
          LWLockAcquire(AutovacuumScheduleLock, LW_EXCLUSIVE);
          LWLockAcquire(AutovacuumLock, LW_SHARED);
--- 2344,2366 ----
          }

          /*
!          * Find out whether the table is shared or not.  (It's slightly
!          * annoying to fetch the syscache entry just for this, but in typical
!          * cases it adds little cost because table_recheck_autovac would
!          * refetch the entry anyway.  We could buy that back by copying the
!          * tuple here and passing it to table_recheck_autovac, but that
!          * increases the odds of that function working with stale data.)
!          */
!         classTup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
!         if (!HeapTupleIsValid(classTup))
!             continue;            /* somebody deleted the rel, forget it */
!         isshared = ((Form_pg_class) GETSTRUCT(classTup))->relisshared;
!         ReleaseSysCache(classTup);
!
!         /*
!          * Hold schedule lock from here until we've claimed the table.  We
!          * also need the AutovacuumLock to walk the worker array, but that one
!          * can just be a shared lock.
           */
          LWLockAcquire(AutovacuumScheduleLock, LW_EXCLUSIVE);
          LWLockAcquire(AutovacuumLock, LW_SHARED);
*************** do_autovacuum(void)
*** 2381,2386 ****
--- 2397,2412 ----
          }

          /*
+          * Store the table's OID in shared memory before releasing the
+          * schedule lock, so that other workers don't try to vacuum it
+          * concurrently.  (We claim it here so as not to hold
+          * AutovacuumScheduleLock while rechecking the stats.)
+          */
+         MyWorkerInfo->wi_tableoid = relid;
+         MyWorkerInfo->wi_sharedrel = isshared;
+         LWLockRelease(AutovacuumScheduleLock);
+
+         /*
           * Check whether pgstat data still says we need to vacuum this table.
           * It could have changed if something else processed the table while
           * we weren't looking.
*************** do_autovacuum(void)
*** 2396,2414 ****
          if (tab == NULL)
          {
              /* someone else vacuumed the table, or it went away */
              LWLockRelease(AutovacuumScheduleLock);
              continue;
          }

          /*
-          * Ok, good to go.  Store the table in shared memory before releasing
-          * the lock so that other workers don't vacuum it concurrently.
-          */
-         MyWorkerInfo->wi_tableoid = relid;
-         MyWorkerInfo->wi_sharedrel = tab->at_sharedrel;
-         LWLockRelease(AutovacuumScheduleLock);
-
-         /*
           * Remember the prevailing values of the vacuum cost GUCs.  We have to
           * restore these at the bottom of the loop, else we'll compute wrong
           * values in the next iteration of autovac_balance_cost().
--- 2422,2435 ----
          if (tab == NULL)
          {
              /* someone else vacuumed the table, or it went away */
+             LWLockAcquire(AutovacuumScheduleLock, LW_EXCLUSIVE);
+             MyWorkerInfo->wi_tableoid = InvalidOid;
+             MyWorkerInfo->wi_sharedrel = false;
              LWLockRelease(AutovacuumScheduleLock);
              continue;
          }

          /*
           * Remember the prevailing values of the vacuum cost GUCs.  We have to
           * restore these at the bottom of the loop, else we'll compute wrong
           * values in the next iteration of autovac_balance_cost().
*************** deleted:
*** 2522,2531 ****
           * settings, so we don't want to give up our share of I/O for a very
           * short interval and thereby thrash the global balance.
           */
!         LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
          MyWorkerInfo->wi_tableoid = InvalidOid;
          MyWorkerInfo->wi_sharedrel = false;
!         LWLockRelease(AutovacuumLock);

          /* restore vacuum cost GUCs for the next iteration */
          VacuumCostDelay = stdVacuumCostDelay;
--- 2543,2552 ----
           * settings, so we don't want to give up our share of I/O for a very
           * short interval and thereby thrash the global balance.
           */
!         LWLockAcquire(AutovacuumScheduleLock, LW_EXCLUSIVE);
          MyWorkerInfo->wi_tableoid = InvalidOid;
          MyWorkerInfo->wi_sharedrel = false;
!         LWLockRelease(AutovacuumScheduleLock);

          /* restore vacuum cost GUCs for the next iteration */
          VacuumCostDelay = stdVacuumCostDelay;