Re: optimizing vacuum truncation scans

On Mon, Jun 29, 2015 at 11:24 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>
>
> Attached is a patch that implements the vm scan for truncation. It
introduces a variable to hold the last blkno which was skipped during the
forward portion. Any blocks after both this blkno and after the last
inspected nonempty page (which the code is already tracking) must have been
observed to be empty by the current vacuum. Any other process rendering
the page nonempty are required to clear the vm bit, and no other process
can set the bit again during the vacuum's lifetime. So if the bit is still
set, the page is still empty without needing to inspect it.
>

One case where this patch can behave differently then current
code is, when before taking Exclusive lock on relation for truncation,
if some backend clears the vm bit and then inserts-deletes-prune that
page. I think with patch it will not consider to truncate pages whereas
current code will allow to truncate it as it re-verifies each page. I think
such a case would be rare and we might not want to bother about it,
but still worth to think about it once.

Some minor points about patch:

count_nondeletable_pages()
{
..
if (PageIsNew(page) || PageIsEmpty(page))
{
/* PageIsNew probably shouldn't happen... */
UnlockReleaseBuffer(buf);
continue;
}
..
}

Why vmbuffer is not freed in above loop?

count_nondeletable_pages()
{
..
+ /*
+ * Pages that were inspected and found to be empty by this vacuum run
+ * must still be empty if the vm bit is still set. Only vacuum sets
+ * vm bits, and only one vacuum can exist in a table at one time.
+ */
+ trust_vm=vacrelstats->nonempty_pages>vacrelstats->skipped_pages ?
+ vacrelstats->nonempty_pages : vacrelstats->skipped_pages;

..
}

I think it is better to have spaces in above assignment statement
(near '=' and near '>')

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com