On 16/11/2011, at 2:05 AM, Yeb Havinga wrote: > On 2011-10-05 00:45, Royce Ausburn wrote: >> Attached is v2 of my patch that doesn't update CATALOG_VERSION_NO. I've >> also fixed the name of an argument to pgstat_report_vacuum which I don't >> think was particularly good, and I've replace the word "tuple" with "row" in >> some docs I added for consistency. >> > > I reviewed your patch. I think it is in good shape, my two main remarks (name > of n_unremovable_tup and a remark about documentation at the end of this > review) are highly subjective and I wouldn't spend time on it unless other > people have the same opinion. > > Remarks: > > * rules regression test fails because pg_stat_all_tables is changed, > pg_stat_sys_tables and pg_stat_user_tables as well, but the new > expected/rules.out is not part of the patch.
Doh! Thank you for spotting this. Should we decide to continue this patch I'll look in to fixing this. > > * I'm not sure about the name n_unremovable_tup, since it doesn't convey it > is about dead tuples and judging from only the name it might as well include > the live tuples. It also doesn't hint that it is a transient condition, which > vacuum verbose does with the word 'yet'. > What about n_locked_dead_tup? - this contains both information that it is > about dead tuples, and the lock suggests that once the lock is removed, the > dead tuple can be removed. > Looks like we have some decent candidates later in the thread. Should this patch survive I'll look at updating it. > * The number shows correctly in the pg_stat_relation. This is a testcase that > gives unremovable dead rows: > > I was puzzled for a while that n_unremovable_tup >= n_dead_tup doesn't hold, > after all, the unremovable tuples are dead as well. Neither the current > documentation nor the documentation added by the patch do help in explaining > the meaning of n_dead_tup and n_unremovable_tup, which may be clear to > seasoned vacuum hackers, but not to me. In both the case of n_dead_tup it > would have been nice if the docs mentioned that dead tuples are tuples that > are deleted or previous versions of updated tuples, and that only analyze > updates n_dead_tup (since vacuum cleans them), in contrast with > n_unremovable_tup that gets updated by vacuum. Giving an example of how > unremovable dead tuples can be caused would IMHO also help understanding. Fair enough! I'll review this as well. Thanks again! --Royce -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
