Remove ability to synchronize objects without primary key #7039
Conversation
With the legacy server this was based on GlobalKey, but if we at some point will allow objects without primary key to be synced to MongoDB server, we will probably have to invent a new strategy. As we remove support for GlobalKey we will have to change the way we allocate ObjKeys for tombstones. Beforehand we used a hash of the primary key to construct a GlobalKey and from that an ObjKey. Now we just use the current ObjKey to construct a corresponding unresolved key. But it has the effect that there is not an easy way to come from primary key to potential unresolved key, so we will have to search for a potential tombstone.
Pull Request Test Coverage Report for Build github_pull_request_281922
💛 - Coveralls |
|
I believe |
jedelbo
force-pushed
the
je/global-key
branch
from
fc5ee2a
to
b394e92
Compare
jedelbo
force-pushed
the
je/global-key
branch
from
33808b1
to
80dfb33
Compare
jedelbo
force-pushed
the
je/global-key
branch
from
0505de4
to
78ebbd0
Compare
| bad_transaction_log("CreateObject(GlobalKey) on table with a primary key"); | ||
| } | ||
| m_last_object = table->create_object(key); | ||
| [&](GlobalKey) { |
There was a problem hiding this comment.
I guess we don't remove this type all together so there is no breaking change?
There was a problem hiding this comment.
Well, this is kind of breaking in that we don't support GlobalKey anymore. But is was never used in context of MongoDB cloud. I think next step would be to remove the type altogether.
There was a problem hiding this comment.
what's stopping us to do it now?
| if (auto sz = m_tombstones->size()) { | ||
| // Check for potential tombstone | ||
| Iterator end(*m_tombstones, sz); | ||
| for (Iterator it(*m_tombstones, 0); it != end; ++it) { |
There was a problem hiding this comment.
isn't this going to be slow? maybe we should run some benchmarks.
There was a problem hiding this comment.
For sure this will be slower than before. The assumption is that we will have a limited number of tombstones. Alternatively we could implement an index just for tombstones.
There was a problem hiding this comment.
Does the assumption hold true? At least with some benchmark we'd know what to expect.
| @@ -570,6 +570,7 @@ void Transaction::upgrade_file_format(int target_file_format_version) | |||
| if (current_file_format_version < 24) { | |||
| for (auto k : table_keys) { | |||
| auto t = get_table(k); | |||
| t->free_collision_table(); | |||
There was a problem hiding this comment.
what was the consequence of not doing this?
There was a problem hiding this comment.
Not sure what the question is. But the collision table is not needed anymore. We will not generate the unresolved key based on the primary key.
There was a problem hiding this comment.
I see. I thought it was a fix.
With the legacy server this was based on GlobalKey, but if we at some point will allow objects without primary key to be synced to MongoDB server, we will probably have to invent a new strategy.
As we remove support for GlobalKey we will have to change the way we allocate ObjKeys for tombstones. Beforehand we used a hash of the primary key to construct a GlobalKey and from that an ObjKey. Now we just use the current ObjKey to construct a corresponding unresolved key. But it has the effect that there is not an easy way to come from primary key to potential unresolved key, so we will have to search for a potential tombstone.
What, How & Why?
☑️ ToDos