Re: (ITS#8117) Bugs related to key-size in lmdb and backend

[h.b.furuseth@usit.uio.no]

On 03/05/15 09:09, Howard Chu wrote:
> Hallvard Breien Furuseth wrote:
>> test001 still crashes if you insert
>>      assert(a->mv_size == sizeof(int) && b->mv_size == sizeof(int));
>> in mdb_cmp_int().
>> Using RE24 commit 98f70f1841f85d93 "ITS#8120 Move final...".
>
> Not happening here, all tests pass with asserts enabled.

You just fixed it.  Turns out your older patches to this ITS
introduced that one.

>> Looking yet again at the doc, mdb_dbi_open():INTEGERKEY
>> says "typically" sizeof(int/size_t), I can't see it
>> requires these sizes.  Maybe you copied that from a
>> suggestion from me back when I asked for clarifications.
>
> Probably. The point was to show the expected sizes without enumerating
> every possible C integer type that would work - e.g. unsigned int,
> unsigned long, off_t, pid_t, whatever. At some point you have to say
> "don't be an idiot."

Don't be an idiot.  (a) then we'd just say "the architecture's native
unsigned integer types" or something, it's no need to be so vague nor
to drag in every typedef under the sun.  And (b) that's not what the
code does anyway.  If I understand correctly this time, the doc should
simply say that the two types size_t and unsigned int are supported.
The "typically" is plain misleading.

That is, if mdb_node_search() shows the intent when it picks either
int-sized or size_t-sized for IS_BRANCH.  OTOH, another bug:

default_cmp pick cmp_int for INTEGERDUP|DUPFIXED and some other
replaces md_dcmp = cmp_int with cmp_long, which makes no sense unless
cmp_int is just a placeholder.  Yet at least mdb_dcmp() calls that
placeholder directly without checking the data size.  And if it's
just a placeholder, how about instead setting dcmp to something which
just abort()s, to make sure the bug of not replacing it is caught?

>> "keys must all be of the same size" in the doc is unclear.
>> I gather it should be "... in the DB's lifetime".  But it
>> can be read as "...at a given time": Write size_t-sized
>> integers, empty the DB, then write int-sized integers.
>> That does not work, since me_dbxs[].md_dcmp is updated.
>
> Don't be an idiot. If no time is stated, then it's For All Time.
> Anything not called out specifically is, by definition, non-specific.

I can't spot the idiocy of expecting a property which springs
dynamically into life, to dynamically go away when the data which
caused it is gone.  In particular since that's how LMDB alignment works.
Anyway, that seems to be what happens after latest commit,
so maybe it doesn't matter anymore.

Maybe the DB flags should include log2(integer sizes) in 2 2-bit fields.