Re: Bug in VACUUM reporting of "removed %d row versions" in 9.2+

Where are we on this?

---------------------------------------------------------------------------

On Fri, May 10, 2013 at 04:37:58PM +0100, Simon Riggs wrote:
> Commit d0dcb315db0043f10073a9a244cea138e9e60edd and previous
> introduced a bug into the reporting of removed row versions. ('Twas
> myself et al, before you ask).
>
> In those commits, lazy_vacuum_heap() skipped pinned blocks, but then
> failed to report that accurately, claiming that the tuples were
> actually removed when they were not. That bug has masked the effect of
> the page skipping behaviour.
>
> Bug is in 9.2 and HEAD.
>
> Attached patch corrects that, with logic to move to the next block
> rather than re-try the lock in a tight loop once per tuple, which was
> mostly ineffective.
>
> Attached patch also changes the algorithm slightly to retry a skipped
> block by sleeping and then retrying the block, following observation
> of the effects of the current skipping algorithm once skipped rows are
> correctly reported.
>
> It also adds a comment which explains the skipping behaviour.
>
> Viewpoints?
>
> --
> Simon Riggs http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services

> diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
> index 02f3cf3..f0d054a 100644
> --- a/src/backend/commands/vacuumlazy.c
> +++ b/src/backend/commands/vacuumlazy.c
> @@ -1052,15 +1052,15 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
> static void
> lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
> {
> - int tupindex;
> - int npages;
> + int tupindex = 0;
> + int npages = 0;
> + int ntupskipped = 0;
> + int npagesskipped = 0;
> PGRUsage ru0;
> Buffer vmbuffer = InvalidBuffer;
>
> pg_rusage_init(&ru0);
> - npages = 0;
>
> - tupindex = 0;
> while (tupindex < vacrelstats->num_dead_tuples)
> {
> BlockNumber tblk;
> @@ -1075,9 +1075,32 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
> vac_strategy);
> if (!ConditionalLockBufferForCleanup(buf))
> {
> - ReleaseBuffer(buf);
> - ++tupindex;
> - continue;
> + /*
> + * If we can't get the lock, sleep, then try again just once.
> + *
> + * If we can't get the lock the second time, skip this block and
> + * move onto the next one. This is possible because by now we
> + * know the tuples are dead and all index pointers to them have been
> + * removed, so it is safe to ignore them, even if not ideal.
> + */
> + VacuumCostBalance += VacuumCostLimit;
> + vacuum_delay_point();
> + if (!ConditionalLockBufferForCleanup(buf))
> + {
> + BlockNumber blkno = tblk;
> +
> + ReleaseBuffer(buf);
> + tupindex++;
> + for (; tupindex < vacrelstats->num_dead_tuples; tupindex++)
> + {
> + ntupskipped++;
> + tblk = ItemPointerGetBlockNumber(&vacrelstats->dead_tuples[tupindex]);
> + if (tblk != blkno)
> + break;
> + }
> + npagesskipped++;
> + continue;
> + }
> }
> tupindex = lazy_vacuum_page(onerel, tblk, buf, tupindex, vacrelstats,
> &vmbuffer);
> @@ -1098,9 +1121,9 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
> }
>
> ereport(elevel,
> - (errmsg("\"%s\": removed %d row versions in %d pages",
> + (errmsg("\"%s\": removed %d row versions in %d pages (skipped %d row versions in %d pages)",
> RelationGetRelationName(onerel),
> - tupindex, npages),
> + tupindex - ntupskipped, npages, ntupskipped, npagesskipped),
> errdetail("%s.",
> pg_rusage_show(&ru0))));
> }

>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +