Re: Vacuum: allow usage of more than 1GB of work mem

On Tue, Jan 31, 2017 at 3:05 AM, Claudio Freire <klaussfreire(at)gmail(dot)com> wrote:
> On Mon, Jan 30, 2017 at 5:51 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> ----
>> * We are willing to use at most maintenance_work_mem (or perhaps
>> * autovacuum_work_mem) memory space to keep track of dead tuples. We
>> * initially allocate an array of TIDs of that size, with an upper limit that
>> * depends on table size (this limit ensures we don't allocate a huge area
>> * uselessly for vacuuming small tables). If the array threatens to overflow,
>>
>> I think that we need to update the above paragraph comment at top of
>> vacuumlazy.c file.
>
> Indeed, I missed that one. Fixing.
>
>>
>> ----
>> + numtuples = Max(numtuples,
>> MaxHeapTuplesPerPage);
>> + numtuples = Min(numtuples, INT_MAX / 2);
>> + numtuples = Min(numtuples, 2 *
>> pseg->max_dead_tuples);
>> + numtuples = Min(numtuples,
>> MaxAllocSize / sizeof(ItemPointerData));
>> + seg->dt_tids = (ItemPointer)
>> palloc(sizeof(ItemPointerData) * numtuples);
>>
>> Why numtuples is limited to "INT_MAX / 2" but not INT_MAX?
>
> I forgot to mention this one in the OP.
>
> Googling around, I found out some implemetations of bsearch break with
> array sizes beyond INT_MAX/2 [1] (they'd overflow when computing the
> midpoint).
>
> Before this patch, this bsearch call had no way of reaching that size.
> An initial version of the patch (the one that allocated a big array
> with huge allocation) could reach that point, though, so I reduced the
> limit to play it safe. This latest version is back to the starting
> point, since it cannot allocate segments bigger than 1GB, but I opted
> to keep playing it safe and leave the reduced limit just in case.
>

Thanks, I understood.

>> ----
>> @@ -1376,35 +1411,43 @@ lazy_vacuum_heap(Relation onerel, LVRelStats
>> *vacrelstats)
>> pg_rusage_init(&ru0);
>> npages = 0;
>>
>> - tupindex = 0;
>> - while (tupindex < vacrelstats->num_dead_tuples)
>> + segindex = 0;
>> + tottuples = 0;
>> + for (segindex = tupindex = 0; segindex <=
>> vacrelstats->dead_tuples.last_seg; tupindex = 0, segindex++)
>> {
>> - BlockNumber tblk;
>> - Buffer buf;
>> - Page page;
>> - Size freespace;
>>
>> This is a minute thing but tupindex can be define inside of for loop.
>
> Right, changing.
>
>>
>> ----
>> @@ -1129,10 +1159,13 @@ lazy_scan_heap(Relation onerel, int options,
>> LVRelStats *vacrelstats,
>> * instead of doing a second scan.
>> */
>> if (nindexes == 0 &&
>> - vacrelstats->num_dead_tuples > 0)
>> + vacrelstats->dead_tuples.num_entries > 0)
>> {
>> /* Remove tuples from heap */
>> - lazy_vacuum_page(onerel, blkno, buf, 0, vacrelstats, &vmbuffer);
>> + Assert(vacrelstats->dead_tuples.last_seg == 0); /*
>> Should not need more
>> + *
>> than one segment per
>> + * page */
>>
>> I'm not sure we need to add Assert() here but it seems to me that the
>> comment and code is not properly correspond and the comment for
>> Assert() should be wrote above of Assert() line.
>
> Well, that assert is the one that found the second bug in
> lazy_clear_dead_tuples, so clearly it's not without merit.
>
> I'll rearrange the comments as you ask though.
>
>
> Updated and rebased v7 attached.
>
>
> [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=776671

Thank you for updating the patch.

Whole patch looks good to me except for the following one comment.
This is the final comment from me.

/*
* lazy_tid_reaped() -- is a particular tid deletable?
*
* This has the right signature to be an IndexBulkDeleteCallback.
*
* Assumes dead_tuples array is in sorted order.
*/
static bool
lazy_tid_reaped(ItemPointer itemptr, void *state)
{
LVRelStats *vacrelstats = (LVRelStats *) state;

You might want to update the comment of lazy_tid_reaped() as well.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center