Re: Fwd: [HACKERS] Vacuum: allow usage of more than 1GB of work mem - Mailing list pgsql-hackers
From | Alexey Chernyshov |
---|---|
Subject | Re: Fwd: [HACKERS] Vacuum: allow usage of more than 1GB of work mem |
Date | |
Msg-id | 0caf061e-2075-934a-130f-61fc149395f6@postgrespro.ru Whole thread Raw |
In response to | Fwd: [HACKERS] Vacuum: allow usage of more than 1GB of work mem (Claudio Freire <klaussfreire@gmail.com>) |
Responses |
Re: Fwd: [HACKERS] Vacuum: allow usage of more than 1GB of work mem
(Claudio Freire <klaussfreire@gmail.com>)
|
List | pgsql-hackers |
Thank you for the patch and benchmark results, I have a couple remarks. Firstly, padding in DeadTuplesSegment typedef struct DeadTuplesSegment { ItemPointerData last_dead_tuple; /* Copy of the last dead tuple (unset * until the segment is fully * populated). Keep it first to simplify * binary searches */ unsigned short padding; /* Align dt_tids to 32-bits, * sizeof(ItemPointerData) is aligned to * short, so add a padding short, to make the * size of DeadTuplesSegment a multiple of * 32-bits and align integer components for * better performance during lookups into the * multiarray */ int num_dead_tuples; /* # of entries in the segment */ int max_dead_tuples; /* # of entries allocated in the segment */ ItemPointer dt_tids; /* Array of dead tuples */ } DeadTuplesSegment; In the comments to ItemPointerData is written that it is 6 bytes long, but can be padded to 8 bytes by some compilers, so if we add padding in a current way, there is no guaranty that it will be done as it is expected. The other way to do it with pg_attribute_alligned. But in my opinion, there is no need to do it manually, because the compiler will do this optimization itself. On 11.07.2017 19:51, Claudio Freire wrote: >> ----- >> >> + /* Search for the segment likely to contain the item pointer */ >> + iseg = vac_itemptr_binsrch( >> + (void *) itemptr, >> + (void *) >> &(vacrelstats->dead_tuples.dt_segments->last_dead_tuple), >> + vacrelstats->dead_tuples.last_seg + 1, >> + sizeof(DeadTuplesSegment)); >> + >> >> I think that we can change the above to; >> >> + /* Search for the segment likely to contain the item pointer */ >> + iseg = vac_itemptr_binsrch( >> + (void *) itemptr, >> + (void *) &(seg->last_dead_tuple), >> + vacrelstats->dead_tuples.last_seg + 1, >> + sizeof(DeadTuplesSegment)); >> >> We set "seg = vacrelstats->dead_tuples.dt_segments" just before this. > Right In my mind, if you change vacrelstats->dead_tuples.last_seg + 1 with GetNumDeadTuplesSegments(vacrelstats), it would be more meaningful. Besides, you can change the vac_itemptr_binsrch within the segment with stdlib bsearch, like: res = (ItemPointer) bsearch((void *) itemptr, (void *) seg->dt_tids, seg->num_dead_tuples, sizeof(ItemPointerData), vac_cmp_itemptr); return (res != NULL); > Those are before the changes in the review. While I don't expect any > change, I'll re-run some of them just in case, and try to investigate > the slowdown. But that will take forever. Each run takes about a week > on my test rig, and I don't have enough hardware to parallelize the > tests. I will run a test on a snapshot of a particularly troublesome > production database we have, that should be interesting. Very interesting, waiting for the results.
pgsql-hackers by date: