Re: Vacuum statistics - Mailing list pgsql-hackers

On Wed, Aug 21, 2024 at 1:39 AM Alena Rybakina <a.rybakina@postgrespro.ru> wrote:
>
> I think you've counted the above system tables from the database, but
> I'll double-check it. Thank you for your review!
>
> On 19.08.2024 19:28, Ilia Evdokimov wrote:
> > Are you certain that all tables are included in
> > `pg_stat_vacuum_tables`? I'm asking because of the following:
> >
> >
> > SELECT count(*) FROM pg_stat_all_tables ;
> >  count
> > -------
> >    108
> > (1 row)
> >
> > SELECT count(*) FROM pg_stat_vacuum_tables ;
> >  count
> > -------
> >     20
> > (1 row)
> >

I'd like to do some review a well.

+   MyDatabaseId = dbid;
+
+   PG_TRY();
+   {
+       tabentry = pgstat_fetch_stat_tabentry(relid);
+       MyDatabaseId = storedMyDatabaseId;
+   }
+   PG_CATCH();
+   {
+       MyDatabaseId = storedMyDatabaseId;
+   }
+   PG_END_TRY();

I think this is generally wrong to change MyDatabaseId, especially if you have to wrap it with PG_TRY()/PG_CATCH().  I think, instead we need proper API changes, i.e. make pgstat_fetch_stat_tabentry() and others take dboid as an argument.

+/*
+ * Get the vacuum statistics for the heap tables.
+ */
+Datum
+pg_stat_vacuum_tables(PG_FUNCTION_ARGS)
+{
+   return pg_stats_vacuum(fcinfo, PGSTAT_EXTVAC_HEAP, EXTVACHEAPSTAT_COLUMNS);
+
+   PG_RETURN_NULL();
+}

The PG_RETURN_NULL() is unneeded after another return statement.  However, does pg_stats_vacuum() need to return anything?  What about making its return type void?

@@ -874,4 +874,38 @@ pgstat_get_custom_snapshot_data(PgStat_Kind kind)
   return pgStatLocal.snapshot.custom_data[idx];
 }
 
+/* hash table for statistics snapshots entry */
+typedef struct PgStat_SnapshotEntry
+{
+  PgStat_HashKey key;
+  char     status;        /* for simplehash use */
+  void     *data;         /* the stats data itself */
+} PgStat_SnapshotEntry;

It would be nice to preserve encapsulation and don't expose pgstat_snapshot hash in the headers.  I see there is only one usage of it outside of pgstat.c: pg_stats_vacuum().  

+        Oid                  storedMyDatabaseId = MyDatabaseId;
+
+        pgstat_update_snapshot(PGSTAT_KIND_RELATION);
+        MyDatabaseId = storedMyDatabaseId;

This manipulation with storedMyDatabaseId looks pretty useless.  It seems to be intended to change MyDatabaseId, while I'm not fan of this as I mentioned above.