Re: statistics for shared catalogs not updated when autovacuum is off - Mailing list pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:
> The problem is different I think. Since 9.3, database-related
> statistics are located on separate files. And the statistics of shared
> tables is visibly located in a file with database name set as
> InvalidOid, leading to the presence of db_0.stat in pg_stat_tmp. So
> the fix for shared relations is to make sure that
> backend_read_statsfile can load the file dedicated to shared objects
> when data from it is needed, like pg_database stats. So making
> backend_read_statsfile a bit smarter looks like the good answer to me.

I don't think that's quite it.  The problem basically is that the stats
collector will only write out the shared-catalog stats file when it's
prompted to by a PGSTAT_MTYPE_INQUIRY message for database OID 0.
Ordinary backends will never do that; they always send their own DB OID.
The autovac launcher accidentally does that, because it calls
pgstat_send_inquiry with MyDatabaseId which it has never set nonzero.
So once per autovac launcher cycle, an update of the shared-catalog stats
file will happen on disk, but with autovac off it never happens at all.

Meanwhile, backends look only at the timestamp on the non-shared stats
file corresponding to their DB.  When that's sufficiently up to date,
they read in both that file and the shared-stats file and call it good.

> At the same time I am not getting why pgstat_fetch_stat_tabentry needs
> to be that complicated. Based on the relation OID we can know if it is
> a shared relation or not, there is no point in doing two times the
> same lookup in the pgstat hash table.

True, although I'm not sure if adding a dependency on IsSharedRelation()
here is a good thing.  In any case, you took the dependency too far ...

> Attached is a patch that fixes the issue here:

I have not tested it, but I would bet a lot that this patch is broken:
what will happen if the first request in a transaction is for a shared
catalog is that we'll read just the shared-catalog data, and then
subsequent requests for stats for an unshared table will find nothing.
Moreover, even if you undid the change to the pgstat_read_statsfiles()
call so that we still did read the appropriate unshared stats, this
would have the effect of reversing the problem: if the first request
is for a shared catalog, then we'd check to ensure the shared stats
are up-to-date, but fail to ensure that the unshared ones are.

We could possibly fix this by having backends perform the poll/request
logic in backend_read_statsfile() for *both* their own DB OID and OID 0.
(They would always have to do both, regardless of which type of table is
initially asked about, since there's no way to predict what other requests
will be made in the same transaction.)  This seems pretty inefficient to
me, though.  I think it would be better to redefine the behavior in
pgstat_write_statsfiles() so that the shared-catalog stats file is always
written if any DB statsfile write has been requested.  Then backends could
continue to poll just their own DB OID and expect to see up-to-date shared
stats as well.

I initially thought there might be a race condition with that, but there's
not really because pgstat_read_db_statsfile_timestamp() is not actually
looking at on-disk file timestamps: it's reading the global stats file and
seeing whether the per-db last write time recorded in that is new enough.
So all of the relevant data (global stats file, shared-catalog table stats
file, and per-database table stats file) will appear to update atomically.

In short, I think the attached is enough to fix it.  There's some cosmetic
cleanup that could be done here: a lot of these functions could use better
comments, and I'm not happy about the autovac launcher depending on
MyDatabaseId being zero.  But those are not functional problems.

            regards, tom lane

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 41f4374..f35e680 100644
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
*************** pgstat_db_requested(Oid databaseid)
*** 5504,5509 ****
--- 5504,5518 ----
  {
      slist_iter    iter;

+     /*
+      * If any requests are outstanding at all, we should write the stats for
+      * shared catalogs (the "database" with OID 0).  This ensures that
+      * backends will see up-to-date stats for shared catalogs, even though
+      * they send inquiry messages mentioning only their own DB.
+      */
+     if (databaseid == InvalidOid && !slist_is_empty(&last_statrequests))
+         return true;
+
      /* Check the databases if they need to refresh the stats. */
      slist_foreach(iter, &last_statrequests)
      {