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

On Wed, Jan 25, 2017 at 1:54 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> Thank you for updating the patch!
>
> +       /*
> +        * Quickly rule out by lower bound (should happen a lot) Upper bound was
> +        * already checked by segment search
> +        */
> +       if (vac_cmp_itemptr((void *) itemptr, (void *) rseg->dead_tuples) < 0)
> +               return false;
>
> I think that if the above result is 0, we can return true as itemptr
> matched lower bound item pointer in rseg->dead_tuples.

That's right. Possibly not a great speedup but... why not?

>
>  +typedef struct DeadTuplesSegment
> +{
> +       int                     num_dead_tuples;        /* # of
> entries in the segment */
> +       int                     max_dead_tuples;        /* # of
> entries allocated in the segment */
> +       ItemPointerData last_dead_tuple;        /* Copy of the last
> dead tuple (unset
> +
>           * until the segment is fully
> +
>           * populated) */
> +       unsigned short padding;
> +       ItemPointer dead_tuples;        /* Array of dead tuples */
> +}      DeadTuplesSegment;
> +
> +typedef struct DeadTuplesMultiArray
> +{
> +       int                     num_entries;    /* current # of entries */
> +       int                     max_entries;    /* total # of slots
> that can be allocated in
> +                                                                * array */
> +       int                     num_segs;               /* number of
> dead tuple segments allocated */
> +       int                     last_seg;               /* last dead
> tuple segment with data (or 0) */
> +       DeadTuplesSegment *dead_tuples;         /* array of num_segs segments */
> +}      DeadTuplesMultiArray;
>
> It's a matter of personal preference but some same dead_tuples
> variables having different meaning confused me.
> If we want to access first dead tuple location of first segment, we
> need to do 'vacrelstats->dead_tuples.dead_tuples.dead_tuples'. For
> example, 'vacrelstats->dead_tuples.dt_segment.dt_array' is better to
> me.

Yes, I can see how that could be confusing.

I went for vacrelstats->dead_tuples.dt_segments[i].dt_tids[j]

> +                                       nseg->num_dead_tuples = 0;
> +                                       nseg->max_dead_tuples = 0;
> +                                       nseg->dead_tuples = NULL;
> +                                       vacrelstats->dead_tuples.num_segs++;
> +                               }
> +                               seg = DeadTuplesCurrentSegment(vacrelstats);
> +                       }
> +                       vacrelstats->dead_tuples.last_seg++;
> +                       seg = DeadTuplesCurrentSegment(vacrelstats);
>
> Because seg is always set later I think the first line starting with
> "seg = ..." is not necessary. Thought?

That's correct.

Attached a v6 with those changes (and rebased).

Make check still passes.