Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the updated ledger state in ingest #603

Merged
merged 3 commits into from
Aug 28, 2018
Merged

Conversation

bartekn
Copy link
Contributor

@bartekn bartekn commented Aug 21, 2018

This PR fixes random DB constraint errors like (and similar):

import session failed: Ingestion.Flush error: Error adding values while inserting to history_effects: Error adding values while inserting to history_effects: exec failed: pq: duplicate key value violates unique constraint "hist_e_by_order"

The reason is a synchronization issue between App.UpdateLedgerState and ingest.System.runOnce go routines. Both go routines are executed every second by the application ticker. The following sequence of events is possible:

Time Tick1 Tick2
1 UpdateLedgerState result = [horizon ledger = 10, core ledger = 11]
2 Start ingest session ledgers: [11, 11]
3 UpdateLedgerState result = [horizon ledger = 10, core ledger = 11]
4 Finish ingest session ledgers: [11, 11]
5 Start ingest session ledgers: [11, 11]
6 Error in ingest session ledgers: [11, 11] (because already saved in t=4)

So it is possible that ingest.System.runOnce reads the stale state of ledgers. The simple fix in this PR moves loading the state to ingest.System.runOnce. The long-term fix, in my opinion, should remove synchronization requirement in ingest package (use internal state only, use infinite for instead of ticker etc.).

Close #513.

PS. I've been searching for this for so long! It feels good! 🤓

Copy link
Contributor

@robertDurst robertDurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM based on conversation we had and working with tick/latest ledger state in my current horizon PR. However, may want some input from someone more familiar with the ingestion codebase.

@@ -241,6 +239,24 @@ func (i *System) runOnce() {
is := i.current
i.lock.Unlock()

// Warning: do not check the current ledger state using ledger.CurrentState()! It is updated
// in another go routine and can return the same data for two different ingesiton sessions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ingesiton -> ingestion

@bartekn
Copy link
Contributor Author

bartekn commented Aug 24, 2018

Fortunately, I found a possible deadlock and fixed it in 79e453a. Deadlock was possible when one of the DB queries errors (this did not set i.current = nil and making a new ingest sessions impossible). We should really do: #613.

@bartekn bartekn merged commit c00f136 into master Aug 28, 2018
@bartekn bartekn deleted the fix-ingest-ledger-state branch August 28, 2018 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

duplicate key value violates unique constraint "hist_e_by_order"
2 participants