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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
