Re: optimizing vacuum truncation scans

On Wed, Jul 15, 2015 at 11:19 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> 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.

Thanks for your review.

corrected the code as instead of returning the blkno after visibility map
check failure, the code just continue to verify the contents as
earlier approach.

> 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?
>

In the above check, we are just continuing to the next blkno, at the
end of the loop the vmbuffer
is freed.

> 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 '>')
>

corrected.

Here I attached updated patches,
1) without prefetch logic.
2) with combined vm and prefetch logic.

I marked the patch as ready for committer.

Regards,
Hari Babu
Fujitsu Australia