On 23/08/2019 11:44, Masahiko Sawada wrote:
> On Fri, Aug 23, 2019 at 7:05 AM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
>>
>> On Thu, Aug 22, 2019 at 1:14 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>> The patch also includes a little unit test module to test this without
>>> creating a 16 TB table. A whole new test module seems a bit like
>>> overkill just for this, but clearly we were missing test coverage here.
>>> And it will come handy, if we want to invent a new better posting list
>>> format in the future. Thoughts on whether to include the test module or not?
>>
>>
>> I like the test as importantly adds missing coverage. Also, really simplifies validation effort if required to make
changein this area anytime in future. So, I would +1 keeping the same.
>
> I'd +1 too. It's valuable to test hard-to-reproduce case. I often want
> to do such unit tests with more cheaper costs, though.
Ok, including it seems to be the consensus.
> BTW it's not related to this patch but I got confused that where "17
> bits" of the following paragraph in ginpostinglist.c comes from. If we
> use only 43 bit out of 64-bit unsigned integer we have 21 bits left.
>
> * For encoding purposes, item pointers are represented as 64-bit unsigned
> * integers. The lowest 11 bits represent the offset number, and the next
> * lowest 32 bits are the block number. That leaves 17 bits unused, i.e.
> * only 43 low bits are used.
Huh, you're right. I think it should be "leaves 21 bits unused". I
suspect the patch used 15 bits for the offset number early in the
development, which would've left 17 bits unused, and when it was later
changed to use only 11 bits, that comment was neglected.
Fixed that comment, too, and pushed. Thanks!
- Heikki