Re: Proposed patch for 8.3 VACUUM FULL crash

Tomas Szepe reported here
http://archives.postgresql.org/pgsql-bugs/2008-02/msg00068.php
about a bug in VACUUM FULL, which turned out to be that the code
was expecting pages with no live tuples to always get added to
the fraged_pages list, and this was sometimes not happening.

On investigation the problem occurs because we changed vacuum.c's
PageGetFreeSpaceWithFillFactor() to use PageGetHeapFreeSpace()
instead of just computing pd_upper - pd_lower as it had done in
every previous release. This was *not* a good idea: VACUUM FULL
does its own accounting for line pointers and does not need "help".
Aside from exposing the aforementioned bug, this would result in
pages sometimes being passed over as not being useful insertion
targets, when in fact they were going to be completely empty after
the "reaping" step.

The attached proposed patch reverts that bad decision, and also adds an
extra condition to force pages into the fraged_pages list when notup is
true. Under normal circumstances this extra condition should be a
useless test, but I am worried that with extremely small fillfactor
settings it might still be possible to provoke the empty_end_pages
problem.

I am thinking that the extra notup condition should be back-patched,
at least as far back as we have fillfactor support, and maybe all
the way back on general principles.

Comments?

regards, tom lane

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> On investigation the problem occurs because we changed vacuum.c's
> PageGetFreeSpaceWithFillFactor() to use PageGetHeapFreeSpace()
> instead of just computing pd_upper - pd_lower as it had done in
> every previous release. This was *not* a good idea: VACUUM FULL
> does its own accounting for line pointers and does not need "help".

Fwiw this change appears to have crept in when the patch was merged.
Ironically while most of us have been complaining about patches not being
widely visible and tested outside of CVS in this case we perhaps suffered from
the opposite problem. The patch was fairly heavily tested on this end before
it was posted and I'm not sure those tests have been repeated since the merge.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's 24x7 Postgres support!

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> On investigation the problem occurs because we changed vacuum.c's
>> PageGetFreeSpaceWithFillFactor() to use PageGetHeapFreeSpace()
>> instead of just computing pd_upper - pd_lower as it had done in
>> every previous release. This was *not* a good idea: VACUUM FULL
>> does its own accounting for line pointers and does not need "help".

> Fwiw this change appears to have crept in when the patch was merged.

Yeah, it's entirely likely that this was my fault :-(. It wasn't
apparent at the time that this change wasn't safe and conservative...

regards, tom lane