Lisk-sdk: Improve logic account rank performance

[LucasIsasmendi]

Expected behavior

Improve performance by refactoring the query that obtains the rank from an address.
Options:

• create a view
• create a materialized view

Check #1310 and #1362 for further details about discussion.
This solution will implement a database benchmark process.

Actual behavior

The field rank is calculated with a select over mem_accounts table

Steps to reproduce

Execute npm run mocha test/functional/system/blocks/chain.deleteLastBlock.js on 1.0.0 branch with only (type 2) register delegate

Which version(s) does this affect? (Environment, OS, etc...)

1.0.0

[4miners]

Delegate rank only changes when vote changes, so after finish round. So best performance we can achieve by adding rank field to mem_accounts and update it as part of Round.prototype.land/ound.prototype.backwardLand:

Proposed query:
WITH new AS (SELECT row_number() OVER (ORDER BY vote DESC, "publicKey" ASC) AS rank, "publicKey" FROM mem_accounts WHERE "isDelegate" = 1) UPDATE mem_accounts SET rank = new.rank FROM new WHERE mem_accounts."publicKey" = new."publicKey";

"isDelegate" = 1 in WHERE is optional and depends on performance - we need to ensure we have proper indexes.
_Edit:_ "isDelegate" = 1 is mandatory, as we only need to update rank for delegates.

[robladbrook]

@4miners your approach looks good 👍

I'd join on address instead of publicKey because it's indexed. Ideally you'd be using an integer surrogate key on the table for these situations but that might be premature optimisation!

Can you please tell me why address is saved in a case sensitive format which then requires the two indexes: mem_accounts_address and mem_accounts_address_upper. Why are we not just uppercasing the address each time we insert a new mem_account so we don't have to worry about casing?

[vitaly-t]

I've seen lots of PostgreSQL projects, looking to improve performance, and they all end up with large CTEs, which I anticipated here as well, and that's one of the big reasons why I threw all SQL into external files, before they all start turning into large CTE-s, and thus become impossible to manage while still in the code.

With the new SQL files we can run CTE-s side-by-side, comparing the performance, and all without having to restart the app, thanks to the debug feature provided by class QueryFile :wink:

Moving into plenty of large CTE-s is an unavoidable eventuality for this project ;)

We will get there :wink:

[4miners]

@robladbrook I would like to use publicKey, because it will be still viable after our planned change of addresses system. If there will be a missing index, then for sure it should be added.

Regarding address field - in our blockchain we have at least few transactions (in trs table) that involves addresses that ends with lowercase letter. It was fixed early, both in logic (so we always insert with L) and in database (by some migration - l -> L in few tables) and from that time all addresses should ends with uppercase letter. So we don't need both indexes currently.

[robladbrook]

Makes sense, thanks for explaining @4miners :)

[4miners]

Proposed task list:

• There is column rate in mem_accounts table (not used currently), add migration and rename that column to rank, remove default value 0 and set NULL for all entries
• Update Account.prototype.model in logic/account, update normalFields in db/repos/accounts.js, check for other rate occurencies in application
• Consider move rank from normalFileds to immutableFields
• Remove rank from dynamicFields (db/repos/accounts.js) together with columnRank definition and corresponding SQL file
• Add proposed query to db repos (db/sql/rounds/update_delegates_rank.sql or similar name), link in db/sql/index.js and add function in db/repos/rounds.js
• Add function in logic/round for calling that query (returns promise)
• Call new function as last step of both Round.prototype.land and Round.prototype.backwardLand
• Add unit tests for new functions and update existing unit tests

Please review @karmacoma

[karmacoma]

@4miners Sounds like a plan. I would also add to that task list:

• Extend rounds system functional tests to cover rank update at end of round

[karmacoma]

As some discussion we have decided to delay work on this issue until after 1.0.0.

The next planned step is to benchmark automatically the performance against the existing implementation in order to gives us a clear answer of when to tackle the issue with priority.

This is will be done as part of a wider issue aimed at creating a set of automated db query performance acceptance tests for the entire application, not only this part of it.

[MaciejBaj]

Performance measurements of accounts rank queries will be done as part of https://github.com/LiskHQ/lisk/issues/1623 and refactor if qualified as unacceptable.