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
