Checking of the indexes that are on the deletion

[vasiliy-yashkov]

There is a possibility that an index that is on a deletion can be selected by the optimizer and we get "index unexpectedly deleted" error. This is caused by the fact that the index still holds in the index tree and will be deleted on commit.
To reproduce this error:

1. Drop the index in the first transaction. Not to do commit.
2. Set the breakpoint at phase 4 in delete_index (dfw.cpp)
3. Make the commit of the first transaction. The debugger will stop at the breakpoint.
4. In the second transaction execute the query with the index.
5. Continue running the server from the breakpoint.

The optimizer first executes the BTR_all, and then the index is locked, but it's already deleted.

[AlexPeshkoff]

I see the following lines added by your commit:
+// These lists keep information of the deleted indices
+static Firebird::GlobalPtr<Firebird::ArrayFirebird::MetaName > deleted_index_names;
+static Firebird::GlobalPtr<Firebird::ArrayJrd::index_desc > deleted_indices;

The problem with such approach is that it often has problems on classic server. Did you check on classic?

[hvlad]

I also failed to see how it could work on CS.
Usually, to fix this kind of issues, used some kind of [metadata] locking.

[vasiliy-yashkov]

Fixed locking of index on deletion stage via lock manager. Works on super/classic server.

[hvlad]

I don't recommend to apply this patch

[hvlad]

Where is "lock" stored\deleted ? Do we have memory leak there ?

[hvlad]

Why direct call of LM, not LCK_lock() here ?

[hvlad]

Additional call of LM in BTR_all looks like bad idea - it is not needed in 99% of cases and add performance overhead.
Also, i don't understand trick with storing request offset (lck_id) at lock data and double reading of it later in all places.
Also, here we have another leak of "Lock" instance.

Seems, you better should ask in fb-devel how to work with LM properly...

[hvlad]

Why introduce new kind of lock ? Why not use LCK_idx_exists ? Usage of lock data is wrong and not needed, sorry.
In short: add AST handler to the LCK_idx_exists, acquire PW\EX lock on deletion and mark index as deleting in corresponding AST, check mark in BTR_all, clear mark when SR lock is acquired.

PS If you need help understanding how locking in general and metadata locking supposed to work - feel free to ask in fb-devel or contact me directly.

[hvlad]

This attempt is better but still not working :)

1. Every time the SR lock is acquired, idl_deletion flag should be cleared
2. AST allowed to release SR lock if and only if idl_count is zero, else AST should set flag idl_deletion
3. If idl_count > 0 then SR lock must be acquired
4. If idl_count become 0 (after decrement) and if idl_deletion is set, SR lock should be released
5. You should not acquire EX lock on existing instance of idl_lock, use temporary instance instead.
   This temp lock should not have AST handler set
6. AST handler should be set at the same place where idl_lock is created - in CMP_get_index_lock
   (i wonder why this routine is at CMP, IDX or MET is much better place for it, imo)
7. blocking_ast_index should be declared at the same module that used it, currently CMP
8. Consider to convert IndexLock into class and make CMP_get_index_lock() its method and add
   methods to increment\decrement usage counter and post\cancel deletion state.

Probably i missed something, good luck

[vasiliy-yashkov]

Thank you for reply! I need to "digest" this information.
I am confused by fact that when someone tryis to drop index (first stage is passed, but not comitted), and a new user tries to use it, a BTR_descrpiption assert idx->idx_expression != NULL. I'm trying to return a false from it (BTR_description), if there is no way to get SR lock on the index.