Re: [PATCH v1] Batch B-tree TIDs when building a bitmap

> The hash path still appears to use the old pattern, though:
>
> while (so->currPos.itemIndex <= so->currPos.lastItem)
> tids[ntids_page++] =
> so->currPos.items[so->currPos.itemIndex++].heapTid;
>
> I meant applying the same idea there as in B-tree: iterate with a local
> index and update itemIndex once after copying the page.

Changed.

> On the small B-tree regression, I agree that the absolute difference
> is tiny. I measured it with a release build using 24 paired runs of
> the baseline and patched servers. The medians were:
>
> baseline 33.195 us
> patched 33.569 us
>
> The median paired increase was 0.283 us, and the patched version was
> slower in 19 of 24 pairs. The paired 95% interval for the relative
> difference was +0.55% to +1.28%.
>
> So it was repeatable on that machine, rather than a single timing
> sample, but I agree that attributing such a small difference
> specifically to the TID copy would be too strong. Since you don't
> reproduce it independently, I don't think it is actionable or a reason
> to change the patch.

Agreed.

> Thanks also for adding the other AMs to the benchmark. Those results
> address my concern about extending the optimization beyond B-tree. In
> particular, the GIN, GiST, hash, and SP-GiST results show useful
> Bitmap Index Scan savings even though their result streams have
> different locality characteristics.

It was useful not only as confirmation but it also made me find the
bloom residual-batch issue. So time well spent.

> And yes, I wasn't suggesting dynamic allocation for the GiST/SP-GiST
> buffers. Looking at their sizes again, I agree that fixed buffers are
> preferable to adding allocation/lifetime overhead.

OK.

> With the corresponding local-index cleanup in hash, the patch looks
> good to me.
Attached is v4.

The only change is using a local counter in hashgetbitmap(), instead of
updating so->currPos.itemIndex every loop iteration to save on
load/store instructions. Now the code is consistent with the btgetbitmap().

I also rebased on master.

--
David Geier