Fix queryPoolFetchRetry #1353
Fix queryPoolFetchRetry #1353
Conversation
erikd
force-pushed
the
erikd/issue-1347
branch
from
e7f5d3e to
dbaab61
Compare
This query was incorrectly putting the Bech32 pool indentifier into the pool URL field of the PoolFetchRetry struct (both were of type `Text` in the database schema). Instead of just fixing this in a simple/naive manner, the PoolUrl type was pushed into the schema (no migration needed) to make querying that field type safe. Closes: #1347
erikd
force-pushed
the
erikd/issue-1347
branch
from
dbaab61 to
a5164a0
Compare
schema/migration-2-0026-20230208.sql
Outdated
| BEGIN | ||
| SELECT stage_two + 1 INTO next_version FROM schema_version ; | ||
| IF next_version = 26 THEN | ||
| EXECUTE 'ALTER TABLE "reverse_index" ALTER COLUMN "min_ids" SET NOT NULL' ; |
There was a problem hiding this comment.
This migration seems irrelevant with this pr. Strange that it wasn't already in place as there is no Maybe annotation in the persistent TH. Possibly the best idea is to actually turn this into nullable. Could we skip the migration for now?
There was a problem hiding this comment.
I always tried to make sure the migration files were always up-to-date.
The problem is that if we do not do this now, this will become part of the next schema change even though its completely unrelated.
There was a problem hiding this comment.
I actually think this shouldn't be a schema change, but a change to the TH and Haskell types.
There was a problem hiding this comment.
This is not a change.
Its fixing an inconsistency between the Haskell schema definition and the migration files.
I think there should be a CI test added to prevent this ever happening again.
There was a problem hiding this comment.
It definitely changes the schema and this should be done only if there is an important reason, as the pr template suggests Migrations
For example for the next release we don't have any schema changes in place. If it ends up being a schema change bump just for this change, then new snapshots need to be created (ie 13.2) which won't be compatible with 13.1 as migrations are not reversible.
There was a problem hiding this comment.
I think there should be a CI test added to prevent this ever happening again.
I agree that a test like this would be great, to avoid similar issues in the future.
There was a problem hiding this comment.
It definitely changes the schema and this should be done only if there is an important reason, as the pr template suggests
The code that is shipped now, means that the Haskell code assumes that this column is never a Maybe whereas the SQL schema thinks that column is nullable. I think that is dangerous.
The migration I am adding makes the SQL column non-nullable.
There was a problem hiding this comment.
Yes in general such mismatches are dangerous. But since only db-sync writes to disk, there will never be a case where a null field is inserted, which could cause issues.
I created a ticket for this #1358 and added it to the next schema changing planned release (13.2.0.0) so we don't forget it https://github.com/orgs/input-output-hk/projects/52/views/6
erikd
force-pushed
the
erikd/issue-1347
branch
from
a5164a0 to
8625330
Compare
|
Hydra is dead, but GHA CI passed. |
No description provided.