Re: Vacuum statistics

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.rybakina@postgrespro.ru> wrote:
>>
>> On 08.11.2024 22:34, Jim Nasby wrote:
>>>
>>>> On Nov 2, 2024, at 7:22 AM, Alena Rybakina 
>>>> <a.rybakina@postgrespro.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