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

On Tue, Jan 31, 2017 at 3:05 AM, Claudio Freire <klaussfreire@gmail.com> wrote:
> On Mon, Jan 30, 2017 at 5:51 AM, Masahiko Sawada <sawada.mshk@gmail.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