From 9b626f6c334f95f1052b22b13d9ddbcc4de5fc4f Mon Sep 17 00:00:00 2001 From: Andrew Gierth Date: Thu, 16 Mar 2017 22:31:49 +0000 Subject: [PATCH] Avoid having vacuum set reltuples to 0 on non-empty relations in the presence of page pins, which leads to serious estimation errors in the planner. This particularly affects small heavily-accessed tables, especially where locking (e.g. from FK constraints) forces frequent vacuums for mxid cleanup. Fix by keeping separate track of pages whose live tuples were actually counted vs. pages that were only scanned for freezing purposes. Thus, reltuples can only be set to 0 if all pages of the relation were actually counted. Backpatch to all supported versions. Per bug #14057 from Nicolas Baccelli, analyzed by me. Discussion: https://postgr.es/m/20160331103739.8956.94469@wrigleys.postgresql.org --- src/backend/commands/vacuumlazy.c | 15 +++-- .../isolation/expected/vacuum-reltuples.out | 62 +++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + .../isolation/specs/vacuum-reltuples.spec | 45 ++++++++++++++ 4 files changed, 119 insertions(+), 4 deletions(-) create mode 100644 src/test/isolation/expected/vacuum-reltuples.out create mode 100644 src/test/isolation/specs/vacuum-reltuples.spec diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c index b5fb3250071bd..e1e39dfb4ee96 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -108,7 +108,8 @@ typedef struct LVRelStats BlockNumber scanned_pages; /* number of pages we examined */ BlockNumber pinskipped_pages; /* # of pages we skipped due to a pin */ BlockNumber frozenskipped_pages; /* # of frozen pages we skipped */ - double scanned_tuples; /* counts only tuples on scanned pages */ + BlockNumber tupcount_pages; /* pages whose tuples we counted */ + double scanned_tuples; /* counts only tuples on tupcount_pages */ double old_rel_tuples; /* previous value of pg_class.reltuples */ double new_rel_tuples; /* new estimated total # of tuples */ double new_dead_tuples; /* new estimated total # of dead tuples */ @@ -293,6 +294,10 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params, * density") with nonzero relpages and reltuples=0 (which means "zero * tuple density") unless there's some actual evidence for the latter. * + * It's important that we use tupcount_pages and not scanned_pages for the + * check described above; scanned_pages counts pages where we could not + * get cleanup lock, and which were processed only for frozenxid purposes. + * * We do update relallvisible even in the corner case, since if the table * is all-visible we'd definitely like to know that. But clamp the value * to be not more than what we're setting relpages to. @@ -302,7 +307,7 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params, */ new_rel_pages = vacrelstats->rel_pages; new_rel_tuples = vacrelstats->new_rel_tuples; - if (vacrelstats->scanned_pages == 0 && new_rel_pages > 0) + if (vacrelstats->tupcount_pages == 0 && new_rel_pages > 0) { new_rel_pages = vacrelstats->old_rel_pages; new_rel_tuples = vacrelstats->old_rel_tuples; @@ -489,6 +494,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats, nblocks = RelationGetNumberOfBlocks(onerel); vacrelstats->rel_pages = nblocks; vacrelstats->scanned_pages = 0; + vacrelstats->tupcount_pages = 0; vacrelstats->nonempty_pages = 0; vacrelstats->latestRemovedXid = InvalidTransactionId; @@ -811,6 +817,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats, } vacrelstats->scanned_pages++; + vacrelstats->tupcount_pages++; page = BufferGetPage(buf); @@ -1254,7 +1261,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats, /* now we can compute the new value for pg_class.reltuples */ vacrelstats->new_rel_tuples = vac_estimate_reltuples(onerel, false, nblocks, - vacrelstats->scanned_pages, + vacrelstats->tupcount_pages, num_tuples); /* @@ -1618,7 +1625,7 @@ lazy_cleanup_index(Relation indrel, ivinfo.index = indrel; ivinfo.analyze_only = false; - ivinfo.estimated_count = (vacrelstats->scanned_pages < vacrelstats->rel_pages); + ivinfo.estimated_count = (vacrelstats->tupcount_pages < vacrelstats->rel_pages); ivinfo.message_level = elevel; ivinfo.num_heap_tuples = vacrelstats->new_rel_tuples; ivinfo.strategy = vac_strategy; diff --git a/src/test/isolation/expected/vacuum-reltuples.out b/src/test/isolation/expected/vacuum-reltuples.out new file mode 100644 index 0000000000000..ee3adf580509b --- /dev/null +++ b/src/test/isolation/expected/vacuum-reltuples.out @@ -0,0 +1,62 @@ +Parsed test spec with 2 sessions + +starting permutation: modify vac stats +step modify: + insert into smalltbl select max(id)+1 from smalltbl; + delete from smalltbl where id in (select min(id) from smalltbl); + +step vac: + vacuum smalltbl; + +step stats: + select relpages, reltuples from pg_class + where oid='smalltbl'::regclass; + +relpages reltuples + +1 20 + +starting permutation: modify open fetch1 vac close stats +step modify: + insert into smalltbl select max(id)+1 from smalltbl; + delete from smalltbl where id in (select min(id) from smalltbl); + +step open: + begin; + declare c1 cursor for select * from smalltbl; + +step fetch1: + fetch next from c1; + +id + +2 +step vac: + vacuum smalltbl; + +step close: + commit; + +step stats: + select relpages, reltuples from pg_class + where oid='smalltbl'::regclass; + +relpages reltuples + +1 20 + +starting permutation: modify vac stats +step modify: + insert into smalltbl select max(id)+1 from smalltbl; + delete from smalltbl where id in (select min(id) from smalltbl); + +step vac: + vacuum smalltbl; + +step stats: + select relpages, reltuples from pg_class + where oid='smalltbl'::regclass; + +relpages reltuples + +1 20 diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 2606a276247ee..8e404b7a355e8 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -56,4 +56,5 @@ test: alter-table-2 test: alter-table-3 test: create-trigger test: async-notify +test: vacuum-reltuples test: timeouts diff --git a/src/test/isolation/specs/vacuum-reltuples.spec b/src/test/isolation/specs/vacuum-reltuples.spec new file mode 100644 index 0000000000000..52bc405547adb --- /dev/null +++ b/src/test/isolation/specs/vacuum-reltuples.spec @@ -0,0 +1,45 @@ +# Test for vacuum's handling of reltuples when pages are skipped due +# to page pins. We absolutely need to avoid setting reltuples=0 in +# such cases, since that interferes badly with planning. + +setup { + create table smalltbl + as select i as id from generate_series(1,20) i; + alter table smalltbl set (autovacuum_enabled = off); +} +setup { + vacuum analyze smalltbl; +} + +teardown { + drop table smalltbl; +} + +session "worker" +step "open" { + begin; + declare c1 cursor for select * from smalltbl; +} +step "fetch1" { + fetch next from c1; +} +step "close" { + commit; +} +step "stats" { + select relpages, reltuples from pg_class + where oid='smalltbl'::regclass; +} + +session "vacuumer" +step "vac" { + vacuum smalltbl; +} +step "modify" { + insert into smalltbl select max(id)+1 from smalltbl; + delete from smalltbl where id in (select min(id) from smalltbl); +} + +permutation "modify" "vac" "stats" +permutation "modify" "open" "fetch1" "vac" "close" "stats" +permutation "modify" "vac" "stats"