On Wed, 12 Jun 2024 at 11:38, Alena Rybakina <lena.ribackina@yandex.ru> wrote:
>
> Hi!
>
> On 11.06.2024 16:09, Alena Rybakina wrote:
>
> On 08.06.2024 09:30, Alena Rybakina wrote:
>
> I am currently working on dividing this patch into three parts to simplify the review process: one of them will
containcode for collecting vacuum statistics on tables, the second on indexes and the last on databases.
>
> I have divided the patch into three: the first patch contains code for the functionality of collecting and storage
fortables, the second one for indexes and the last one for databases.
>
> --
> Regards,
> Alena Rybakina
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
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?
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.
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?
Other places look more or less fine to me.
However, I'll maybe post some additional nit-picky comments on the
next patch version.
--
Best regards,
Kirill Reshke
