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
