Use proper data type for timestamps in Postgres by pm47 · Pull Request #1778 · ACINQ/eclair

pm47 · 2021-04-20T13:38:57Z

Did some refactoring in tests and introduced a new migrationCheck
helper method.

Note that the change of data type in sqlite for the commitment_number
field (from BLOB to INTEGER) is not a migration. If the table has
been created before, it will stay like it was. It doesn't matter due to
how sqlite stores data, and we make sure in tests that there is no
regression.

Did some refactoring in tests and introduced a new `migrationCheck` helper method. Note that the change of data type in sqlite for the `commitment_number` field (from `BLOB` to `INTEGER`) is not a migration. If the table has been created before, it will stay like it was. It doesn't matter due to how sqlite stores data, and we make sure in tests that there is no regression.

pm47 · 2021-04-20T14:31:00Z

I didn't add a specific v5->v6 migration test because existing tests already make sure that there are no issues with the Long/Timestamp conversion. The risk is essentially around the queries, which are tested in the add/list events test. I feel like a new test would add a lot of verbosity for little to no value. Let me know if you disagree.

edit: also, v5 was not part of a release.

pm47 · 2021-04-20T14:55:06Z

eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala

      }

+      def migration56(statement: Statement): Unit = {
+        statement.executeUpdate("ALTER TABLE sent               ALTER COLUMN timestamp SET DATA TYPE TIMESTAMP WITH TIME ZONE USING timestamp with time zone 'epoch' + timestamp * interval '1 millisecond'")


This was provided as an example in the postgres documentation.

I don't think the spaces are very useful, I don't see a good reason to align these statements together.

The idea was to make similarities obvious between lines. Removed in 641abae.

It's still there in this file...?

Oops, forgot one file. Fixed with 1ff9b7b.

thomash-acinq · 2021-04-20T14:56:15Z

I didn't add a specific v5->v6 migration test because existing tests already make sure that there are no issues with the Long/Timestamp conversion. The risk is essentially around the queries, which are tested in the add/list events test. I feel like a new test would add a lot of verbosity for little to no value. Let me know if you disagree.

I agree that the migration is already covered by the other tests. However I find it awkward that the sqlite and postgres databases are now using different versions, I would bump the version for sqlite too just to stay consistent.

codecov-commenter · 2021-04-20T15:08:20Z

Codecov Report

Merging #1778 (1ff9b7b) into master (e092677) will increase coverage by 0.01%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master    #1778      +/-   ##
==========================================
+ Coverage   89.27%   89.29%   +0.01%     
==========================================
  Files         144      144              
  Lines       10903    10923      +20     
  Branches      475      458      -17     
==========================================
+ Hits         9734     9754      +20     
  Misses       1169     1169

Impacted Files	Coverage Δ
...main/scala/fr/acinq/eclair/db/jdbc/JdbcUtils.scala	`95.91% <75.00%> (+0.08%)`	⬆️
...c/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala	`98.27% <95.45%> (-0.83%)`	⬇️
...ain/scala/fr/acinq/eclair/db/pg/PgChannelsDb.scala	`96.34% <100.00%> (+0.45%)`	⬆️
...a/fr/acinq/eclair/db/sqlite/SqliteChannelsDb.scala	`95.94% <100.00%> (ø)`
...cinq/eclair/blockchain/bitcoind/zmq/ZMQActor.scala	`95.00% <0.00%> (+5.00%)`	⬆️

pm47 · 2021-04-20T15:17:32Z

I find it awkward that the sqlite and postgres databases are now using different versions, I would bump the version for sqlite too just to stay consistent.

The reason is that sqlite has not been migrated, it still stores timestamps as INTEGERs. The rationale for not migrating sqlite is that it is much more difficult to do than on postgres (there is no ALTER DATA TYPE) and adds unnecessary risk on our production db. In the future we may significantly change the database format only in Postgres for optimization purposes (e.g. split serialized blobs into several columns), and I think it would make sense to have the version reflect the difference in the schema. Another argument: we could use a totally different backend (e.g. NOSQL) and the versioning would not be the same at all.

eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala

t-bast

Ok I can see why having the version numbers diverge between sqlite and postgres make the migration tests quite harder to read...

But I think there's no way around it, we will want to make them evolve independently. I think it's ok to have completely separate migration tests for each DB type in the future. The important tests to share between DB types are the functional tests of the behavior of the latest DB version (e.g. add/remove/list channels for the channels DB).

t-bast · 2021-04-21T06:50:26Z

eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala

      }

+      def migration56(statement: Statement): Unit = {
+        statement.executeUpdate("ALTER TABLE sent               ALTER COLUMN timestamp SET DATA TYPE TIMESTAMP WITH TIME ZONE USING timestamp with time zone 'epoch' + timestamp * interval '1 millisecond'")


I don't think the spaces are very useful, I don't see a good reason to align these statements together.

eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala

eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqliteChannelsDb.scala

eclair-core/src/test/scala/fr/acinq/eclair/TestDatabases.scala

eclair-core/src/test/scala/fr/acinq/eclair/db/AuditDbSpec.scala

eclair-core/src/test/scala/fr/acinq/eclair/db/ChannelsDbSpec.scala

pm47 · 2021-04-22T08:16:23Z

🤟

For some reason, the payments database was forgotten by #1778.

pm47 force-pushed the db-timestamps branch from fd1e62c to 3213ff4 Compare April 20, 2021 13:45

update audit db (timestamps)

bac226e

pm47 marked this pull request as ready for review April 20, 2021 14:31

pm47 requested a review from t-bast April 20, 2021 14:31

clean up audit tests

c7a88f1

pm47 commented Apr 20, 2021

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala Outdated Show resolved Hide resolved

add ORDER BY timestamp to listRelayed

f418d5b

t-bast reviewed Apr 21, 2021

View reviewed changes

pm47 added 5 commits April 21, 2021 21:21

remove alignment in sql statements

641abae

remove redundant ORDER BY clauses

6b20b25

remove superfluous IF NOT EXISTS clauses

c97900a

nits

aba59ab

fixup! remove alignment in sql statements

1ff9b7b

t-bast approved these changes Apr 22, 2021

View reviewed changes

pm47 merged commit e14c40d into master Apr 22, 2021

pm47 deleted the db-timestamps branch April 22, 2021 08:16

pm47 added a commit that referenced this pull request Jul 5, 2021

Use proper data type for timestamps in Postgres 2

6e47547

For some reason, the payments database was forgotten by #1778.

pm47 mentioned this pull request Jul 5, 2021

Use proper data type for timestamps in Postgres 2 #1862

Merged

pm47 added a commit that referenced this pull request Jul 7, 2021

Use proper data type for timestamps in Postgres 2

3ce0716

For some reason, the payments database was forgotten by #1778.

pm47 added a commit that referenced this pull request Jul 7, 2021

Use proper data type for timestamps in Postgres 2

e06c23f

For some reason, the payments database was forgotten by #1778.

pm47 added a commit that referenced this pull request Jul 8, 2021

Use proper data type for timestamps in Postgres 2

22602c0

For some reason, the payments database was forgotten by #1778.

pm47 added a commit that referenced this pull request Jul 8, 2021

Use proper data type for timestamps in Postgres 2 (#1862)

cea3fc0

For some reason, the payments database was forgotten by #1778.

Conversation

pm47 commented Apr 20, 2021

Uh oh!

pm47 commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pm47 Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-bast Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

pm47 Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

t-bast Apr 22, 2021

Choose a reason for hiding this comment

Uh oh!

pm47 Apr 22, 2021

Choose a reason for hiding this comment

Uh oh!

thomash-acinq commented Apr 20, 2021

Uh oh!

codecov-commenter commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pm47 commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

t-bast Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pm47 commented Apr 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pm47 commented Apr 20, 2021 •

edited

Loading

pm47 Apr 20, 2021 •

edited

Loading

codecov-commenter commented Apr 20, 2021 •

edited

Loading

pm47 commented Apr 20, 2021 •

edited

Loading