analyzer: grab blocks in parallelism-friendly way

[ptrus]

This PR adds support for running multiple consensus/runtime analyzers in parallel. Additionally, the before duplicated "block-picking" code is unified between the consensus and runtime analyzers in analyzer/block/block.go.

"Locking" is implemented by adding locked_time TIMESTAMP WITH TIME ZONE column to the chain.processed_blocks table. processed_time is updated to allow NULL values. The "fast-sync" semantics are:

locked_time	processed_time	state
"expired" (application controlled - 5 minutes)	NULL	unlocked: block can be processed
set and "not-expired"	NULL	locked: block is being processed (started at locked_time)
set	set	done: block was processed (at processed_time)

The block analyzer also supports a "slow-sync" mode, where it is assumed that a single analyzer is processing the blocks, and it ensures blocks that blocks are processed in order.

TODO:

• rebase on upstream

• add unit tests for the new blockBasedAnalyzer code

  • added two basic happy-path tests,
  • add one more tests with some failures

• sanity-check the mega-query performance on a huge table

  • tried locally with 10_000_000 rows in the chain.processed_blocks table. Two cases:
    • most blocks processed: expected case, queries ran in ~10ms and indexes were used as expected
    • most blocks unprocessed: This is a pathological example where there are 10milion locked and failed (expired) blocks in the DB. Queries took up to ~1sec in this case, mostly due to the highest_done_block subquery, which needs to scan all expired locks. Note: this case is likely unable to happen with current design of the indexer: Even if all analyzers keep failing all blocks, the locks will expire (or be unlocked) and the analyzers will keep retrying same blocks, so there is a limit on a number of failed blocks in the db.

• run on mainnet (with parallelism=1) and ensure blocks/sec stays relatively similar, and sanity check results (reindex_and_run.sh)

  • blocks/sec stays consistent on my node (~18 blocs/sec)
  • sanity-check results: no regressions in first 100 blocks

[pro-wh]

is this meant to allow multiple indexer processes to operate on the same db? could we only parallelize block grabbing in a single process for now, without coordinating in the db

[ptrus]

could we only parallelize block grabbing in a single process for now, without coordinating in the db

Could. The ticket suggested using the DB to coordinate as a possible solution, and I think it does make sense in the long-term.
The single-process parallelism would be indeed a bit simpler, though.
Can probably do some tests on how much the in-process parallelism even helps with #385 fix in. I would assume it's less effective than before.

[pro-wh]

ugh I'd better look at the ticket

[pro-wh]

oh dang, it really does call for concurrent access from multiple indexer instances

https://app.clickup.com/t/8669qeadp

[mitjat]

Thank you Peter! I have some nits (it's probably too early for them, but oh well), and a larger comment/question around the core logic of how we grab blocks.

[mitjat]

I'm working on a related change (https://app.clickup.com/t/8669qdzbh), and had a few more realizations in the process of thinking about it.

[mitjat]

Great progress!
I went overboard with one of the comment threads again :|. It's just a lot of text, not a big comment in terms of work.

[mitjat]

Nice. It feels like we're quite close now 👍

[ptrus]

@mitjat ready for final reviews. I have one more todo planned for running reindex_and_run.sh and sanity checking results, other than that I think this is mostly ready. I have also updated the PR description.

[ptrus]

BTW: The block analyzer currently doesn't ensure that blocks are indexed in order even if only a single analyzer is used (e.g. restarting the analyzer process will likely keep some locked unprocessed blocks that will be skipped for the "timeout-period"). So we likely want to merge #405 first, and then also implement a non-fast-sync mode, that is aware of there being a single analyzer and ignoring locks (and always processing in order).

[ptrus]

The UnlockBlockForProcessing can set it to NULL (fast-unlock in case of block process failure).

However, we could change the NULL there to be -infinity (or 0). Not sure why I didn't think of this before. I'll change it, so I can remove the OR IS NULL check in at least one place.

[mitjat]

BTW: The block analyzer currently doesn't ensure that blocks are indexed in order even if only a single analyzer is used

Uh, good point. I don't want to block you on #405 because I got sidetracked with a higher-prioritized requests of making sapphire testnet indexing work.

What if this PR added a fastSync top-level constant, or env-variable-based toggle? Then #405 or its followup (which will implement fast-sync everywhere) can rework that into a parameter to Start(). I think for non-fast-sync mode, we need:

• To run a check that there are no locked blocks right before the megaquery, under the same advisory lock
• To change the continue in line 214 (after a failed block) into a break. Ideally we'd also unlock the remaining blocks in the batch at that point. Alternatively, replace continue with "retry loop at current height".

Did you have different thoughts about implementing slow sync?

[mitjat]

Thank you for all the work on this!
This LGTM 🎉 , with the exception of the point you bring up about slow sync, i.e. guaranteeing that blocks will be processed in-order. Maybe you can add that as a separate commit?

[ptrus]

Thanks for the suggestion!

Did you have different thoughts about implementing slow sync?

I opted for a similar, but a bit of a different solution. Basically, we use the same query in slow-sync mode, only ignoring locks (this is done by using 0 for the lockExpiry in the query). As you suggested, we also break processing the batch when encountering an error, but don't worry about unlocking blocks as the next iteration of picking blocks will treat all locks as expired.

Ideally we'd also unlock the remaining blocks in the batch at that point

I chose to ignore locks, as that is a much more fool-proof solution than relying on unlocking blocks. In unexpected crashes or restarts of the process/host, there is no way to ensure that locks are cleaned up.

To run a check that there are no locked blocks right before the megaquery, under the same advisory lock

I opted against doing this, because there are cases where locked blocks are expected to be present (leftover due to a restart or a crash of the slow-analyzer mid-batch). Ensuring all "fast-synced" blocks were processed and that there is only a single instance of the "slow-sync" analyzer running should be handled in the code setting up the analyzers.

---

See the second commit in the PR for the details.

[mitjat]

I chose to ignore locks, as that is a much more fool-proof solution than relying on unlocking blocks.

I'd say if anything, it's the other way around. These particular locks expire with time, so the situation is self-healing. OTOH now that we're ignoring locks, we have to be careful to not ever run two slow-sync analyzers in parallel (even accidentally, e.g. during a k8s rolling upgrade); or we have to have some code that checks for this situation, as you suggested below.

Ensuring all "fast-synced" blocks were processed and that there is only a single instance of the "slow-sync" analyzer running should be handled in the code setting up the analyzers.

I'd prefer not have the controlling code have to know about the DB and the analyzers' internals. So I would opt to add this check into the slow sync path of the BlockAnalyzer after all. But wherever it gets added, let's do it in a separate PR so we can wrap this one up. The current situation (before this PR) is no different: The enforcement of a single indexer running at a time has to be manual and external.

[mitjat]

Thank you!

[ptrus]

Thanks for all the reviews! I'll likely merge this tomorrow, have a plan to run one more test.

I'd say if anything, it's the other way around. These particular locks expire with time, so the situation is self-healing

I just don't like the idea that in the slow-mode (single analyzer case) a non-graceful restart would then cause 5 minutes (=lock timeout) of downtime where no blocks would be processed. Although the 5-minute timeout was chosen arbitrarily, I would feel differently about it if it would be a minute or so - It might make sense to use a lower timeout at least for slow-syncs.

Technically, the locks check could also miss other analyzers if the query would be run right after a batch was processed by another analyzer, so there would be no locked blocks, so it's not an ideal solution.

Also, there are other ways to detect if multiple analyzers are running, e.g. ensuring that the last-processed block is increasing as expected.

Anyway, agree to revisiting this after.