|From:||Claudio Freire <klaussfreire(at)gmail(dot)com>|
|To:||Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>|
|Cc:||Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru>, Anastasia Lubennikova <lubennikovaav(at)gmail(dot)com>, PostgreSQL-Dev <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Vacuum: allow usage of more than 1GB of work mem|
|Views:||Raw Message | Whole Thread | Download mbox|
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,
> + numtuples = Min(numtuples, INT_MAX / 2);
> + numtuples = Min(numtuples, 2 *
> + 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  (they'd overflow when computing the
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.
> @@ -1376,35 +1411,43 @@ lazy_vacuum_heap(Relation onerel, LVRelStats
> 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.
> @@ -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.
|Next Message||Michael Paquier||2017-01-31 02:07:10||Re: Potential data loss of 2PC files|
|Previous Message||Stephen Frost||2017-01-31 01:48:32||Fix handling of ALTER DEFAULT PRIVILEGES|