cruppstahl / upscaledb

A very fast lightweight embedded database engine with a built-in query language.
https://upscaledb.com
Apache License 2.0
568 stars 69 forks source link

Unexpected cursor behaviour with UPS_ENABLE_TRANSACTIONS #101

Closed Amadeszueusz closed 6 years ago

Amadeszueusz commented 6 years ago

Please look at the sample:

int main()
{
    ups_env_t* env;
    //ups_env_create(&env, "test.db", UPS_ENABLE_TRANSACTIONS, 0664, 0);
    ups_env_create(&env, "test.db", 0, 0664, 0);

    ups_parameter_t params[] = {
    {UPS_PARAM_KEY_TYPE, UPS_TYPE_UINT32},
    {0, }
    };

    ups_db_t* db;
    ups_env_create_db(env, &db, 1, 0, &params[0]);

    for (int i = 0; i < 4; i++)
    {
        ups_key_t key = ups_make_key(&i, sizeof(i));
        ups_record_t record = {0};

        ups_db_insert(db, 0, &key, &record, 0);
    }

    ups_cursor_t* cur;
    ups_cursor_create(&cur, db, 0, 0);

    ups_key_t cur_key;

    int key_val = -1;

    ups_cursor_move(cur, &cur_key, 0, UPS_CURSOR_LAST);
    std::cout << (key_val = *(int*)cur_key.data) << std::endl;
    ups_cursor_move(cur, &cur_key, 0, UPS_CURSOR_NEXT);
    std::cout << (key_val = *(int*)cur_key.data) << std::endl;
    ups_cursor_move(cur, &cur_key, 0, UPS_CURSOR_PREVIOUS);
    std::cout << (key_val = *(int*)cur_key.data) << std::endl;

    return 0;
}

With transactions disabled in standard output I read: 3 3 2

With transactions enabled in standard output I read: 3 3 3

cruppstahl commented 6 years ago

The second call to ups_cursor_move fails b/c you've reached the end of the database. If you remove this call then the weird behaviour disappears.

Amadeszueusz commented 6 years ago

I expected second ups_cursor_move call to fail. I also expected that cursor would not invalidate but stay in place, so it will be reusable. (in third call) API documentation about ups_cursor_find states that " If the item could not be found, the Cursor is not modified.". API documentation of ups_cursor_move doesn't adress this case, but I expected that when key cannot be found cursor won't be modified also.

In case of enviornment with disabled transaction cursor appears not beeing modified after second call. In case of enviornment with enabled transaction cursor also appears not beeing modified after second call, but third call of ups_cursor_move doesn't move cursor on the expected position.

So when ups_cursor_move returns UPS_KEY_NOT_FOUND I should assume that cursor is invalid and shouldn't be used anymore?

cruppstahl commented 6 years ago

What you discovered is clearly a bug. The behaviour should be identical, regardless whether transactions are enabled or not. And the cursor must not be invalidated if a key is not found. I'll try to work on a fix asap, but please don't expect it before the weekend of Dec 16...

2017-12-07 22:06 GMT+01:00 Amadeusz Serafin notifications@github.com:

I expected second ups_cursor_move call to fail. I also expected that cursor would not invalidate but stay in place, so it will be reusable. (in third call) API documentation about ups_cursor_find states that " If the item could not be found, the Cursor is not modified.". API documentation of ups_cursor_move doesn't adress this case, but I expected that when key cannot be found cursor won't be modified also.

In case of enviornment with disabled transaction cursor appears not beeing modified after second call. In case of enviornment with enabled transaction cursor also appears not beeing modified after second call, but third call of ups_cursor_move doesn't move cursor on the expected position.

So when ups_cursor_move returns UPS_KEY_NOT_FOUND I should assume that cursor is invalid?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cruppstahl/upscaledb/issues/101#issuecomment-350094271, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMxgwMeh7bAlMY4cNpXgDvpwSEGv079ks5s-FNOgaJpZM4Q34Tu .

cruppstahl commented 6 years ago

This should be fixed now; if not then please reopen.

Amadeszueusz commented 6 years ago

@cruppstahl I have tested this case on database containing duplicates:

int main()
{
    ups_env_t* env;
    ups_env_create(&env, "test.db", UPS_ENABLE_TRANSACTIONS, 0664, 0);
    //ups_env_create(&env, "test.db", 0, 0664, 0);

    ups_parameter_t params[] = {
    {UPS_PARAM_KEY_TYPE, UPS_TYPE_UINT32},
    {0, }
    };

    ups_db_t* db;
    ups_env_create_db(env, &db, 1, UPS_ENABLE_DUPLICATES, &params[0]);

    int i = 0;
    for (; i < 4; i++)
    {
        ups_key_t key = ups_make_key(&i, sizeof(i));
        ups_record_t record = {0};

        ups_db_insert(db, 0, &key, &record, 0);
    }
    i = 3;

    ups_key_t key = ups_make_key(&i, sizeof(i));
    ups_record_t record = {0};
    ups_db_insert(db, 0, &key, &record, UPS_DUPLICATE);

    ups_cursor_t* cur;
    ups_cursor_create(&cur, db, 0, 0);

    ups_key_t cur_key;

    int key_val = -1;

    ups_status_t st;

    ups_cursor_move(cur, &cur_key, 0, UPS_CURSOR_LAST);
    std::cout << (key_val = *(int*)cur_key.data) << std::endl;
    ups_cursor_move(cur, &cur_key, 0, UPS_CURSOR_NEXT);
    std::cout << (key_val = *(int*)cur_key.data) << std::endl;
    ups_cursor_move(cur, &cur_key, 0, UPS_CURSOR_PREVIOUS);
    std::cout << (key_val = *(int*)cur_key.data) << std::endl;

    return 0;
}

With transactions enabled cursor call with UPS_CURSOR_PREVIOUS returns 2 instead of 3.

cruppstahl commented 6 years ago

Should be fixed now, thanks for reporting

cruppstahl commented 6 years ago

That sounds inconsistent. I'll look into it over the weekend.