[GAIAPLAT-1749] INDEX: Keep track of updated txn id, mark committed txn ids and skip if no work to be done #1121
Conversation
production/db/inc/index/index.hpp
Outdated
| @@ -89,6 +90,9 @@ class index_t : public base_index_t | |||
| T_structure m_data; | |||
|
|
|||
| private: | |||
| gaia_txn_id_t m_last_updated_txn_id; | |||
There was a problem hiding this comment.
Add a comment to explain what these fields are being used for.
Also, can you move find_physical_key above the field declarations, so that we keep the field declarations last?
production/db/inc/index/index.inc
Outdated
| @@ -9,6 +9,13 @@ template <typename T_structure, typename T_iterator> | |||
| void index_t<T_structure, T_iterator>::insert_index_entry(index_key_t&& key, index_record_t record) | |||
| { | |||
| std::lock_guard lock(m_index_lock); | |||
|
|
|||
| // Update the last updated txn id if necessary. | |||
| if (m_last_updated_txn_id < record.txn_id) | |||
There was a problem hiding this comment.
This looks like a Yoda-expression. Same for all other such checks.
production/db/inc/index/index.inc
Outdated
| @@ -41,6 +55,15 @@ void index_t<T_structure, T_iterator>::remove_index_entry_with_offsets(const std | |||
| template <typename T_structure, typename T_iterator> | |||
| void index_t<T_structure, T_iterator>::mark_entries_committed(gaia_txn_id_t txn_id) | |||
| { | |||
| { | |||
| std::lock_guard lock(m_index_lock); | |||
| if (m_last_updated_txn_id < m_last_mark_committed_txn_id) | |||
There was a problem hiding this comment.
Should this be <=?
There was a problem hiding this comment.
Also, the logic is not very clear here. I would have expected some check involving txn_id rather this check between two other values.
There was a problem hiding this comment.
Does this mean: "if all updates have already been marked as committed, then there is nothing left to be marked"?
There was a problem hiding this comment.
Should this be <=?
Yes it should have been <=.
Also, the logic is not very clear here. I would have expected some check involving txn_id rather this check between two other values.
The check for txn_id is incorrect! This is because the m_last_updated_txn_id can fall behind the watermark to truncate the metadata, as such, we could leave the index structure referencing truncated metadata.
Yes, the logic if all updates have already been marked committed, there is nothing left to mark is correct. I will be adding some comments to the effect.
production/db/inc/index/index.inc
Outdated
| { | ||
| std::lock_guard lock(m_index_lock); | ||
|
|
||
| if (m_last_updated_txn_id < gc_txn_id) |
There was a problem hiding this comment.
I don't understand this condition. At least, it needs a comment.
There was a problem hiding this comment.
I'm not confident I understand the logic here; some comments (and a bit of reordering as @LaurentiuCristofor suggested) would go a long way.
production/db/inc/index/index.hpp
Outdated
| // The following txn_ids are internal markers to determine if any work needs to be done for garbage collection. | ||
| // | ||
| // last_updated_txn_id - txn_id where the last index is updated/modified. | ||
| // last_mark_committed_txn_id - the txn_id below which all index entries have been mark committed. |
production/db/inc/index/index.hpp
Outdated
|
|
||
| // The following txn_ids are internal markers to determine if any work needs to be done for garbage collection. | ||
| // | ||
| // last_updated_txn_id - txn_id where the last index is updated/modified. |
There was a problem hiding this comment.
What does "last index" mean?
production/db/inc/index/index.hpp
Outdated
| @@ -76,7 +75,7 @@ class index_t : public base_index_t | |||
|
|
|||
| // This method will mark all entries below a specified txn_id as committed. | |||
| // This must only be called once all aborted/terminated index entries below the txn_id are garbage collected. | |||
No description provided.