On 2013-11-27 14:45:25 +0200, Heikki Linnakangas wrote:
> On 11/27/13 14:11, Andres Freund wrote:
> >I don't think this warning is likely to be hit as the code stands -
> >heap_page_prune() et. al. will have removed all dead tuples already,
> >right and so has_dead_tuples won't be set.
>
> It might be a good idea to add such a warning to heap_page_prune(). Or also
> emit the warning in lazy_scan_heap() if heap_page_prune() returned > 0.
> >Independent from this, ISTM we should add a
> > else if (PageIsAllVisible(page) && all_visible)
> >to those checks.
>
> Can you elaborate, where should that be added?
I was thinking of adding such a warning below elog(WARNING, "page containing dead tuples is marked as all-visible in
relation\"%s\" page %u",..)
but cannot warn against that because GetOldestXmin() can go backwards...
I think it's probably sufficient to set has_dead_tuples = true in the
ItemIdIsDead() branch in lazy_scan_heap(). That should catch relevant
actions from heap_page_prune().
Besides not warning in against deletions from heap_page_prune(), the
current warning logic is also buggy for tables without indexes...
/* * If there are no indexes then we can vacuum the page right now * instead of doing a second
scan. */ if (nindexes == 0 && vacrelstats->num_dead_tuples > 0) { /* Remove
tuplesfrom heap */ lazy_vacuum_page(onerel, blkno, buf, 0, vacrelstats, &vmbuffer); has_dead_tuples
=false;
That happens before the else if (PageIsAllVisible(page) && has_dead_tuples)
check.
With regard to fixing things up, ISTM the best bet is heap_prune_chain()
so far. That's executed b vacuum and by opportunistic pruning and we
know we have the appropriate locks there. Looks relatively easy to fix
up things there. Not sure if there are any possible routes to WAL log
this but using log_newpage()?
I am really not sure what the best course of action is :(
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
