Subject: Re: PoC: history of recent vacuum/checkpoint runs (using new hooks)

From: wenhui qiu <[email-masked]>
Date: Fri, 27 Dec 2024 00:00:36 +0800

Hi

As far as I know, more than 10,000 tables  of instances  are often
encountered,
So I insist that the maximum can be appropriately increased to 256MB，Can be
more adaptable to many table situations

On Thu, 26 Dec 2024 at 23:23, Robert Treat <[email-masked]> wrote:

> On Wed, Dec 25, 2024 at 12:26 PM Tomas Vondra <[email-masked]> wrote:
> >
> > Hi,
> >
> > On 12/23/24 07:35, wenhui qiu wrote:
> > > Hi Tomas
> > >      This is a great feature.
> > > + /*
> > > + * Define (or redefine) custom GUC variables.
> > > + */
> > > + DefineCustomIntVariable("stats_history.size",
> > > + "Sets the amount of memory available for past events.",
> > > + NULL,
> > > + &statsHistorySizeMB,
> > > + 1,
> > > + 1,
> > > + 128,
> > > + PGC_POSTMASTER,
> > > + GUC_UNIT_MB,
> > > + NULL,
> > > + NULL,
> > > + NULL);
> > > +
> > > RAM is in terabytes now, the statsHistorySize is 128MB ,I think can
> > > increase to store more history record ?
> > >
> >
> > Maybe, the 128MB is an arbitrary (and conservative) limit - it's enough
> > for ~500k events, which seems good enough for most systems. Of course,
> > on systems with many relations might need more space, not sure.
> >
> > I was thinking about specifying the space in more natural terms, either
> > as amount of time ("keep 1 day of history") or number of entries ("10k
> > entries"). That would probably mean the memory can't be allocated as
> > fixed size.
> >
>
> Based on the above, a rough calculation is that this is enough for
> holding 1 year of hourly vacuum runs for 50 tables, or a year of daily
> vacuums for 1000 tables. Most folks will fall somewhere in that range
> (and won't really need a year's history) but that seems like plenty
> for a default.
>
> > But maybe it'd be possible to just write the entries to a file. We don't
> > need random access to past entries (unlike e.g. pg_stat_statements), and
> > people won't query that very often either.
> >
>
> Yeah, workloads will vary, but it doesn't seem like they would more
> than query workloads do.
>
> Robert Treat
> https://xzilla.net
>

---

From: Tomas Vondra <[email-masked]>
Date: Thu, 26 Dec 2024 18:58:11 +0100

On 12/26/24 17:00, wenhui qiu wrote:
> So I insist that the maximum can be appropriately increased to 256MB，
> Can be more adaptable to many table situations
> 

If 128MB is insufficient, why would 256MB be OK? A factor of 2x does not
make a fundamental difference ...

Anyway, the 128MB value is rather arbitrary. I don't mind increasing the
limit, or possibly removing it entirely (and accepting anything the
system can handle).

regards

-- 
Tomas Vondra

---

From: Michael Paquier <[email-masked]>
Date: Fri, 27 Dec 2024 13:00:37 +0900

How about some time-based retention?  Data size can be hard to think
about for the end user, while there would be a set of users that would
want to retain data for the past week, month, etc?  If both size and
time upper-bound are define, then entries that match one condition or
the other are removed.

+        checkpoint_log_hook(
+          CheckpointStats.ckpt_start_t,            /* start_time */
+          CheckpointStats.ckpt_end_t,                /* end_time */
+          (flags & CHECKPOINT_IS_SHUTDOWN),        /* is_shutdown */
+          (flags & CHECKPOINT_END_OF_RECOVERY),    /* is_end_of_recovery */
+          (flags & CHECKPOINT_IMMEDIATE),            /* is_immediate */
+          (flags & CHECKPOINT_FORCE),                /* is_force */
+          (flags & CHECKPOINT_WAIT),                /* is_wait */
+          (flags & CHECKPOINT_CAUSE_XLOG),        /* is_wal */
+          (flags & CHECKPOINT_CAUSE_TIME),        /* is_time */
+          (flags & CHECKPOINT_FLUSH_ALL),            /* is_flush_all */
+          CheckpointStats.ckpt_bufs_written,        /* buffers_written */
+          CheckpointStats.ckpt_slru_written,        /* slru_written */
+          CheckpointStats.ckpt_segs_added,        /* segs_added */
+          CheckpointStats.ckpt_segs_removed,        /* segs_removed */
+          CheckpointStats.ckpt_segs_recycled,        /* segs_recycled */

That's a lot of arguments.  CheckpointStatsData and the various
CHECKPOINT_* flags are exposed, why not just send these values to the
hook?

For v1-0001 as well, I'd suggest some grouping with existing
structures, or expose these structures so as they can be reused for
out-of-core code via the proposed hook.  More arguments lead to more
mistakes that could be easily avoided.
--
Michael

---

From: Tomas Vondra <[email-masked]>
Date: Sat, 28 Dec 2024 02:25:16 +0100

> time upper-bound are define, then entries that match one condition or

Right. In my response [1] I suggested replacing the simple memory limit
with a time-based limit, but I haven't done anything about that yet.

And the more I think about it the more I'm convinced we don't need to
keep the data about past runs in memory, a file should be enough (except
maybe for a small buffer). That would mean we don't need to worry about
dynamic shared memory, etc. I initially rejected this because it seemed
like a regression to how pgstat worked initially (sharing data through
files), but I don't think that's actually true - this data is different
(almost append-only), etc.

The one case when we may need co read the data is in response to DROP of
a table, when we need to discard entries for that object. Or we could
handle that by recording OIDs of dropped objects ... not sure how
complex this would need to be.

We'd still want to enforce some limits, of course - a time-based limit
of the minimum amount of time to cover, and maximum amount of disk space
to use (more as a safety).

FWIW there's one "issue" with enforcing the time-based limit - we can
only do that for the "end time", because that's when we see the entry.
If you configure the limit to keep "1 day" history, and then a vacuum
completes after running for 2 days, we want to keep it, so that anyone
can actually see it.

[1]
https://www.postgresql.org/message-id/8df7cee1-31aa-4db3-bbb7-83157ca139da%40vondra.me

> mistakes that could be easily avoided.

Yes, I admit the number of parameters seemed a bit annoying to me too,
and maybe we could reduce that somewhat. Certainly for checkpoints,
where we already have a reasonable CheckpointStats struct and flags,
wrapping most of the fields.

With vacuum it's a bit more complicated, for two reasons: (a) the
counters are simply in LVRelState, mixed with all kinds of other info,
and it seems "not great" to pass it to a "log" hook, and (b) there are
some calculated values, so I guess the hook would need to do that
calculation on it's own, and it'd be easy to diverge over time.

For (a) we could introduce some "stats" struct to keep these counters
for vacuum (the nearby parallel vacuum patch actually does something
like that, I think). Not sure if (b) is actually a problem in practice,
but I guess we could add those fields to the new "stats" struct too.

regards

-- 
Tomas Vondra

---

From: Robert Treat <[email-masked]>
Date: Sun, 29 Dec 2024 10:39:13 -0500

> completes after running for 2 days, we want to keep it, so that anyone

I can't say I recall all the reasoning involved in making
pg_stat_statements just be based on a fixed number of entries, but the
ability to come up with corner cases was certainly a factor. For
example, imagine the scenario where you set a max at 30 days, but you
have some tables only being vacuumed every few months. Ideally you
probably want the last entry no matter what, and honestly probably the
last 2 (in case you are troubleshooting something, having the last run
and something to compare against is ideal). In any case, it can get
complicated pretty quickly.

> but I guess we could add those fields to the new "stats" struct too.

At the risk of increasing scope, since you already are working on
checkpoints along with vacuums, I'm curious if there was a reason not
to do analyze stats retention as well? It seems pretty correlated in
the same area/problems as vacuum history.

Robert Treat
https://xzilla.net

---

From: Tomas Vondra <[email-masked]>
Date: Sun, 29 Dec 2024 19:25:05 +0100

> complicated pretty quickly.

I really don't want to get into such complicated retention policies,
because there's no "right" solution, and it adds complexity both to
implementation (e.g. we'd need to keep per-relation counters for all the
various events), and processing (How would you know if there were no
other vacuum runs on a relation or if those other events were removed?)

I think the best solution to use a trivial retention policy (e.g. keep
everything for X days), and if you want to keep a longer history, you
need to read and archive that regularly (and the retention period
ensures you don't miss any events).

After all, that's what we assume for most other runtime stats anyway -
it's not very useful to know the current values, if you can't calculate
deltas. So you have to sample the stats.

There's also the trouble that this is not crash safe, so I'd hesitate to
rely on this very much if it can disappear.

> ...

I agree tracking ANALYZE would be useful. I ignored it mostly to keep
the scope as limited as possible, so two separate events were enough.
Adding another hook to do_analyze_rel() would be fairly trivial, but
I'll leave that for later.

regards

-- 
Tomas Vondra

---

From: Michael Paquier <[email-masked]>
Date: Tue, 31 Dec 2024 10:06:45 +0900

> (almost append-only), etc.

Right, I was looking a bit at 0003 that introduces the extension.  I
am wondering if we are not repeating the errors of pgss by using a
different file, and if we should not just use pgstats and its single 
file instead to store this data through an extension.  You are right
that as an append-only pattern using the dshash of pgstat does not fit
well into this picture.   How about the second type of stats kinds:
the fixed-numbered stats kind?  These allocate a fixed amount of
shared memory, meaning that you could allocate N entries of history
and just manage a queue of them, then do a memcpy() of the whole set
if adding new history at the head of the queue, or just append new
ones at the tail of the queue in shmem, memcpy() once the queue is
full.  The extension gets simpler:
- No need to manage a new file, flush of the stats is controlled by
pgstats itself.
- The extension could register a fixed-numbered custom stats kind.
- Push the stats with the new hooks.
- Perhaps less useful, but it is possible to control the timing where
the data is pushed.
- Add SQL wrappers on top to fetch the data from pgstat.
--
Michael

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 31 Dec 2024 16:06:26 +0100

> - The extension could register a fixed-numbered custom stats kind.

I'm not against leveraging some of the existing pstat infrastructure,
but as I explained earlier I don't like the "fixed amount of shmem"
approach. It's either wasteful (on machines with few vacuum runs) or
difficult to configure to keep enough history.

FWIW I'm not sure what you mean by "pgss errors" - I'm not saying it's
flawless, but OTOH reading/writing a flat file with entries is not
particularly hard. (It'd need to be a bit more complex to evict stuff
for dropped objects, but not by much).

> - Perhaps less useful, but it is possible to control the timing where

Not sure what you mean by "pushed". Pushed where, or how would that matter?

> - Add SQL wrappers on top to fetch the data from pgstat.

OK, although I think this is not particularly complicated part of the
code. It could even be simplified further.

regards

-- 
Tomas Vondra

---

From: Cédric Villemain <[email-masked]>
Date: Tue, 7 Jan 2025 10:47:42 +0100

> difficult to configure to keep enough history.

A bit late on this topic, I wrote StatsMgr which run stats snapshots 
based on interval, it covers only fixed stats at the moment though, it's 
ring buffer too, not perfect but does the job. It has many similarities 
and I though it'll be interesting to share what I was looking at to 
improve further.

I think it'll be interesting to have 2 hooks in PgStat_KindInfo 
functions call back:

* "flush_fixed_cb()": a hook inside to be able to execute other action 
while flushing, like generating aggregates ("counter summary")

* a new member to do the snapshot directly (or whatever action relevant 
when the stats are in an interesting state), maybe snapshot_fixed_cb(), 
with a hook inside.

This way, with 2 hooks we can cover both usages, for all stats (fixed 
here, but probably the same hooks for variable stats), and have actions 
on events instead of only on time interval.

I don't have strict opinion about files management but having a facility 
from PostgreSQL to store *metrics* (as opposed to "stats" which are 
required for PostgreSQL) will be very convenient. Maybe like SLRU or 
similar (the thing used to keep commits timestamp) ? I didn't checked 
all already available options in this area.

[1] https://codeberg.org/Data-Bene/StatsMgr

---
Cédric Villemain +33 6 20 30 22 52
https://www.Data-Bene.io
PostgreSQL Support, Expertise, Training, R&D