Re: Vacuum statistics

On Wed, 13 Nov 2024 at 21:21, Alena Rybakina <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>
> Hi! Thank you for your contribution to this thread!
>
> On 13.11.2024 03:24, Jim Nasby wrote:
>
> On Nov 10, 2024, at 2:09 PM, Alena Rybakina <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>
>
> On 08.11.2024 22:34, Jim Nasby wrote:
>
>
> On Nov 2, 2024, at 7:22 AM, Alena Rybakina <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>
> The second is the interrupts field. It is needed for monitoring to know
> do we have them or not, so tracking them on the database level will do
> the trick. Interrupt is quite rare event, so once the monitoring system
> will catch one the DBA can go to the server log for the details.
>
> Just to confirm… by “interrupt” you mean vacuum encountered an error?
>
> Yes it is.
>
> In that case I feel rather strongly that we should label that as “errors”. “Interrupt” could mean a few different things, but “error” is very clear.
>
> I updated patches. I excluded system and user time statistics and save number of interrupts only for database. I removed the ability to get statistics for all tables, now they can only be obtained for an oid table [0], as suggested here. I also renamed the statistics from pg_stat_vacuum_tables to pg_stat_get_vacuum_tables and similarly for indexes and databases. I noticed that that’s what they’re mostly called. Ready for discussion.
>
> I think it’s better that the views follow the existing naming conventions (which don’t include “_get_”; only the functions have that in their names). Assuming that, the only question becomes pg_stat_vacuum_* vs pg_stat_*_vacuum. Given the existing precedent of pg_statio_*, I’m inclined to go with pg_stat_vacuum_*.
>
> I have fixed it.
>
>
> I’ve reviewed and made some cosmetic changes to patch 1, though of note it looks like an effort has been made to keep stat_reset_timestamp at the end of PgStat_StatDBEntry, so I re-arranged that. I also removed some obviously dead code. It appears that pgstat_update_snapshot(), InitSnapshotIterator() and ScanStatSnapshot() are also dead, but I’ve left it in incase I’m missing something. The tests are also failing for me because a number of psql variables aren’t set.
>
> Thank you! Yes, I have deleted them.
>
>
> I do think we should separate out the counts for deleted but still visible tuples vs tuples where we couldn’t get a cleanup lock (in other words, recently_dead_tuples and missed_dead_tuples from LVRelState). I realize that’s a departure from how some of the existing reporting works, but IMO combining them together isn’t a pattern we should be repeating since they mean completely different things. Towards that end I did remove missed_dead_tuples from the reporting, and renamed ExtVacReport.dead_tuples to recently_dead_tuples, but I stopped short of creating a separate entry for missed_dead_tuples. Note that while recently_dead_tuples is really a global thing (so only needs to be reported at a global (or at most per-database) level, but missed_dead_tuples should really be at a per-table level.
>
> I am willing to agree with your idea. But we need to think about how clearly describe them in the documentation.
>
>
> Updated 0001-v13 attached, as well as the diff between v12 and v13.
>
> Thank you)
>
> And I agree with your changes. And included them in patches.
>
> ---
> Regards,
> Alena Rybakina
> Postgres Professional

Hello!
After a brief glance, I think this patch set is good.
But there isn't any more time in the current CF to commit this :(.
So I moved to the next CF.

I also like the 0001 commit message. This commit message is quite
large and easy to understand. Actually, it might be too big. Perhaps
rather of being a commit message, the final paragraph (pages_frozen -
number of pages that..) need to be a part of the document. Perhaps
delete the explanation on pages_frozen that we have in 0004?

--
Best regards,
Kirill Reshke