Change SQL statement InsertNewAsset into an upsert. #684
Conversation
ffranr
force-pushed
the
balance-bug-db-query-fix
branch
from
66a73c2 to
f10120c
Compare
ffranr
changed the title
|
A less risk solution, please consider first: #687 |
ffranr
force-pushed
the
balance-bug-db-query-fix
branch
from
98c2233 to
ec00e24
Compare
|
I've just rebased and bumped SQL migration numbers. The itest was removed from this PR because it's already merged. |
| @@ -0,0 +1,31 @@ | |||
| -- We will apply a new unique constraint on the assets table. This constraint | |||
There was a problem hiding this comment.
nit: we should add a comment to the .down.sql file that it's empty on purpose.
| -- This is the row number of the row within the set of rows that violate | ||
| -- the constraint. We will delete all rows with a row number greater | ||
| -- than 1. | ||
| ROW_NUMBER() OVER (PARTITION BY anchor_utxo_id, genesis_id, script_key_id ORDER BY asset_id) AS row_num |
There was a problem hiding this comment.
Ah, I see now why we'd want to have a way to test migration scripts...
I think this probably works, but we definitely need to make sure it does the correct thing for both Postgres and SQLite.
| ); | ||
|
|
||
| -- Create a unique index on the new table | ||
| CREATE UNIQUE INDEX assets_uniqueness_index_anchor_utxo_id_genesis_id_script_key_id |
There was a problem hiding this comment.
Do we have a way to remove the index created by the previous UNIQUE(asset_id, genesis_id, script_key_id) part? I guess that's probably dependent on the actual DB implementation.
This is likely a downside of using the "inline" unique key creation, that it doesn't have a name and therefore can't easily be updated.
There was a problem hiding this comment.
Hmm, I think I google that before at some point... And if I remember correctly, we basically need to re-create the table to get rid of the original unique constraint. But I think we'd probably want to do that, as otherwise this is an expensive but useless index we're carrying along.
IIRC this is how you replace a table "correctly":
CREATE TABLE assets_tmp ... (full definition, but without the inline UNIQUE constraint)INSERT INTO assets_tmp AS SELECT * from assets- Alter any foreign keys that point to the old
assetstable DROP TABLE assetsALTER TABLE assets_tmp RENAME TO assets
|
Now that the db migration work has been merged. We should test this PR using that framework. |
|
@ffranr is this PR still in progress or should we close? |
|
I don't think this is worth doing right now. Perhaps in the future. |
This PR changes an asset insert SQL query into an upsert.
InsertNewAsset->UpsertAsset. This should fix #665 at the DB level. But we should also apply a fix to the custodian.I think that this PR sucessfully adds a new constraint to the
assetsSQL table. It doesn't remove the old constraint. But I don't think we need to because the old unique constraint didn't actually constrain anything. It contained the row ID primary key so it was basically a NOP. Am I right?I'm not sure if I've implemented the
assetstable migration correctly. What happens to those rows which are in tables with foreign key relationships to theassetstable?Even if this PR works. There's a bunch more things that we need to do to make tapd robust to this problem:
UpsertAssetquery.