Optimise _update_client_ips_batch_txn to batch together database operations.

[reivilibre]

This easy optimisation was noticed in #10425 by anoa, and it seemed like the right time to implement these
(along with #12251). Not expecting super magical powers, but it might be a nice little improvement.

Depends on: #12251

[clokep]

Looks pretty good!

[clokep]

Pretty sure this is fine, but a couple questions on the locking!

[clokep]

I'm guessing this works fine since we lock the table (as part of the transaction) in the first iterate of the loop, but then it stays locked until the end of the transaction?

1. Is this accurate?
2. If so, is there overhead to locking it for every value? (Should we only lock the table on the first iteration of the loop?)

[reivilibre]

1. Yeah, the lock remains throughout the transaction, as far as I can tell.
2. There may well be, hence adding the flag. Though I suppose you raise a good point that simple_upsert_many_txn_emulated could do that, rather than the calling code...

Note that, as it stands, there's no functional change to the code in the case that native upserts aren't supported. (I'm not even sure upon which databases this code will now run ... ancient SQLites? Do we even support any systems shipping such old SQLites nowadays?...).

[reivilibre]

Debian stretch (just about not EOL) ships an older version of SQLite without upserts, but buster (the next one along) has one modern enough. So I guess this code may not be entirely legacy yet.

[clokep]

@reivilibre judging from your commits my comment above was accurate, I think?

[clokep]

Note that I think this is fine to default to True since simple_upsert_txn_emulated (which this eventually calls) defaults to True. So pretty much this was the default behavior.

[reivilibre]

Yeah, exactly — this is just providing a way to opt out of it.

[clokep]

I don't think this code is used on postgres though since that doesn't need the emulated support here?

[reivilibre]

Well, sigh, you're absolutely right. Locking on SQLite is a no-op (apparently! I haven't fully reasoned through how that's sufficient, but the code is a no-op...?!).

• Postgres: always supports native upsert, so locking doesn't apply.
• SQLite: may not support native upsert, locking is a no-op.

Why do upserts have locking at all then? Maybe I made the mistake of assuming that there must have been a reason for it because of the code that was here before...

In one of the cases, an emulated upsert can be a SELECT followed by an INSERT. In the other case, it's an UPDATE followed by an INSERT...

The latter case (UPDATE, INSERT) doesn't sound like it would need a lock, because both of those operations need a PENDING lock. There's no way another writer can get in the middle of those two.

(Assuming traditional SQLite3 locking) The first case (SELECT, INSERT) may be problematic because SELECT only needs a SHARED lock. But it needs to upgrade to a PENDING lock when it begins the INSERT. If there's another PENDING or stronger lock, it will fail and will need to retry the transaction.

I can't tell whether this works properly in WAL mode or not; the WAL docs aren't particularly clear about how transactions interact like the old-style locking page is. That said, the page on transactions has this to say:

A read transaction is used for reading only. A write transaction allows both reading and writing. A read transaction is started by a SELECT statement, and a write transaction is started by statements like CREATE, DELETE, DROP, INSERT, or UPDATE (collectively "write statements"). If a write statement occurs while a read transaction is active, then the read transaction is upgraded to a write transaction if possible. If some other database connection has already modified the database or is already in the process of modifying the database, then upgrading to a write transaction is not possible and the write statement will fail with SQLITE_BUSY.

Assuming that's still true for WAL mode, then I think it's safe on SQLite.

---

Ah! However, there's one case where Postgres DOES need the emulated support, so I need to take back some of the things I just wrote :/.
That's when the unique index on the table is not ready yet and the table is 'unsafe to upsert'. (This is only a notable effect with Postgres, since SQLite's indices block when adding; Postgres' are added concurrently sometimes). In that case, locking does serve a useful role because Postgres' MVCC means that duplicates could be introduced (and then lead to the failure of creating the index altogether).
Aside: user_directory_search is an eternal exception — it's always 'unsafe to upsert' on SQLite....

That turned into something more complex than I thought :/

[reivilibre]

A read transaction is used for reading only. A write transaction allows both reading and writing. A read transaction is started by a SELECT statement, and a write transaction is started by statements like CREATE, DELETE, DROP, INSERT, or UPDATE (collectively "write statements"). If a write statement occurs while a read transaction is active, then the read transaction is upgraded to a write transaction if possible. If some other database connection has already modified the database or is already in the process of modifying the database, then upgrading to a write transaction is not possible and the write statement will fail with SQLITE_BUSY.

Assuming that's still true for WAL mode, then I think it's safe on SQLite.

I tried this and it seems safe on both WAL and non-WAL mode.

The advantage of WAL mode in this case is that another transaction merely running 'select' and hanging around doesn't prevent you from committing. But that transaction will have to restart (at least if it's in conflict; not sure about generally) — it gets told the database is locked each time it tries to commit or update.

I learnt a bit about SQLite's WAL mode, so that's good at least

[clokep]

From our conversations, I think this is fine.