On Mon, Jan 07, 2019 at 06:10:21PM +0900, Masahiko Sawada wrote: > 0002 and 0003 are merged and posted by Michael-san and it looks good > to me, so I've looked at the 0001, 0004, 0005 and 0006 patches. Here > is a few review comments.
I have done another round on 0002/0003 (PQfinish was lacking after error-ing on incompatible options) and committed the thing. > Even if all tables are filtered by --min-relation-size, min-mxid-age > or min-xid-age the following message appeared. > > $ vacuumdb --verbose --min-relation-size 1000000 postgres > vacuumdb: vacuuming database "postgres" > > Since no tables are processed in this case isn't is better not to > output this message by moving the message after checking if ntup == > 0? Hmm. My take on this one is that we attempt to vacuum relations on this database, but this may finish by doing nothing. Printing this message is still important to let the user know that an attempt was done so I would keep the message. > You use pg_total_relation_size() to check the relation size but it > might be better to use pg_relation_size() instead. The > pg_total_relation_size() includes the all index size but I think it's > common based on my experience that we check only the table size > (including toast table) to do vacuum it or not. Yes, I am also not completely sure if the way things are defined in the patch are completely what we are looking for. Still, I have seen as well people complain about the total amount of space a table is physically taking on disk, including its indexes. So from the user experience the direction taken by the patch makes sense, as much as does the argument you do. -- Michael
signature.asc
Description: PGP signature
