Issue 6105 - lmdb - Cannot create entries with long rdn #6130
Conversation
This fix is split in two commits:
Part 1 refactorize the entryrdn static subfunctions parameters
Part 2 implement the use of a redirect database file
in two commits because the first part has a big diff
but it is quite straightforward as it only refactorize
the set of parameters used by the entryrdn static subfunctions
to rather use a single parameter (A single context struct
containing all the parameters needed to access the database
(like the backend, the database instances, the txn and the cursor )
The benefit are:
- avoid having too much parameters in sub functions
(especially for the second part of the fix that implements a second db to
handle the entryrdn)
- avoid duplicating the retry loops to open/close the cursor
- IMHO it made the code clearer
|
FYI: To the reviewer: I split the changes in two commit: |
|
Fixed the typo and the missing test description. |
| key.mv_size = sizeof (*smallkey); | ||
| rc = MDB_CURSOR_GET(db->cursor, &key, &data, MDB_SET_RANGE); | ||
| /* Let walk all the keys that have the right hash */ | ||
| while (rc == 0 && key.mv_size == sizeof(privdb_small_key_t) && |
There was a problem hiding this comment.
Hi @progier389 , sorry for the late question. I am a bit puzzled, I assume this loop is to handle duplicate (small) keys. Correct ?
My understanding is that a database handling duplicate keys have values with limited size. So how the longkey could fit in a value of such database ?
There was a problem hiding this comment.
The small keys looks like "==>HaveValue:Count"
The goal of the loop is to get the highest Count for a given hash value (to handle the collisions)
| key2.mv_size = sizeof small_key; | ||
| rc = dbmdb_privdb_init_small_key(db, key, 0, &small_key); | ||
| if (!rc) { | ||
| rc = MDB_CURSOR_GET(db->cursor, &key2, data, MDB_SET_KEY); |
There was a problem hiding this comment.
I am surprise it uses the same db->cursor. Depending on the size of the key, is not it using a different database/cursor. Is db->cursor already set to the appropriate DB ?
There was a problem hiding this comment.
Note: that there are two different mechanism
[1] For entryrdn (in this case, AFAIK there are two database)
Entryrdn: small-key --> long-data
is replaced by:
Entryrdn: small-key --> redirect-key:
@longentryrdn: redirect-key: long-data
[2] For the import temporary private db (which is the case here)
privdb: long-key --> data
is replaced by:
small-key1 --> data
small-key2 --> long-key (smallkey1 and smallkey2 are the same except for the first characters (= versus @)
So yes there is only 1 db (and 1 single cursor) involved in this case
* Issue 6105 - lmdb - add fails if rdn is longer than 250 bytes - Part 1
This fix is split in two commits:
Part 1 refactorize the entryrdn static subfunctions parameters
Part 2 implement the use of a redirect database file
in two commits because the first part has a big diff
but it is quite straightforward as it only refactorize the set of parameters used by the entryrdn static subfunctions
to rather use a single parameter (A single context struct containing all the parameters needed to access the
database (like the backend, the database instances, the txn and the cursor )
The benefit are:
- avoid having too much parameters in sub functions
especially for the second part of the fix that implements a second db to handle the entryrdn
- avoid duplicating the retry loops to open/close the cursor
- IMHO it made the code clearer
* Issue 6105 - lmdb - Cannot create entries with long rdn
- the use of a redirect database file
- the use of redirect link with the private database used by import to build the dn/rdn/ancestor relationship
- the CI testcase
* Issue 6105 - lmdb - Cannot create entries with long rdn - review feedback
- fix some comments
- improve the CI tests by adding children to an ou with long rdn then renaming it.
* Issue 6105 - lmdb - add fails if rdn is longer than 250 bytes - Part 1
This fix is split in two commits:
Part 1 refactorize the entryrdn static subfunctions parameters
Part 2 implement the use of a redirect database file
in two commits because the first part has a big diff
but it is quite straightforward as it only refactorize the set of parameters used by the entryrdn static subfunctions
to rather use a single parameter (A single context struct containing all the parameters needed to access the
database (like the backend, the database instances, the txn and the cursor )
The benefit are:
- avoid having too much parameters in sub functions
especially for the second part of the fix that implements a second db to handle the entryrdn
- avoid duplicating the retry loops to open/close the cursor
- IMHO it made the code clearer
* Issue 6105 - lmdb - Cannot create entries with long rdn
- the use of a redirect database file
- the use of redirect link with the private database used by import to build the dn/rdn/ancestor relationship
- the CI testcase
* Issue 6105 - lmdb - Cannot create entries with long rdn - review feedback
- fix some comments
- improve the CI tests by adding children to an ou with long rdn then renaming it.
(cherry picked from commit b551b18)
The issue is that entryrdn.db does not support data longer than 511 bytes and same issue about the internal db keys used for the import/reindex pipeline.
Solution for entryrdn. I created a dbi that does not support duplicate keys (but it supports long data)
and replaced the long rdn_elem by a "redirect" rdn_elem that contains the key to the long data in @long-entryrdn.db
As entryrdn code is tricky, I have first done some cleanup by using a context as parameter rather than several parameters
(because the parameter list was getting quite large for some functions) and added new functions to factorize the open/close for the context (and the cursor open/close) retry loop)
I also improved the debug features because I need it !
About the import internal db/ I create redirect key that refer to records whose data contains the real key
Issue #6105
Reviewed by: @droideck