-
Notifications
You must be signed in to change notification settings - Fork 4
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
consensus: use batch for genesis queries #630
Conversation
oh and hide whitespace on the diff due to some indentation decrease |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, I see the dilemma.
- The 13->17s does suck for local development.
- The dumpable queries ... I don't care for particularly. They can be post-processed pretty easily to show in a readable format, IMO. Sth like output the query template only if it differs from the previous query, otherwise only output the args. Not that we should implement that now; it's just that whoever needs to debug genesis queries that deeply (I haven't, in recent memory) can hack together some insight easily enough.
- The code is so much cleaner, and more resilient to future DB changes.
I vote to check this in. I can wait the extra 4 seconds (times 30 iterations) next time I'm working on something that requires genesis processing.
And it can be sped up if needed:
- The queries are absolutely dominated by account balances (~280k) and staking delegations (~70); there's <10k other rows. So we only really need to speed the first two, or even just the balances.
- The data types involved are simple, so using the COPY protocol like so should be simple too.
- The caveat is that COPY cannot do upserts. We do need upserts because we apply the genesis after fast sync. So you'd need to COPY into a temp table, and then do a
INSERT FROM mytemptable INTO accounts ON CONFLICT ...
. - The exact implementation is a little messy too, but it's not super bad. We'd need to either pass the db connection into genesis.go (inversion of control from what we have now; I'm not a fan), or
Process()
ingenesis.go
would need to return a triple: the[][]interface{}
for balances, the[][]interface{}
for delegations, and the batch. The caller would then have to make both CopyFrom() calls plus exec the batch (which would contain commands for upserting from the temp tables).
- The caveat is that COPY cannot do upserts. We do need upserts because we apply the genesis after fast sync. So you'd need to COPY into a temp table, and then do a
- Conveniently, those are also the only two queries you needed to newly add into
queries.go
. (That's mostly irrelevant, except emotionally :))
Whether you want to take a stab at that is up to you. Even if not, I think this PR is a step in the right direction. Nice cleanup, thank you.
740b9f7
to
65296a8
Compare
ea61d0b
to
b0c1a87
Compare
b0c1a87
to
92aa008
Compare
2446d19
to
fca8b83
Compare
92aa008
to
7af6e38
Compare
fca8b83
to
56baec8
Compare
7af6e38
to
faf3cbd
Compare
56baec8
to
c7b6c03
Compare
f67bc7b
to
943a172
Compare
c7b6c03
to
5c3324f
Compare
943a172
to
8457396
Compare
fba7669
to
8b046c5
Compare
b44e84b
to
2487e50
Compare
32eb535
to
ff220a6
Compare
944e7a8
to
24cbe24
Compare
65790ae
to
12d77ef
Compare
8a8fbb1
to
2f89492
Compare
cf1a164
to
af96146
Compare
2f89492
to
b68a82a
Compare
7cb42d7
to
c79a2c2
Compare
b68a82a
to
96eba3f
Compare
let's go with how this is now |
this kinda sucks
also some of the ON CONFLICT is not exactly preserved, so might be some hidden bugs in network upgrades
measurements, very informal, only one run each
reuse
{"analyzer":"consensus","caller":"genesis.go:52","count":355437,"height":"genesis","level":"info","module":"analysis_service","msg":"generated genesis queries","ts":"2024-02-08T01:42:57.758442953Z"}
{"analyzer":"consensus","caller":"consensus.go:204","level":"info","module":"analysis_service","msg":"genesis document queries generated","process_duration":"32.732929903s","ts":"2024-02-08T01:42:57.758504654Z"}
{"analyzer":"consensus","caller":"consensus.go:228","level":"info","module":"analysis_service","msg":"genesis document processed","send_batch_duration":"17.026809672s","ts":"2024-02-08T01:43:14.785349655Z"}
control
{"analyzer":"consensus","caller":"genesis.go:56","count":369,"height":"genesis","level":"info","module":"analysis_service","msg":"generated genesis queries","ts":"2024-02-08T02:30:31.547038915Z"}
{"analyzer":"consensus","caller":"consensus.go:204","level":"info","module":"analysis_service","msg":"genesis document queries generated","process_duration":"43.796619503s","ts":"2024-02-08T02:30:31.547102908Z"}
{"analyzer":"consensus","caller":"consensus.go:229","level":"info","module":"analysis_service","msg":"genesis document processed","send_batch_duration":"13.060895665s","ts":"2024-02-08T02:30:44.608061096Z"}