On 01/14/2014 03:59 AM, Tom Lane wrote:
> BTW, while I'm looking at this ... the writing side of the code seems a
> few bricks shy of a load too:
>
> /*
> * InHotStandby we need to scan right up to the end of the index for
> * correct locking, so we may need to write a WAL record for the final
> * block in the index if it was not vacuumed. It's possible that VACUUMing
> * has actually removed zeroed pages at the end of the index so we need to
> * take care to issue the record for last actual block and not for the
> * last block that was scanned. Ignore empty indexes.
> */
> if (XLogStandbyInfoActive() &&
> num_pages > 1 && vstate.lastBlockVacuumed < (num_pages - 1))
> {
> Buffer buf;
>
> /*
> * We can't use _bt_getbuf() here because it always applies
> * _bt_checkpage(), which will barf on an all-zero page. We want to
> * recycle all-zero pages, not fail. Also, we want to use a
> * nondefault buffer access strategy.
> */
> buf = ReadBufferExtended(rel, MAIN_FORKNUM, num_pages - 1, RBM_NORMAL,
> info->strategy);
> LockBufferForCleanup(buf);
> _bt_delitems_vacuum(rel, buf, NULL, 0, vstate.lastBlockVacuumed);
> _bt_relbuf(rel, buf);
> }
>
> If the last physical page of the index is all-zero, the preceding loop
> won't have any problem with that (nor should it); but this code sure will
> have a problem, because _bt_delitems_vacuum isn't prepared to cope with an
> all-zero page AFAICS.
Yep, that's broken.
> Nor am I following the logic of the initial comment. If we know that
> pages above N are all-zero, why are we worried about taking locks on them?
> There shouldn't be any scans reaching such pages, and any that did would
> error out anyhow in _bt_getbuf().
Also, btree relations are never truncated, so the comment is wrong when
it claims that VACUUM might've removed zero pages at the end of the index.
> ISTM the right thing here is for the btvacuumpage loop to remember the
> last ordinary, valid index page it saw, and point to that one in this
> added WAL entry.
Agreed.
- Heikki