Re: Vacuum statistics

Hi! Thank you for inquiring about our topic!

On 10.08.2024 23:57, Kirill Reshke wrote:
> Hi!
> Few suggestions on this patch-set
>
> 1)
>> +{ oid => '4701',
>> +  descr => 'pg_stats_vacuum_tables return stats values',
>> +  proname => 'pg_stats_vacuum_tables', provolatile => 's', prorettype => 'record',proisstrict => 'f',
>> +  proretset => 't',
>> +  proargtypes => 'oid oid',
>> +  proallargtypes =>
> During development, OIDs should be picked up from range 8000-9999.
> Same for pg_stats_vacuum_database & pg_stats_vacuum_indexes
>
> Also, why are these function naming schemes like
> pg_stats_vacuum_*something*, not pg_stat_vacuum_*something*, like
> pg_stat_replication etc?
To be honest, when I named it, I missed this aspect. I thought about the 
plural vacuum statistics we show, so I named them. I fixed it.
> 2) In 0003:
>> +  proargnames => '{dboid,dboid,db_blks_read,db_blks_hit,total_blks_dirtied,total_blks_written,wal_records,wal_fpi,wal_bytes,blk_read_time,blk_write_time,delay_time,system_time,user_time,total_time,interrupts}',
> Repeated dboid arg name is strange. Is it done this way to make
> pg_stats_vacuum function call in more unified fashion? I don't see any
> other place within postgresql core with similar approach, so I doubt
> it is correct.
Both parameters are required for input and output. We are trying to find 
statistics for a specific database if the database oid was specified by 
the user or display statistics for all objects, but we need to display 
which database these statistics are for. I corrected the name of the 
first parameter.
> 3) 0001 patch vacuum_tables_statistics test creates
> statistic_vacuum_database1, but does not use it. 0003 do.
> Also I'm not sure if these additional checks on the second database
> adds much value. Can you justify this please?

The statistic_vacuum_database1 needs us to check the visible of 
statistics from another database (statistic_vacuum_database) as they are 
after the manipulation with tables in another database, and after 
deleting the vestat table . In the latter case, we need to be sure that 
all the table statistics are not visible to us.

So, I agree that it should be added only in the latest version of the 
patch, where we add vacuum statistics for databases. I fixed it.

> Other places look more or less fine to me.
> However, I'll maybe post some additional nit-picky comments on the
> next patch version.
We are glad any feedback and review, so feel free to do it)

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