Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change SQL statement InsertNewAsset into an upsert. #684

Closed
wants to merge 2 commits into from
Closed

Change SQL statement InsertNewAsset into an upsert. #684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42a64b1
tapdb: add unique constraint on field anchor_utxo_id to table assets
ffranr Nov 17, 2023
ec00e24
tapdb: change InsertNewAsset into UpsertAsset
ffranr Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 6 additions & 24 deletions tapdb/assets_common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ type UpsertAssetStore interface {
QueryAssets(context.Context, QueryAssetFilters) ([]ConfirmedAsset,
error)

// InsertNewAsset inserts a new asset on disk.
InsertNewAsset(ctx context.Context,
arg sqlc.InsertNewAssetParams) (int64, error)
// UpsertAsset upserts an asset on disk.
UpsertAsset(ctx context.Context,
arg sqlc.UpsertAssetParams) (int64, error)

// UpsertAssetMeta inserts a new asset meta into the DB.
UpsertAssetMeta(ctx context.Context, arg NewAssetMeta) (int64, error)
Expand Down Expand Up @@ -204,28 +204,10 @@ func upsertAssetsWithGenesis(ctx context.Context, q UpsertAssetStore,
anchorUtxoID = anchorUtxoIDs[idx]
}

// Check for matching assets in the database. If we find one,
// we'll just use its primary key ID, and we won't attempt to
// insert the asset again.
existingAssets, err := q.QueryAssets(ctx, QueryAssetFilters{
AnchorUtxoID: anchorUtxoID,
GenesisID: sqlInt64(genAssetID),
ScriptKeyID: sqlInt64(scriptKeyID),
})
if err != nil {
return 0, nil, fmt.Errorf("unable to query assets: %w",
err)
}

if len(existingAssets) > 0 {
assetIDs[idx] = existingAssets[0].AssetPrimaryKey
continue
}

// With all the dependent data inserted, we can now insert the
// With all the dependent data inserted, we can now upsert the
// base asset information itself.
assetIDs[idx], err = q.InsertNewAsset(
ctx, sqlc.InsertNewAssetParams{
assetIDs[idx], err = q.UpsertAsset(
ctx, sqlc.UpsertAssetParams{
GenesisID: genAssetID,
Version: int32(a.Version),
ScriptKeyID: scriptKeyID,
Expand Down
91 changes: 51 additions & 40 deletions tapdb/sqlc/assets.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Empty file added tapdb/sqlc/migrations/000013_assets_unique_constraint.down.sql
View file
Open in desktop
Empty file.
31 changes: 31 additions & 0 deletions tapdb/sqlc/migrations/000013_assets_unique_constraint.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
-- We will apply a new unique constraint on the assets table. This constraint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we should add a comment to the .down.sql file that it's empty on purpose.

-- will be on the columns (anchor_utxo_id, genesis_id, script_key_id).
-- Rows in the existing table may violate this constraint, so we need to delete
-- all but one row in each violating set of rows before applying the constraint.
WITH duplicate_rows AS (
SELECT
asset_id,
anchor_utxo_id,
genesis_id,
script_key_id,

-- 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

FROM assets
)
DELETE FROM assets
WHERE (anchor_utxo_id, genesis_id, script_key_id) IN (
SELECT anchor_utxo_id, genesis_id, script_key_id
FROM duplicate_rows
-- Delete all rows with a row number greater than 1. This should leave
-- exactly one row (row number 0) for each set of rows that violate the
-- constraint.
WHERE row_num > 1
);

-- Create a unique index on the new table
CREATE UNIQUE INDEX assets_uniqueness_index_anchor_utxo_id_genesis_id_script_key_id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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":

  1. CREATE TABLE assets_tmp ... (full definition, but without the inline UNIQUE constraint)
  2. INSERT INTO assets_tmp AS SELECT * from assets
  3. Alter any foreign keys that point to the old assets table
  4. DROP TABLE assets
  5. ALTER TABLE assets_tmp RENAME TO assets

ON assets (anchor_utxo_id, genesis_id, script_key_id);

2 changes: 1 addition & 1 deletion tapdb/sqlc/querier.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 14 additions & 3 deletions tapdb/sqlc/queries/assets.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,13 +178,24 @@ INSERT INTO genesis_assets (
DO UPDATE SET asset_id = EXCLUDED.asset_id
RETURNING gen_asset_id;

-- name: InsertNewAsset :one
-- name: UpsertAsset :one
INSERT INTO assets (
genesis_id, version, script_key_id, asset_group_witness_id, script_version,
amount, lock_time, relative_lock_time, anchor_utxo_id, spent
) VALUES (
$1, $2, $3, $4, $5, $6, $7, $8, $9, $10
) RETURNING asset_id;
@genesis_id, @version, @script_key_id, @asset_group_witness_id,
@script_version, @amount, @lock_time, @relative_lock_time, @anchor_utxo_id,
@spent
) ON CONFLICT (anchor_utxo_id, genesis_id, script_key_id)
DO UPDATE SET
version = @version,
asset_group_witness_id = @asset_group_witness_id,
script_version = @script_version,
amount = @amount,
lock_time = @lock_time,
relative_lock_time = @relative_lock_time,
spent = @spent
RETURNING asset_id;

-- name: FetchAssetsForBatch :many
WITH genesis_info AS (
Expand Down