Re: Fwd: [HACKERS] 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.