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

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.