Create index method #125
Create index method #125
Conversation
|
|
|
@lthibault Thx for approve. Is there anything I have to do for merging or is all set? |
|
@hashicorp-cla @lthibault Hi, will this PR get merged in the near future? Or did I miss anything I have to do? |
|
@smcdonald45 I'm afraid I'm not part of the official team, so I can't say. 😕 |
|
@armon Hi, could anyone of you or your team please review this PR? That would be great. |
|
@smcdonald45 If this project turns out to be unmaintained by Hashicorp, I'd be happy to participate in a fork. Maybe that's the best option? |
|
@smcdonald45 thanks for this awesome PR 🎉 Sorry this has sat for so long, I just came across it today. At a quick glance, I think this looks really interesting. I'll try to take a closer look this week as I'm heading out for today. Out of curiosity - do you have any ballpark numbers on how much quicker your bulk loading is with this PR? In Consul we've known that index building is expensive on startup for a while although it's not tended to be a deal-breaker for our users at typical data sizes Consul uses. Given that this is only additive it seems reasonably low risk and I'd love to have folks on the Consul team see what impact it has on startup times with a 1GB+ snapshot. |
@banks , I can setup some experiments to measure the potential improvements in consul startup time by this PR . If I understand correctly, in the experiment we need
Do I miss any steps? Thanks. |
|
@huikang thanks for looking. After thinking some more it's less clear how easily we'd be able to benefit from this in Consul. But with a bit more thought, that would be a pretty significant change to our state store since "rebuilding" our memDB isn't just bulk-loading data but actually replaying raft logs or snapshot entries which are not the same format as in-memory and so need to pass through all the logic of our FSM before being inserted. For this to work we'd need to do something like:
At this point this is sounding like a major and risky refactor as well as long-term maintenance complexity so it may well not be worth pursuing for Consul. If we decide startup time is important enough to warrant that much effort, there are probably ways we could do even better if we changed how Restore works in Consul (e.g. batching updates in transactions, changing our snapshot format so there is less work to do etc.). My initial hope was that this was a realtively easy change that would get some benefit but I'm not sure it is for Consul at least. That doesn't mean this PR is not useful to other users of the library though so we should review it in general! |
There was a problem hiding this comment.
Hi @smcdonald45,
This looks great to me. I had a couple of minor questions inline. None are show stoppers as this seems pretty low risk, but would love to know if there are opportunities to make it even simpler!
memdb.go
Outdated
| for _, indexSchema := range schema { | ||
| index := iradix.New() | ||
| path := indexPath(table, indexSchema.Name) | ||
| root, _, _ = root.Insert(path, index) |
There was a problem hiding this comment.
Is there a reason why the new indexes are added to the root tree outside of the transaction below? If there are 10 indexes for example this would be 10 separate root tree transactions. I guess there is no way these insertions can "fail" so it's not really a correctness issue but leaves me wondering if there is some subtle reason I don't understand for it to be this way instead of creating the transaction above and using it?
| // copy the new created indexes into our schema | ||
| tableSchema := db.schema.Tables[table] | ||
| for _, indexSchema := range schema { | ||
| tableSchema.Indexes[indexSchema.Name] = indexSchema | ||
| } |
There was a problem hiding this comment.
Hmmm this seems like a race footgun that we should at least document in the doc comment for this method.
I think it's true that today we never expect db.schema to change after initialization therefore readers will read from it without taking the db.writer.Lock which would be a data race if there were any readers concurrent with this call.
I think that's probably OK for it's intended usage and adding a lock to every read would be too invasive and impact performance for all users, but we should at least point out the requirement in the doc comments that this MUST NOT be called concurrently with any reads. For it's intended purpose that would probably be a logical bug anyway since the indexes the readers presumably need wouldn't exist yet but it might not be obvious to other library users!
| // Get the primary ID of the object | ||
| ok, idVal, err := idIndexer.FromObject(next) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to build primary index: %v", err) | ||
| } | ||
| if !ok { | ||
| return fmt.Errorf("object missing primary index") | ||
| } | ||
|
|
||
| for _, indexSchema := range indexes { | ||
| indexTxn := txn.writableIndex(table, indexSchema.Name) | ||
|
|
||
| var vals [][]byte | ||
| vals, err = txn.createIndexValue(indexSchema, idVal, next) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Update the value of the index | ||
| for _, val := range vals { | ||
| indexTxn.Insert(val, next) | ||
| } | ||
| } |
There was a problem hiding this comment.
I see you factored out txn.createIndexValue from Insert rather than copying it which makes sense, but unless I'm missing something, isn't the whole of this block identical to the Insert code? Could we factor out the whole thing? Or even just call txn.Insert directly? I guess that does slightly more work by replacing the ID index with existing values too which is a no-op but wasted effort.
I'd be interested to know if you measure significnatly worse times from just calling Insert for your use-case. The benefit is reduced duplication and no change to the current code path every other caller of this library is using. The change is straightforward so it's not high risk, but it's more than no change!
Hi,
while starting our server we consume a lot of data and stores it into the go-memdb. While consuming we have a lot of updates which takes really long cause we have a multi-value index in our data.
With this PR we can remove the expensive index from the initial schema, store all data and create the index afterwards. This safes us a lot of startup time.
Maybe you can take a look at this PR. If there are any questions feel free to ask.
Thx a lot!!
Stephan