Refactor Postgres code #1743
Refactor Postgres code #1743
Conversation
More symmetry between postgres and sqlite init.
Thoses tests apply to both sqlite and postgres.
Those classes implement traits `Databases`, `FileBackup` and `ExclusiveLock`. The goal is to have access to backend-specific attributes, particularly in tests. It arguably makes the `Databases` cleaner and simpler, with a nice symmetry between the `apply methods`.
- replace exceptions by failures+Either - cleaner failure handlers
| private def acquireExclusiveTableLock()(implicit connection: Connection): Unit = { | ||
| using(connection.createStatement()) { | ||
| statement => | ||
| statement.executeUpdate(s"SET lock_timeout TO '${LockTimeout.toSeconds}s'") |
There was a problem hiding this comment.
@rorp. Why not use NOWAIT here?
There was a problem hiding this comment.
I went ahead and changed it in 723d926, but I'm still interested in your rationale opting for a non-zero timeout.
There was a problem hiding this comment.
AFAIR there was some weirdness in exceptions Pg driver threw in NOWAIT mode. Should work in both ways.
There was a problem hiding this comment.
Seeing that lock errors seem to be handled fine in tests, I think we can go ahead with NOWAIT.
There was a problem hiding this comment.
Okay I think I know why it was done that way. There can be a conflict between a regular SQL requests checking the lock, and the background task that regularly renew the lease. If those happen at the same time, one of them will fail which will make the app stop. Adding a 5 seconds timeout provided some leeway, one of the lock attempts would just wait after the first one is completed.
Hmm no, that's wrong. Regular SQL requests only check the lease and do not attempt locking, so it can't be that. Apparently I ran into the "weird exceptions" that you were mentioning. Will dig further...
It is more consistent with sqlite, and makes sense because we don't want to mix up testnet and mainnet databases.
eclair-core/src/test/scala/fr/acinq/eclair/db/PgUtilsSpec.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/db/PgUtilsSpec.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/db/PgUtilsSpec.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgPaymentsDb.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/db/PgUtilsSpec.scala
Outdated
Show resolved
Hide resolved
More symmetry between postgres and sqlite, and better tests.