Inconsistency in migrations

[4miners]

Expected behavior

N/A

Actual behavior

There are several inconsistency in our migration process:

• Files
  20161016133824_addBroadhashColumnToPeers.sql
20161016133824_addHeightColumnToPeers.sql
  have same ID, that can result with second migration beign not applied correctly on some nodes. We should change ID of second migration and try to re-execute it if needed (change query to ALTER TABLE "peers" ADD COLUMN IF NOT EXISTS "height" INT;)
• Query that inserts to migations table have ON CONFLICT DO NOTHING, which is not acceptable, as migration should be only executed (and then inserted) once.
• After renaming all SQL files to underscore case (in Updating sql files - Closes #1453 #1455) - entries in migrations table still contains old migration names for already executed migrations. For consistency entries in database should match new names.
• We should review migration script if it sorts migrations based on ID ascending and then executes them, because migrations should be executed in strict order.

Steps to reproduce

N/A

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

1.0.0

[vitaly-t]

have same ID, that can result with second migration beign not applied correctly on some nodes

Was this rule not added to cater for this? - https://github.com/LiskHQ/lisk/blob/development/db/repos/migrations.js#L113

We should change ID of second migration

Change it to what - higher/lower value?

migration should be only executed (and then inserted) once.

We can try, and see if anyone reports issues after such change...

For consistency entries in database should match new names.

We would need to execute a separate script to patch those, at some point, not yet sure when. Can we run such a script manually?

if it sorts migrations based on ID ascending and then executes them, because migrations should be executed in strict order

I thought the ascending order was provided through the fs.readdir API, based on the leading digital stamp in the name, though I didn't verify that.

[vitaly-t]

And here's how you can patch your migrations:

UPDATE migrations SET name = lower(regexp_replace(name, E'([A-Z])', E'\_\\1','g'));

It will replace all camel-case records in column name with the underscore syntax. Repeated runs will not corrupt anything.

[vitaly-t]

I have checked the order in which fs.readdir returns files, and it is always in ascending order:

20160723182900_createSchema.sql
20160723182901_createViews.sql
20160724114255_createMemoryTables.sql
20160724132825_upcaseMemoryTableAddresses.sql
20160725173858_alterMemAccountsColumns.sql
20160908120022_addVirginColumnToMemAccounts.sql
20160908215531_protectMemAccountsColumns.sql
20161007153817_createMemoryTableIndexes.sql
20161016133824_addBroadhashColumnToPeers.sql
20161016133824_addHeightColumnToPeers.sql
20170113181857_addConstraintsToPeers.sql
20170124071600_recreateTrsListView.sql
20170319001337_createIndexes.sql
20170321001337_createRoundsFeesTable.sql
20170328001337_recreateTrsListView.sql
20170403001337_calculateBlocksRewards.sql
20170408001337_createIndex.sql
20170422001337_recreateCalculateBlocksRewards.sql
20170428001337_recreateTrsLiskView.sql
20170614155841_uniqueDelegatesConstraint.sql
20170921105500_recreateRevertMemAccountTrigger.sql

so the migration strategy seems correct.

[karmacoma]

We should change ID of second migration

Higher

For consistency entries in database should match new names.

If possible, as a migration in itself.

[vitaly-t]

So I've tried with this PR: #1473

But for some reasons that starts throwing errors:

Error: relation "blocks" does not exist
Error: relation "mem_accounts" does not exist

Any idea why is that? ;)

[karmacoma]

See #1473 (comment)

[vitaly-t]

The issue has been sorted, by applying the patch only if a migration is found.

[4miners]

I don't think that we should rely on fs.readDir behavior.

References: nodejs/node#3232

[4miners]

Another question, so now underscore patch for migration names will be executed every time (even if already done)? Why not do that as normal miration (with timestamp)?

[4miners]

20161016133825_add_height_column_to_peers.sql should be renamed with timestamp higher than last one, or it will be not executed at all. Also query should be changed as propsed in issue.

[karmacoma]

I don’t think that we should rely on fs.readDir behavior.

Ok fair point maybe.

Another question, so now underscore patch for migration names will be executed every time (even if already done)? Why not do that as normal miration (with timestamp)?

Migration in itself was preferred, but runtime operation not bad either.

20161016133825_add_height_column_to_peers.sql should be renamed with timestamp higher than last one, or it will be not executed at all.

I believe it was renamed as requested. From: 20161016133824_add_height_column_to_peers.sql, to: 20161016133825_add_height_column_to_peers.sql

Also query should be changed as propsed in issue.

Ok true, that was not changed.

[vitaly-t]

I don't think that we should rely on fs.readDir behavior.

You gave a link to some very old, Windows-specific issue. Why do we care about that?

Why not do that as normal migration (with timestamp)?

Could you, please clarify, what you mean by normal migration (with timestamp)?

20161016133825_add_height_column_to_peers.sql should be renamed with timestamp higher than last one

It was renamed into one with higher timestamp, as per the PR. The name changed from 20161016133824_ to 20161016133825_

@karmacoma You posted ahead of me by a second 😄

[4miners]

@vitaly-t Regarding fs.readDir - Are you sure how it will behave on different platforms? Are you sure that behavior will not change in future versions? If it will change we will even not notice.

By normal migration I mean another migration in standard migrations chain, for example: 20180102001337_underscore_patch.sql, as it only needs to be executed once, not every time when application starts.

From issue description: We should change ID of second migration and try to re-execute it if needed (change query to ALTER TABLE "peers" ADD COLUMN IF NOT EXISTS "height" INT;). Increasing migration ID is not something that will cause re-execution of that migration. Because last ID will be 20171227155620, so all migrations with IDs lower than that will be skipped, including 20161016133825_add_height_column_to_peers.sql.

[karmacoma]

@4miners Issue now also assigned to you. Please implement remaining inconsistencies as you seen them and submit as a PR.

[4miners]

For reference - state of migrations table on mainnet:

lisk-node-1=# select * from migrations;
       id       |              name               
----------------+---------------------------------
 20160723182900 | createSchema
 20160723182901 | createViews
 20160724114255 | createMemoryTables
 20160724132825 | upcaseMemoryTableAddresses
 20160725173858 | alterMemAccountsColumns
 20160908120022 | addVirginColumnToMemAccounts
 20160908215531 | protectMemAccountsColumns
 20161007153817 | createMemoryTableIndexes
 20161016133824 | addBroadhashColumnToPeers
 20170113181857 | addConstraintsToPeers
 20170124071600 | recreateTrsListView
 20170319001337 | createIndexes
 20170321001337 | createRoundsFeesTable
 20170328001337 | recreateTrsListView
 20170403001337 | calculateBlocksRewards
 20170408001337 | createIndex
 20170422001337 | recreateCalculateBlocksRewards
 20170428001337 | recreateTrsLiskView
 20170614155841 | uniqueDelegates
 20170921105500 | recreateRevertMemAccountTrigger
(20 rows)