Re: Vacuum statistics

Hi,

On Tue, Aug 11, 2026 at 5:10 PM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
>
> On Thu, Jul 23, 2026 at 12:20 PM Alena Rybakina
> <lena(dot)ribackina(at)yandex(dot)ru> wrote:
>>
> > and, like everything else backends collect, flow into the cumulative
> > statistics system. Nevertheless, the extension gives much more
> > flexibility, and adding filters later on will make it possible to reduce
> > the amount of memory reserved for storing the statistics compared to
> > what the in-core version claimed.
>> .....
>> So you made
> > me rethink the shape of the patch set, and I've reworked it around the
> > approach you (and Amit upthread) suggested. Attached is v42,
> > restructured as follows; per-patch replies are below.
>
> I've some notes regarding the v42 version of the patch.

Thanks for sharing thoughts.

> There are no hooks for pgstat_drop_relation()/pg_stat_drop_database().
> And the shared-memory stats subsystem has no periodic reaper for
> "object gone => drop entry". That could lead to garbage accumulation.

Can't the existing object access type (OAT) hooks for relation and
database drops implemented in an external module take care of this
problem? For database drop, the OAT hook implementation could look at
all the entries of the relations that belonged to the database being
dropped and handle them.

> Where the garbage physically accumulates:
>
> 1. Shared memory: the pgstat shared hash. One
> PgStatShared_ExtVacEntry per relation plus one per database. Grows
> without bound under DROP/CREATE churn.
> 2. The stats file pg_stat/pgstat.stat. Both kinds (relation and
> database) have .write_to_file = true, so entries are serialised on
> clean shutdown (pgstat_write_statsfile()) and read back at startup
> (pgstat_read_statsfile()) with no existence check. Therefore, the
> garbage survives restarts.
>
> Also, previously existed OID could be re-used for another
> relation/database. That would cause abandoned statistics to be
> handled with new relation/database. Built-in stats avoid this by
> dropping corresponding entries explicitly.

Unless I'm missing something, OAT hooks could nicely solve this problem.

> Additionally, pgstat_report_vacuum_error() looks a big dangerous. It
> takes LWLock to update the shared memory inside the error handler.
> Could we better save the fast of the error and process it later after
> exit from the error handler?

Nice catch! Taking LWLock inside the error handler can lead to
problems. The error may come from the LWLock code while holding it
(re-acquiring it would lead to deadlocks or would throw another
ERROR), or from corrupted shared memory (likely a higher severity than
ERROR). Although pgstat_report_vacuum_error() is called only for
ERROR, taking LWLock in the handler still looks risky.

Also, I quickly looked at a couple of existing error callbacks and
none of them seem to be doing heavy stuff like taking locks.

So, +1 to just increment the counter in the callback and report it at
a later safe point.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com