Skip to content

Fixing issues with PostgreSQL#3289

Open
magaflaca wants to merge 3 commits intoPryaxis:general-develfrom
magaflaca:postgres-fix
Open

Fixing issues with PostgreSQL#3289
magaflaca wants to merge 3 commits intoPryaxis:general-develfrom
magaflaca:postgres-fix

Conversation

@magaflaca
Copy link
Copy Markdown

Summary

Fixes several PostgreSQL compatibility issues in TShock database code.

This PR updates SQL generation and direct queries so PostgreSQL no longer receives MySQL-style backtick-quoted identifiers. It fixes region creation and region group updates on PostgreSQL by escaping the Groups column according to the active SQL backend.

It also fixes PostgreSQL ban insertion by returning TicketNumber instead of "Identifier", and improves the legacy Bans table lookup by checking the current schema and using a case-insensitive table name match.

The generic query builder now supports dialect-specific column escaping, uses case-insensitive column matching during table alteration, and fixes BuildWhere so generated WHERE clauses are emitted correctly.

Changelog

  • Fixed PostgreSQL syntax errors caused by MySQL-style backtick-quoted column names in region queries.
  • Fixed PostgreSQL region group insert/select/update queries for the Groups column.
  • Fixed PostgreSQL ban insertion returning the wrong column after insert.
  • Improved PostgreSQL legacy Bans table detection.
  • Added dialect-specific column escaping to query builders.
  • Fixed generated WHERE clause handling in the generic query builder.

Testing

Tested against a PostgreSQL-backed TShock server by creating regions with /region define. The previous PostgreSQL syntax error near backticks no longer occurs.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 5, 2026

Greptile Summary

This PR fixes several PostgreSQL compatibility issues by replacing MySQL-style backtick quoting with dialect-aware identifier escaping, correcting the inverted BuildWhere ternary, and fixing the PostgreSQL ban RETURNING clause to reference the actual primary-key column.

  • GenericQueryBuilder: BuildWhere had its ternary branches swapped — it returned an empty string when conditions existed and tried to generate a WHERE clause when there were none. Both branches are now correct, and the join separator was fixed from , to AND.
  • RegionManager / query builders: Hardcoded MySQL backticks on the Groups column are replaced by a GroupsColumnName property and per-dialect EscapeColumnName overrides. The PostgreSQL and MySQL overrides are correct; the SQLite override uses single-quoted strings ('col'), which SQL treats as string literals rather than identifiers, causing silent data corruption during AlterTable schema migrations.
  • BanManager: PostgreSQL ban insertion now correctly returns TicketNumber instead of \"Identifier\", and the legacy Bans table lookup is scoped to current_schema() with a case-insensitive ILIKE match.

Confidence Score: 3/5

Safe to merge for PostgreSQL users; SQLite schema migrations will silently replace all column values with the column name string until the single-quote escaping is corrected.

The PostgreSQL and MySQL fixes are solid and address real failures. However, the SQLite EscapeColumnName implementation returns single-quoted strings which SQL treats as literals not identifiers. Every AlterTable schema migration on SQLite will silently overwrite all row data with the column name strings themselves — a data-destructive regression for anyone running TShock on SQLite whose schema needs to evolve.

TShockAPI/DB/Queries/SqliteQueryBuilder.cs — the new EscapeColumnName override needs double-quote or backtick quoting, not single-quote quoting.

Important Files Changed

Filename Overview
TShockAPI/DB/Queries/SqliteQueryBuilder.cs Adds EscapeColumnName using single-quoted strings, which are SQL string literals; this corrupts all column data during SQLite schema migrations via AlterTable.
TShockAPI/DB/Queries/GenericQueryBuilder.cs Fixes the inverted ternary in BuildWhere, adds virtual EscapeColumnName, and switches column matching to case-insensitive — all correct changes.
TShockAPI/DB/Queries/PostgresQueryBuilder.cs Adds EscapeColumnName with lowercase + double-quote escaping and applies it to CreateTable column and UNIQUE definitions — correct PostgreSQL identifier handling.
TShockAPI/DB/Queries/MysqlQueryBuilder.cs Adds EscapeColumnName with backtick quoting, consistent with existing MySQL identifier escaping convention.
TShockAPI/DB/BanManager.cs Fixes PostgreSQL ban insertion (RETURNING TicketNumber replaces the wrong RETURNING Identifier) and improves legacy Bans table detection with current_schema() + ILIKE; the unquoted TicketNumber in RETURNING works but is inconsistent with the builder's quoting convention.
TShockAPI/DB/RegionManager.cs Replaces hardcoded MySQL backtick quoting of the Groups column with a dialect-aware GroupsColumnName property used in INSERT, SELECT, and UPDATE queries.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    QB[GenericQueryBuilder] -->|virtual| ECN[EscapeColumnName]
    ECN -->|override| MYSQL["MysqlQueryBuilder\n` backtick `"]
    ECN -->|override| PG["PostgresQueryBuilder\nlowercase double-quote"]
    ECN -->|override| SQLITE["SqliteQueryBuilder\nsingle-quote string literal"]
    QB -->|uses ECN| ALT[AlterTable\nINSERT INTO t cols SELECT cols FROM tmp]
    PG -->|also used in| CT[CreateTable\ncolumn defs + UNIQUE list]
    BW[BuildWhere\nfixed inverted ternary] -->|emits| WHERE["WHERE col = val AND ..."]
    BW -->|used by| DQ[DeleteRow / UpdateValue / ReadColumn]
Loading

Reviews (1): Last reviewed commit: "Fixing issues with PostgreSQL" | Re-trigger Greptile

}

/// <inheritdoc />
protected override string EscapeColumnName(string column) => $"\'{column}\'";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Single-quoted column names corrupt data during schema migration

In SQL, single quotes denote string literals, not identifiers. When AlterTable generates its copy query it reuses the column list in both positions: INSERT INTO 'tablename' ('col1', 'col2') SELECT 'col1', 'col2' FROM 'tmpTableName'. SQLite resolves 'col1' in the SELECT expression as a string constant, so every row in the migrated table ends up with the column name as its value instead of the actual data. The identifier-escaping character for SQLite is " (double-quote, per SQL standard and the SQLite docs) or backtick — single quotes are not valid here.

Suggested change
protected override string EscapeColumnName(string column) => $"\'{column}\'";
/// <inheritdoc />
protected override string EscapeColumnName(string column) => $"\"{column}\"";

Comment thread TShockAPI/DB/BanManager.cs Outdated
SqlType.Mysql => /*lang=mysql*/"; SELECT LAST_INSERT_ID();",
SqlType.Sqlite => /*lang=sqlite*/"; SELECT last_insert_rowid();",
SqlType.Postgres => /*lang=postgresql*/"RETURNING \"Identifier\";",
SqlType.Postgres => /*lang=postgresql*/" RETURNING TicketNumber;",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 The RETURNING clause should quote TicketNumber consistently with how PostgresQueryBuilder.EscapeColumnName stores the column (lowercased, double-quoted). While PostgreSQL folds unquoted TicketNumber to ticketnumber at runtime and the match succeeds today, using the same quoting convention eliminates the silent dependency on case-folding.

Suggested change
SqlType.Postgres => /*lang=postgresql*/" RETURNING TicketNumber;",
SqlType.Postgres => /*lang=postgresql*/" RETURNING \"ticketnumber\";",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant