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.

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

Fix proof storage clobber error for postgres backend #954

Merged
merged 1 commit into from
Jun 24, 2024
Merged

Conversation

gijswijs
Copy link
Contributor

@gijswijs gijswijs commented Jun 18, 2024

Fixes #951.
An (intermitent) failure popped up when running itest on the postgres backend.

This draft PR tries to solve this error, initially by unifying the behavior of both backends.

Currenlty the status is that this leads to an error later in the process (only for postgres), where the given number of events rhat exist with the given status is of by one.

@dstadulis dstadulis added this to the v0.4 milestone Jun 20, 2024
@gijswijs gijswijs marked this pull request as ready for review June 21, 2024 12:27
@gijswijs
Copy link
Contributor Author

Also fixes #961

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice fix! Have it running on my machine for multiple minutes as well now and so far no problem.

tapdb/asset_minting.go Outdated Show resolved Hide resolved
tapdb/assets_store.go Outdated Show resolved Hide resolved
tapdb/assets_store.go Outdated Show resolved Hide resolved
tapdb/assets_store.go Outdated Show resolved Hide resolved
tapdb/sqlc/queries/assets.sql Outdated Show resolved Hide resolved
tapdb/assets_store.go Outdated Show resolved Hide resolved
tapdb/assets_store.go Outdated Show resolved Hide resolved
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

LGTM
was this originally detected via a flake? just asking whether we added coverage for it or not

tapdb/asset_minting.go Show resolved Hide resolved
tapdb/assets_store.go Show resolved Hide resolved
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice fix! Currently confirming this fully fixes the itest with #965 on top, but so far looks good.

Just nits left.

tapdb/assets_store.go Outdated Show resolved Hide resolved
tapdb/interfaces.go Outdated Show resolved Hide resolved
tapdb/sqlerrors.go Show resolved Hide resolved
tapdb/sqlerrors.go Outdated Show resolved Hide resolved
`UpsertAssetProof` gave issues with a postgresql backend. See: #951

By separating the subquery from the upsert, we can error out if we see
more than one asset ID (primary key from table assets) being returned
from the database. This should return a more meaningful error than the
original postgresql error.

Database deadlock errors are now retried similar to database
serialization errors.
@gijswijs gijswijs added this pull request to the merge queue Jun 24, 2024
@guggero guggero removed this pull request from the merge queue due to a manual request Jun 24, 2024
@guggero guggero merged commit 7dfa9bf into main Jun 24, 2024
14 checks passed
@guggero guggero deleted the postgres-clobber branch June 24, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[bug]/tapdb: proof storage clobber error for postgres backend
4 participants