Skip to content

fix(sql): keep the DB connection across maps, batch replace import - #106

Merged
Rushaway merged 1 commit into
masterfrom
fix/sql-audit
Oct 29, 2026
Merged

Rushaway merged 1 commit into
masterfrom
fix/sql-audit

Conversation

@Rushaway

@Rushaway Rushaway commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Reconnect on every map (CustomChatColors.sp): OnMapEnd deleted g_hDatabase on every map change and OnConfigsExecuted opened a new one. Each map paid for a new connection, SET NAMES, 3x CREATE 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 with sm_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: OnMapEnd returns early when the state is Connected and 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. OnConfigsExecuted only 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.
  • Charset (CustomChatColors.sp): the charset was only set with an async SET NAMES. That is lost when the driver auto-reconnects, which matters more now that the connection lives for the whole server uptime. OnSQLConnected now calls db.SetCharset("utf8mb4") once per new MySQL connection, before any query is queued (it stays in the connection options). SET NAMES is kept only as fallback when SetCharset fails, and the startup gate is unchanged.
  • Import N+1 (CustomChatColors.sp): sm_cccimportreplacefile sent one INSERT per KeyValues entry (up to 255 round trips). The entries now go in one Transaction. 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 calls GetClientUserId(0) when run from the server console.
  • Upserts (CustomChatColors.sp): the MySQL ON DUPLICATE KEY UPDATE of ccc_replace, ccc_tag and ccc_ban re-assigned the primary key and repeated every value as a second set of escaped parameters (twice the formatting and query size). They now use col = VALUES(col) without the PK column. The SQLite REPLACE INTO paths are unchanged. The replace INSERT is shared by SQLInsert_Replace and the import through SQL_FormatInsertReplace.
  • Version 8.0.23 -> 8.0.24.

Not included

  • The per-request retry timers (SQL_ScheduleRetry) still use TIMER_FLAG_NO_MAPCHANGE and are dropped on map end, as before.

Test plan

  • Compiles with spcomp 1.12.0.7259 (same deps as CI)
  • CI build passes
  • Change map a few times: "Connected to database." only once in the logs, the MySQL processlist keeps the same connection id, tags load right after the map change.
  • With sm_ccc_db_connect_delay > 0: the delay only applies to the first connect, not to later maps.
  • Kill the plugin's connection (KILL <id>), trigger a query: dropped and reconnected by the existing logic, also across a map change while the reconnect is pending.
  • Tag with emoji (sm_cccaddtag), then KILL the connection so the driver auto-reconnects, set it again: stored correctly.
  • sm_cccimportreplacefile custom-chatcolorsreplace.cfg from 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_cccban on existing rows (MySQL and SQLite): rows updated.

- 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>
Copilot AI balanced review requested due to automatic review settings October 28, 2026 14:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@WareZ57 WareZ57 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_Replace and 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.

@Rushaway
Rushaway merged commit e5b54d0 into master Oct 29, 2026
9 checks passed
@Rushaway
Rushaway deleted the fix/sql-audit branch October 29, 2026 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants