Re: Vacuum statistics

Hi!

On 23.08.2024 04:07, Alexander Korotkov wrote:
> On Wed, Aug 21, 2024 at 1:39 AM Alena Rybakina
> <a(dot)rybakina(at)postgrespro(dot)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.
Thank you very much for your review and contribution to this thread!
>
> +   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.
I fixed it by deleting this part pf the code. We can display statistics
only for current database.
>
> +/*
> + * 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?
I think you are right, we can not return anything. Fixed.
>
> @@ -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().
Fixed.
>
> +        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.
Fixed.
>
> +static PgStat_StatTabEntry *
> +fetch_dbstat_tabentry(Oid dbid, Oid relid)
> +{
> +  Oid                  storedMyDatabaseId = MyDatabaseId;
> +  PgStat_StatTabEntry  *tabentry = NULL;
> +
> +  if (OidIsValid(CurrentDatabaseId) && CurrentDatabaseId == dbid)
> +     /* Quick path when we read data from the same database */
> +     return pgstat_fetch_stat_tabentry(relid);
> +
> +  pgstat_clear_snapshot();
>
> It looks scary to reset the whole snapshot each time we access another
> database.  Need to also mention that the CurrentDatabaseId machinery
> isn't implemented.
Fixed.
>
> New functions
> pg_stat_vacuum_tables(), pg_stat_vacuum_indexes(), pg_stat_vacuum_database()
> are SRFs.  When zero Oid is passed they report all the objects. 
> However, it seems they aren't intended to be used directly.  Instead,
> there are views with the same names. These views always call them with
> particular Oids, therefore SRFs always return one row.  Then why
> bother with SRF?  They could return plain records instead.

I didn't understand correctly - did you mean that we don't need SRF if
we need to display statistics for a specific object?

Otherwise, we need this when we display information on all database
objects (tables or indexes):

while ((entry = ScanStatSnapshot(pgStatLocal.snapshot.stats, &hashiter))
!= NULL)
{
    CHECK_FOR_INTERRUPTS();

    tabentry = (PgStat_StatTabEntry *) entry->data;

    if (tabentry != NULL && tabentry->vacuum_ext.type == type)
        tuplestore_put_for_relation(relid, rsinfo, tabentry);
}

I know we can construct a HeapTuple object containing a TupleDesc,
values, and nulls for a particular object, but I'm not sure we can
augment it while looping through multiple objects.

/* Initialise attributes information in the tuple descriptor */

 tupdesc = CreateTemplateTupleDesc(PG_STAT_GET_SUBSCRIPTION_STATS_COLS);

...

PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)));

If I missed something or misunderstood, can you explain in more detail?

>
> Also, as I mentioned above patchset makes a lot of trouble accessing
> statistics of relations of another database.  But that seems to be
> useless given corresponding views allow to see only relations of the
> current database.  Even if you call functions directly, what is the
> value of this information given that you don't know the relation oids
> in another database?  So, I think if we will give up and limit access
> to the relations of the current database patch will become simpler and
> clearer.
>
I agree with that and have fixed it already.

--
Regards,
Alena Rybakina
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company