Page MenuHomeSoftware Heritage
Differential D8873

storage: Insert from temporary tables in consistent order
ClosedPublic
Actions

Authored by vlorentz on Nov 23 2022, 9:21 AM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Maniphest Tasks
T4696: indexer storage: Deadlock in origin_extrinsic_metadata_add
Commits
rDCIDXf7833b7e2d98: storage: Insert from temporary tables in consistent order
Summary

This avoids having a transaction inserting row A then B, while another
inserts row B then A; which (probably) leads to deadlocks like this:

DeadlockDetected: deadlock detected
DETAIL:  Process 1842336 waits for ShareLock on transaction 1051957280; blocked by process 64261.
Process 64261 waits for ShareLock on transaction 1051957281; blocked by process 1842336.
HINT:  See server log for query details.
CONTEXT:  while inserting index tuple (1972253,5) in relation "origin_extrinsic_metadata"
SQL statement "insert into origin_extrinsic_metadata (id, metadata, indexer_configuration_id, from_remd_id, metadata_tsvector, mappings)

https://sentry.softwareheritage.org/share/issue/52b06caae89f4235a758887fd6817656/

This was already mitigating by sorting before inserting in temporary
tables, then expecting postgresql to read from temporary tables in the
same order rows where inserted. This is often true, but not guaranteed.

Resolves T4696.

Test Plan

No test for this, because I do not see a way to replicate this more than
existing deadlock tests do.

Diff Detail

Repository
rDCIDX Metadata indexer
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 32985
Build 51692: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 51691: arc lint + arc unit

Event Timeline

vlorentz created this revision.Nov 23 2022, 9:21 AM
Herald added a reviewer: Reviewers. · View Herald TranscriptNov 23 2022, 9:21 AM
swh-public-ci added a comment.Nov 23 2022, 9:26 AM

Build is green

Patch application report for D8873 (id=31995)

Rebasing onto 221d48e242...

Current branch diff-target is up to date.
Changes applied before test
commit b52d14e653811077a50f5f300d8a33bc6fe38da9
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 23 09:11:21 2022 +0100

    storage: Insert from temporary tables in consistent order
    
    This avoids having a transaction inserting row A then B, while another
    inserts row B then A; which (probably) leads to deadlocks like this:
DeadlockDetected: deadlock detected
DETAIL:  Process 1842336 waits for ShareLock on transaction 1051957280; blocked by process 64261.
Process 64261 waits for ShareLock on transaction 1051957281; blocked by process 1842336.
HINT:  See server log for query details.
CONTEXT:  while inserting index tuple (1972253,5) in relation "origin_extrinsic_metadata"
SQL statement "insert into origin_extrinsic_metadata (id, metadata, indexer_configuration_id, from_remd_id, metadata_tsvector, mappings)
```

https://sentry.softwareheritage.org/share/issue/52b06caae89f4235a758887fd6817656/

This was already mitigating by sorting before inserting in temporary
tables, then expecting postgresql to read from temporary tables in the
same order rows where inserted. This is often true, but not guaranteed.

No test for this, because I do not see a way to replicate this more than
existing deadlock tests do.
See https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/527/ for more details.
Harbormaster completed remote builds in B32922: Diff 31995.Nov 23 2022, 9:26 AM
vlorentz requested review of this revision.Nov 23 2022, 9:26 AM
ardumont added a subscriber: ardumont.EditedNov 23 2022, 10:18 AM

lgtm but this is missing the migration script update first (and the bump in the schema version somewhere?).

(i'll let you deal with the stuff that shoud disappear as you wish)

swh/indexer/sql/50-func.sql
98

This can go away (and should have already but...), i don't think we still have the content_language_* entrypoints in the indexer storage (rapid check here in storage.py confirms it).

(in another diff/commit or whatever you wish).

149

ditto for content_ctags_*...

olasd accepted this revision.Nov 28 2022, 10:45 AM
olasd added a subscriber: olasd.

As mentioned by @ardumont, please bump the schema version and add a migration script.

(I'm a bit dubious about whether this will actually mitigate the issue, but it can't really hurt to try)

This revision is now accepted and ready to land.Nov 28 2022, 10:45 AM
vlorentz updated this revision to Diff 32036.Nov 28 2022, 11:00 AM

bump schema version and add migration

swh/indexer/sql/50-func.sql
98

ah, I felt there was something fishy about the mismatched number of changes in .py and .sql.

I'll do it in a next diff, thanks

swh-public-ci added a comment.Nov 28 2022, 11:04 AM

Build is green

Patch application report for D8873 (id=32036)

Rebasing onto 221d48e242...

Current branch diff-target is up to date.
Changes applied before test
commit f7833b7e2d98a0e19faa8c0acf0c7fc5b8b3e681
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 23 09:11:21 2022 +0100

    storage: Insert from temporary tables in consistent order
    
    This avoids having a transaction inserting row A then B, while another
    inserts row B then A; which (probably) leads to deadlocks like this:
DeadlockDetected: deadlock detected
DETAIL:  Process 1842336 waits for ShareLock on transaction 1051957280; blocked by process 64261.
Process 64261 waits for ShareLock on transaction 1051957281; blocked by process 1842336.
HINT:  See server log for query details.
CONTEXT:  while inserting index tuple (1972253,5) in relation "origin_extrinsic_metadata"
SQL statement "insert into origin_extrinsic_metadata (id, metadata, indexer_configuration_id, from_remd_id, metadata_tsvector, mappings)
```

https://sentry.softwareheritage.org/share/issue/52b06caae89f4235a758887fd6817656/

This was already mitigating by sorting before inserting in temporary
tables, then expecting postgresql to read from temporary tables in the
same order rows where inserted. This is often true, but not guaranteed.

No test for this, because I do not see a way to replicate this more than
existing deadlock tests do.
See https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/528/ for more details.
Harbormaster completed remote builds in B32985: Diff 32036.Nov 28 2022, 11:04 AM
Closed by commit rDCIDXf7833b7e2d98: storage: Insert from temporary tables in consistent order (authored by vlorentz). · Explain WhyNov 28 2022, 12:33 PM
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDCIDXf7833b7e2d98: storage: Insert from temporary tables in consistent order.
vlorentz mentioned this in D8888: Drop content_language and content_ctags tables and related SQL functions.Nov 28 2022, 12:33 PM
vlorentz added inline comments.Nov 28 2022, 12:34 PM
swh/indexer/sql/50-func.sql
98

D8888

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
indexer/
sql/
9 lines--
upgrades/
214 lines--
storage/
8 lines95%
Loading...

Diff 32036

View Options

swh/indexer/sql/50-func.sql

Loading...
View Options

swh/indexer/sql/upgrades/136.sql

Loading...
View Options

swh/indexer/storage/__init__.py

Loading...
About · Developer information · Legal