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.

Hello Royce,

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.


* 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.

* 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.

* The number shows correctly in the pg_stat_relation. This is a testcase that gives unremovable dead rows:

create table b (a int);
insert into b values (1);

begin transaction ISOLATION LEVEL repeatable read;
select * from b;

update t set b=2 where i=10;
vacuum verbose t;

Then something similar is shown:

INFO:  vacuuming "public.t"
INFO:  index "t_pkey" now contains 1 row versions in 2 pages
DETAIL:  0 index row versions were removed.
0 index pages have been deleted, 0 are currently reusable.
CPU 0.00s/0.00u sec elapsed 0.00 sec.
INFO: "t": found 0 removable, 2 nonremovable row versions in 1 out of 1 pages
DETAIL:  1 dead row versions cannot be removed yet.
There were 0 unused item pointers.
0 pages are entirely empty.
CPU 0.00s/0.01u sec elapsed 0.00 sec.
t=# \x
Expanded display is on.
t=# select * from pg_stat_user_tables;
-[ RECORD 1 ]-----+------------------------------
relid             | 16407
schemaname        | public
relname           | t
seq_scan          | 1
seq_tup_read      | 0
idx_scan          | 1
idx_tup_fetch     | 1
n_tup_ins         | 1
n_tup_upd         | 1
n_tup_del         | 0
n_tup_hot_upd     | 1
n_live_tup        | 2
n_dead_tup        | 0
n_unremovable_tup | 1
last_vacuum       | 2011-11-05 21:15:06.939928+01
last_autovacuum   |
last_analyze      |
last_autoanalyze  |
vacuum_count      | 1
autovacuum_count  | 0
analyze_count     | 0
autoanalyze_count | 0

I did some more tests with larger number of updates which revealed no unexpected results.

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.

Yeb Havinga

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to