satellite/overlay: avoid large statement for piece counts

[egonelbre]

What: Use array arguments instead of constructing a large single query.

Why: Concatenated SQL statements are harder to audit and often can end up slower.

Benchmark                                      old ns/op      new ns/op     delta
BenchmarkDB_PieceCounts/Sqlite/Update-32       2403696100     136937262     -94.30%
BenchmarkDB_PieceCounts/Sqlite/All-32          16454915       17416226      +5.84%
BenchmarkDB_PieceCounts/Postgres/Update-32     562646350      149697062     -73.39%
BenchmarkDB_PieceCounts/Postgres/All-32        13657257       14630751      +7.13%

PS: Each commit is separately reviewable.

Please describe the tests:

• Test 1:
• Test 2:

Please describe the performance impact:

Code Review Checklist (to be filled out by reviewer)

• Does the PR describe what changes are being made?
• Does the PR describe why the changes are being made?
• Does the code follow our style guide?
• Does the code follow our testing guide?
• Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
• Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
• Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
• Does any documentation need updating?
• Do the database access patterns make sense?

[stefanbenten]

I am also not sure why this is written directly into the nodes table, rather than being cached and put into the table together with the other metrics

[stefanbenten]

Is it fine to leave the TODO here and remove the code below?
Or better fix it directly? 👍

[egonelbre]

I didn't want to fix it directly at this moment, otherwise the PR would grow too large. But I certainly can remove it.

[stefanbenten]

Wouldnt this mean that if a single update fails, we return out of the entire function and discard the other inserts.
And the piece count for some nodes could be updated by then, as i dont see that we rollback in an error case?

[stefanbenten]

I am not entirely up to speed how critical this number is..

[egonelbre]

Wouldnt this mean that if a single update fails, we return out of the entire function and discard the other inserts.

Yes, but this would also mean that there's something critically wrong already. Either we ended up creating piece count updates for storage nodes that aren't in our table. Or somebody deleted the node from the table... either would be problematic.

There of course could be an issue with database.

Of course, we don't care that much here, since it's for sqlite, which is mainly for testing.

And the piece count for some nodes could be updated by then, as i dont see that we rollback in an error case?

When WithTx returns an error, it will rollback the transaction.

[stefanbenten]

Thanks, that should be ok then 👍

[egonelbre]

@stefanbenten this is not for metrics, this number is used for calculating garbage collection bloom filter sizes.

[VinozzZ]

Just curious why not use .SliceStable here?

[egonelbre]

Because the node ids are guaranteed to be unique, so there wouldn’t be a difference in the result.

[ethanadams]

LGTM