Re: PATCH: two slab-like memory allocators

On 11/27/2016 07:25 PM, Petr Jelinek wrote:
> On 15/11/16 01:44, Tomas Vondra wrote:
>> Attached is v6 of the patch series, fixing most of the points:
>>
>> * common bits (valgrind/randomization/wipe) moved to memdebug.h/c
>>
>> Instead of introducing a new header file, I've added the prototypes to
>> memdebug.h (which was already used for the valgrind stuff anyway), and
>> the implementations to a new memdebug.c file. Not sure what you meant by
>> "static inlines" though.
>
> I think Andres wanted to put the implementation to the static inline
> functions directly in the header (see parts of pg_list or how atomics
> work), but I guess you way works too.
>

I see. Well turning that into static inlines just like in pg_list is 
possible. I guess the main reason is performance - for pg_list that 
probably makes sense, but the memory randomization/valgrind stuff is 
only ever used for debugging, which already does a lot of expensive 
stuff anyway.

>>
>> I've however kept SlabContext->freelist as an array, because there may
>> be many blocks with the same number of free chunks, in which case moving
>> the block in the list would be expensive. This way it's simply
>> dlist_delete + dlist_push.
>>
>
> +1
>
> I get mxact isolation test failures in test_decoding with this version
> of patch:
>   step s0w: INSERT INTO do_write DEFAULT VALUES;
> + WARNING:  problem in slab TXN: number of free chunks 33 in block
> 0x22beba0 does not match bitmap 34
>   step s0start: SELECT data FROM
> pg_logical_slot_get_changes('isolation_slot', NULL, NULL,
> 'include-xids', 'false');
>   data
> and
>   step s0alter: ALTER TABLE do_write ADD column ts timestamptz;
>   step s0w: INSERT INTO do_write DEFAULT VALUES;
> + WARNING:  problem in slab TXN: number of free chunks 33 in block
> 0x227c3f0 does not match bitmap 34
>   step s0start: SELECT data FROM
> pg_logical_slot_get_changes('isolation_slot', NULL, NULL,
> 'include-xids', 'false');
>   data
>

D'oh! I believe this is a simple thinko in SlabCheck, which iterates 
over chunks like this:
    for (j = 0; j <= slab->chunksPerBlock; j++)    ...

which is of course off-by-one error (and the 33 vs. 34 error message is 
consistent with this theory).

>
> Also, let's just rename the Gen to Generation.
>

OK.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services