Repository navigation
fix(sql): keep the DB connection across maps, batch replace import - #106
Conversation
- OnMapEnd no longer deletes a fully set up connection; a connection still
in its startup (or down, or connecting) is reset as before. OnConfigsExecuted
only connects (or applies the connect delay) when there is no connection,
and re-runs a replace list SELECT whose retry timer died with the map.
- SetCharset("utf8mb4") once per new MySQL connection; SET NAMES only as
fallback (it is lost on the driver's auto-reconnect).
- sm_cccimportreplacefile sends all entries in one transaction.
- ON DUPLICATE KEY UPDATE no longer rewrites the primary key and uses
VALUES(col) instead of repeating every value (MySQL only).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WareZ57
left a comment
There was a problem hiding this comment.
I read this against master (809a269, merges cleanly) and compiled CustomChatColors.sp with spcomp 1.12.0.7259 and the CI deps (MultiColors, SelfMute, sourcebans-pp, DynamicChannels): it builds clean.
The connection-keeping logic looks right to me. OnMapEnd only keeps a handle that is Connected with the startup gate done. In every other state it resets the same way as before: the reconnect timer is deleted and the lock is zeroed, so a connect still in flight is discarded by the lock check in OnSQLConnected. Retry packs and replace reservations are still freed on every map end. SetCharset runs before anything is queued on the new handle.
The import transaction also checks out:
- the pack is freed on both the success and the error callback;
- reservations use the same limit check as
SQLInsert_Replaceand are released on failure; - the error path goes through
SQL_QueryFailed; - the console case no longer calls
GetClientUserId(0); - unlike the old per-row path, the cache is now updated even if the admin left before the insert came back.
Max lengths fit too: trigger 31 bytes into VARCHAR(32), value 255 into VARCHAR(255), and the query stays well under 1024 now that the values aren't repeated, which the old upsert could actually overflow. VALUES(col) works on MySQL 8.4. I tested it: it only raises the 1287 deprecation warning, and it's the form MariaDB also accepts, so that's fine.
One behaviour change I think needs fixing:
The replace list is no longer reloaded on map change (CustomChatColors.sp:382-387)
ccc_replace is a global table with no server id. Before this PR, every map change reconnected and TryFinalizeSQLStartup() ran SQLSelect_Replace() again. So:
- triggers added or deleted from another server sharing the DB (or straight in the DB / from a panel) showed up at the next map;
- a replace-list SELECT that had given up healed itself at the next map.
With the kept connection, OnConfigsExecuted only re-runs the SELECT when g_iSQLSelectReplaceRetry > 0. That counter is reset to 0 both on success and when the retries are exhausted (OnSQLSelect_Replace, :1621; sm_ccc_sql_max_retries defaults to 1). In both cases the list is never refreshed again until sm_reloadccc or a plugin reload.
Concrete case: two servers on the same CCC database. An admin runs sm_cccaddtrigger :foo: bar on server A. Server B never gets :foo: until it restarts or someone runs sm_reloadccc there. On master it would have appeared after B's next map change.
The fix is small: always reload in the kept-connection branch. It's one SELECT of at most 255 rows per map, and OnSQLSelect_Replace already rebuilds the list (ResetReplaceList()), so a duplicate in-flight SELECT is harmless:
if (g_hDatabase != null && g_DatabaseState == DatabaseState_Connected)
{
// Connection kept from the previous map: refresh the replace list like the per-map reconnect used to
SQLSelect_Replace(null);
}Everything else (tags, bans) is loaded per client in OnClientPostAdminCheck, which runs again for everyone after a map change, so I don't see the same problem there.
Minor note, no change needed: the "Imported N trigger(s)" reply is sent before the transaction completes, so on failure the admin sees the success line followed by the failure line. That's acceptable since the error message is explicit.
Summary
CustomChatColors.sp):OnMapEnddeletedg_hDatabaseon every map change andOnConfigsExecutedopened a new one. Each map paid for a new connection,SET NAMES, 3xCREATE TABLE IF NOT EXISTS, the replace-list SELECT and a reload of every in-game client, and admin commands / tag loads failed until that was done (longer withsm_ccc_db_connect_delay). SourceMod keeps the plugin and its handle loaded across maps and the MySQL driver reconnects by itself, so a fully set up connection is now kept:OnMapEndreturns early when the state isConnectedand the startup gate is done. In any other state (down with a reconnect pending, connecting, or still in its startup, whose retry timers die with the map) it resets exactly as before.OnConfigsExecutedonly connects, or applies the connect delay, when there is no connection; with a kept connection it only re-runs the replace-list SELECT if its retry timer was killed by the map change. The epoch / connect-lock / probe / drop logic is unchanged and still decides when a connection is dead. Pending retry packs and replace reservations are still freed on map end as before.CustomChatColors.sp): the charset was only set with an asyncSET NAMES. That is lost when the driver auto-reconnects, which matters more now that the connection lives for the whole server uptime.OnSQLConnectednow callsdb.SetCharset("utf8mb4")once per new MySQL connection, before any query is queued (it stays in the connection options).SET NAMESis kept only as fallback whenSetCharsetfails, and the startup gate is unchanged.CustomChatColors.sp):sm_cccimportreplacefilesent one INSERT per KeyValues entry (up to 255 round trips). The entries now go in oneTransaction. They use the same limit check and pending-trigger reservations, and the cache is updated when the transaction succeeds. On failure, nothing is imported: the reservations are released, the connection is checked like any failed query, and the admin is told. It also no longer callsGetClientUserId(0)when run from the server console.CustomChatColors.sp): the MySQLON DUPLICATE KEY UPDATEofccc_replace,ccc_tagandccc_banre-assigned the primary key and repeated every value as a second set of escaped parameters (twice the formatting and query size). They now usecol = VALUES(col)without the PK column. The SQLiteREPLACE INTOpaths are unchanged. The replace INSERT is shared bySQLInsert_Replaceand the import throughSQL_FormatInsertReplace.Not included
SQL_ScheduleRetry) still useTIMER_FLAG_NO_MAPCHANGEand are dropped on map end, as before.Test plan
sm_ccc_db_connect_delay> 0: the delay only applies to the first connect, not to later maps.KILL <id>), trigger a query: dropped and reconnected by the existing logic, also across a map change while the reconnect is pending.sm_cccaddtag), thenKILLthe connection so the driver auto-reconnects, set it again: stored correctly.sm_cccimportreplacefile custom-chatcolorsreplace.cfgfrom chat and from the server console: one transaction in the general log, triggers work right away. Import again (all duplicates): values updated, no duplicate key error.sm_cccaddtag/sm_cccbanon existing rows (MySQL and SQLite): rows updated.