postgres: do callback hell instead of giant slices - #2002
Conversation
e74cee1 to
7b0f823
Compare
| batch.QueuedQueries = batch.QueuedQueries[:0] | ||
| err = errors.Join( | ||
| tx.SendBatch(ctx, &insertBatch).Close(), | ||
| conn.SendBatch(ctx, &aliasBatch).Close(), |
There was a problem hiding this comment.
I think this batching might introduce deadlocking but I was not able to recreate it in the wild, I think the conditions would have to be:
- Two in-flight alias batches
- Flipped namespace orders (CVE, GHSA and GHSA, CVE)
- Namespace rows are not committed.
This seems very unlikely in the wild but maybe will be more likely the more ecosystems start saving aliases 🤷
There was a problem hiding this comment.
I don't think that will happen because the read-write pair can happen in any order, as long as they're ordered for the connection.
| clear(batch.QueuedQueries) | ||
| batch.QueuedQueries = batch.QueuedQueries[:0] | ||
| err = errors.Join( | ||
| tx.SendBatch(ctx, &insertBatch).Close(), |
There was a problem hiding this comment.
Should we check if insertBatch's tx.SendBatch errors before continuing with the aliasBatch's conn.SendBatch? Seems like FK error would come into play if the vuln batch fails mid-stream but we still try to insert the aliases
There was a problem hiding this comment.
My thinking is that because the batches are populated in the callbacks, we might as well try to flush the remaining ones. I'll add a guard against the vulnID not getting populated.
I think the alias rows being free-floating for a while is fine.
7b0f823 to
0e628c8
Compare
BradLugo
left a comment
BradLugo
left a comment
There was a problem hiding this comment.
I'm not totally convinced this is better than #2001. Couple of things:
-
I don't love the new connection pool. If the implementation turns out to be better, then we can live with it.
-
The benchmarks seem slightly worse:
Benchmark results
│ base │ pr2001 │ pr2002 │ │ sec/op │ sec/op vs base │ sec/op vs base │ UpdateVulnerabilities/100Vulnerabilities-24 75.28m ± 12% 74.11m ± 9% ~ (p=0.796 n=10) 80.31m ± 12% ~ (p=0.143 n=10) UpdateVulnerabilities/500Vulnerabilities-24 109.6m ± 7% 111.9m ± 7% ~ (p=0.052 n=10) 137.4m ± 11% +25.42% (p=0.000 n=10) UpdateVulnerabilities/1200Vulnerabilities-24 171.5m ± 5% 198.4m ± 25% +15.67% (p=0.029 n=10) 278.7m ± 6% +62.51% (p=0.000 n=10) UpdateVulnerabilities/50000Vulnerabilities-24 5.311 ± 5% 7.761 ± 3% +46.13% (p=0.000 n=10) 10.501 ± 2% +97.72% (p=0.000 n=10) UpdateVulnerabilities/75000Vulnerabilities-24 8.157 ± 2% 13.804 ± 2% +69.23% (p=0.000 n=10) 15.876 ± 3% +94.63% (p=0.000 n=10) UpdateVulnerabilities/100000Vulnerabilities-24 10.83 ± 2% 18.66 ± 1% +72.22% (p=0.000 n=10) 20.90 ± 5% +92.91% (p=0.000 n=10) geomean 934.0m 1.219 +30.56% 1.485 +58.98%│ base │ pr2001 │ pr2002 │ │ heapGrowth-B │ heapGrowth-B vs base │ heapGrowth-B vs base │UpdateVulnerabilities/100Vulnerabilities-24 390.8Ki ± 10% 389.0Ki ± 7% ~ (p=1.000 n=10) 626.6Ki ± 7% +60.37% (p=0.000 n=10)
UpdateVulnerabilities/500Vulnerabilities-24 1.524Mi ± 8% 1.550Mi ± 7% ~ (p=0.143 n=10) 2.389Mi ± 2% +56.81% (p=0.000 n=10)
UpdateVulnerabilities/1200Vulnerabilities-24 1.638Mi ± 12% 1.678Mi ± 6% ~ (p=0.796 n=10) 2.500Mi ± 6% +52.65% (p=0.000 n=10)
UpdateVulnerabilities/50000Vulnerabilities-24 37.739Mi ± 12% 1.914Mi ± 17% -94.93% (p=0.000 n=10) 2.631Mi ± 7% -93.03% (p=0.000 n=10)
UpdateVulnerabilities/75000Vulnerabilities-24 48.987Mi ± 6% 2.009Mi ± 8% -95.90% (p=0.000 n=10) 2.633Mi ± 11% -94.62% (p=0.000 n=10)
UpdateVulnerabilities/100000Vulnerabilities-24 67.325Mi ± 28% 2.105Mi ± 10% -96.87% (p=0.000 n=10) 3.018Mi ± 5% -95.52% (p=0.000 n=10)
geomean 7.009Mi 1.414Mi -79.83% 2.060Mi -70.61%│ base │ pr2001 │ pr2002 │ │ B/op │ B/op vs base │ B/op vs base │UpdateVulnerabilities/100Vulnerabilities-24 658.7Ki ± 0% 784.3Ki ± 0% +19.07% (p=0.000 n=10) 1460.1Ki ± 0% +121.67% (p=0.000 n=10)
UpdateVulnerabilities/500Vulnerabilities-24 3.290Mi ± 0% 3.781Mi ± 0% +14.93% (p=0.000 n=10) 6.824Mi ± 0% +107.43% (p=0.000 n=10)
UpdateVulnerabilities/1200Vulnerabilities-24 8.034Mi ± 0% 8.421Mi ± 0% +4.81% (p=0.000 n=10) 16.258Mi ± 0% +102.36% (p=0.000 n=10)
UpdateVulnerabilities/50000Vulnerabilities-24 361.3Mi ± 0% 328.0Mi ± 0% -9.21% (p=0.000 n=10) 673.6Mi ± 0% +86.44% (p=0.000 n=10)
UpdateVulnerabilities/75000Vulnerabilities-24 552.2Mi ± 0% 491.7Mi ± 0% -10.94% (p=0.000 n=10) 1010.3Mi ± 0% +82.97% (p=0.000 n=10)
UpdateVulnerabilities/100000Vulnerabilities-24 722.3Mi ± 0% 655.5Mi ± 0% -9.26% (p=0.000 n=10) 1347.0Mi ± 0% +86.47% (p=0.000 n=10)
geomean 36.72Mi 37.03Mi +0.85% 72.48Mi +97.41%│ base │ pr2001 │ pr2002 │ │ allocs/op │ allocs/op vs base │ allocs/op vs base │UpdateVulnerabilities/100Vulnerabilities-24 5.466k ± 0% 7.881k ± 0% +44.18% (p=0.000 n=10) 18.212k ± 0% +233.19% (p=0.000 n=10)
UpdateVulnerabilities/500Vulnerabilities-24 25.93k ± 0% 37.94k ± 0% +46.36% (p=0.000 n=10) 93.40k ± 0% +260.25% (p=0.000 n=10)
UpdateVulnerabilities/1200Vulnerabilities-24 61.72k ± 0% 90.75k ± 0% +47.04% (p=0.000 n=10) 227.25k ± 0% +268.22% (p=0.000 n=10)
UpdateVulnerabilities/50000Vulnerabilities-24 2.554M ± 0% 3.766M ± 0% +47.45% (p=0.000 n=10) 9.556M ± 0% +274.14% (p=0.000 n=10)
UpdateVulnerabilities/75000Vulnerabilities-24 3.831M ± 0% 5.649M ± 0% +47.45% (p=0.000 n=10) 14.336M ± 0% +274.18% (p=0.000 n=10)
UpdateVulnerabilities/100000Vulnerabilities-24 5.108M ± 0% 7.532M ± 0% +47.46% (p=0.000 n=10) 19.115M ± 0% +274.24% (p=0.000 n=10)
geomean 275.5k 404.0k +46.65% 1.002M +263.73%
That being said, the main memory issue is addressed, so I'm not very inclined to block this merge. I have a couple of other side comments/nits, but I'll defer to @crozzy for the final approval.
0e628c8 to
a5a2b0c
Compare
crozzy
left a comment
crozzy
left a comment
There was a problem hiding this comment.
This seems like a decent improvement
Exercise UpdateVulnerabilities with enough alias-carrying vulnerabilities to cross batch-flush boundaries and verify the link tables directly. The benchmark reports peak live heap alongside the usual metrics, since allocation lifetime is invisible to B/op. Signed-off-by: Brad Lugo <blugo@redhat.com> Signed-off-by: Hank Donnay <hdonnay@redhat.com> Change-Id: Id51fc15a093d90e78733e31182a631e86a6a6964
The chunked link statements are INSERT..SELECTs that must see alias rows committed by concurrent updaters after the transaction began, so pin read committed instead of inheriting default_transaction_isolation. Signed-off-by: Brad Lugo <blugo@redhat.com> Signed-off-by: Hank Donnay <hdonnay@redhat.com> Change-Id: I462ca8ea288011295a2b3cb92ce8ae636a6a6964
Signed-off-by: Hank Donnay <hdonnay@redhat.com> Change-Id: I85a31f5796751b32e698b7b0108461146a6a6964
Signed-off-by: Hank Donnay <hdonnay@redhat.com> Change-Id: I7b0aef57f26107254a546add7442b9556a6a6964
a5a2b0c to
5b55a27
Compare
|
/fast-forward |
3 participants
This set of changes makes the vulnerability update flow operate in more-or-less bounded amounts of memory independent of the number of vulnerability objects handled.
This is done by using a chain of batches where the next one is populated by callbacks triggered when submitting the previous batch. This is a little tricky, but manages to be less code than the previous approach.