Skip to content

InsertBinaryAsync / InsertRawStreamAsync concatenate the raw table name into SQL — names needing backquotes cannot be used #602

Description

@claude

Description

ClickHouseClient's insert paths take a table string and splice it straight into the generated SQL, with no identifier quoting. A perfectly legal ClickHouse table name that requires backquotes (my-table, user events, a name starting with a digit, …) therefore fails with a server-side SYNTAX_ERROR even though the table exists — FROM my-table parses as my minus table.

Affected call sites (all on main):

  • ClickHouse.Driver/Utility/SchemaResolver.cs:91-92 — the schema probe: $"SELECT {columnsExpr} FROM {table} WHERE 1=0"
  • ClickHouse.Driver/ClickHouseClient.cs:691$"INSERT INTO {table} ({string.Join(", ", columnNames)}) FORMAT {options.Format}"
  • ClickHouse.Driver/ClickHouseClient.cs:899-900InsertRawStreamAsync: $"INSERT INTO {table} {columnList} FORMAT {format}", and here the columns are unquoted too (string.Join(", ", columns)), unlike every other insert path

The inconsistency is the tell: column names on the InsertBinaryAsync path already go through StringExtensions.EncloseColumnName() (SchemaResolver.cs:88, 94, 123, 174), and SchemaResolver.BuildCacheKey encloses both the database and the table name for the cache key — but the table name that actually reaches the SQL is never enclosed. EncloseColumnName() already implements the backward-compatible "pass through if already enclosed" contract, so the helper needed for a fix exists and is used a few lines away.

Practical impact: callers must know to pre-quote the argument themselves (client.InsertBinaryAsync("\my-table`", …)` works), which is undocumented — the XML docs describe the parameter as "The destination table name" / "Table name", not as a SQL fragment.

ClickHouse server version

26.8.1.2041 (official build), reached over HTTP at localhost:8123.

Reproduction

NUnit test in ClickHouse.Driver.Tests (run with dotnet test ClickHouse.Driver.Tests/ClickHouse.Driver.Tests.csproj -f net10.0 --filter "FullyQualifiedName~QuotedTableName"):

[TestFixture]
public class QuotedTableNameTests : AbstractConnectionTestFixture
{
    private const string BareName = "scratch-quoted-table";

    [Test]
    public async Task InsertBinaryAsync_TableNameNeedingBackquotes_ShouldWork()
    {
        await client.ExecuteNonQueryAsync($"DROP TABLE IF EXISTS `{BareName}`");
        await client.ExecuteNonQueryAsync(
            $"CREATE TABLE `{BareName}` (id UInt64, value String) ENGINE = MergeTree() ORDER BY id");
        try
        {
            var rows = new List<object[]> { new object[] { 1UL, "a" } };
            await client.InsertBinaryAsync(BareName, new[] { "id", "value" }, rows);
            var count = await client.ExecuteScalarAsync($"SELECT count() FROM `{BareName}`");
            Assert.That(count, Is.EqualTo(1UL));
        }
        finally
        {
            await client.ExecuteNonQueryAsync($"DROP TABLE IF EXISTS `{BareName}`");
        }
    }

    // Control: the same insert succeeds when the caller pre-quotes the name.
    [Test]
    public async Task InsertBinaryAsync_PreQuotedTableName_ShouldWork()
    {
        await client.ExecuteNonQueryAsync($"DROP TABLE IF EXISTS `{BareName}2`");
        await client.ExecuteNonQueryAsync(
            $"CREATE TABLE `{BareName}2` (id UInt64, value String) ENGINE = MergeTree() ORDER BY id");
        try
        {
            var rows = new List<object[]> { new object[] { 1UL, "a" } };
            await client.InsertBinaryAsync($"`{BareName}2`", new[] { "id", "value" }, rows);
            var count = await client.ExecuteScalarAsync($"SELECT count() FROM `{BareName}2`");
            Assert.That(count, Is.EqualTo(1UL));
        }
        finally
        {
            await client.ExecuteNonQueryAsync($"DROP TABLE IF EXISTS `{BareName}2`");
        }
    }
}

Expected: both tests pass — one row inserted into the existing table.

Actual: Failed: 1, Passed: 1. The pre-quoted control passes; the bare-name test throws from the schema probe before any data is sent:

ClickHouse.Driver.ClickHouseServerException : Code: 62. DB::Exception: Syntax error:
failed at position 33 (-): -quoted-table WHERE 1=0. Expected one of: ... (SYNTAX_ERROR)
   at ClickHouse.Driver.Utility.SchemaResolver.LoadAsync(...) SchemaResolver.cs:line 92
   at ClickHouse.Driver.Utility.SchemaResolver.ResolveAsync(...) SchemaResolver.cs:line 79
   at ClickHouse.Driver.ClickHouseClient.PrepareInsertAsync(...) ClickHouseClient.cs:line 682
   at ClickHouse.Driver.ClickHouseClient.InsertBinaryAsync(...) ClickHouseClient.cs:line 835

Supplying InsertOptions.ColumnTypes skips the probe, but the insert then fails identically on INSERT INTO scratch-quoted-table (…) built at ClickHouseClient.cs:691.

Suggested fix

Settle the contract for the table parameter, mirroring what the upstream Java issue asks for:

  1. Treat it as a raw identifier (what the docs imply): run it through EncloseColumnName() at the three sites above. That helper's already-enclosed pass-through preserves today's pre-quoting workaround, so existing callers keep working. A dotted db.table argument is the wrinkle to decide on — SchemaResolver.BuildCacheKey and the InsertBinarySchemaTests comments show qualified names are an accepted input shape, and blanket-enclosing would turn db.table into `db.table`; splitting on an unquoted dot, or enclosing each part, would be needed. This is also worth deciding for InsertOptions.Database.
  2. Or document it as a SQL fragment and state in the XML docs on InsertBinaryAsync, InsertRawStreamAsync and InsertOptions.Database that the caller must quote names that need it.

Either way, InsertRawStreamAsync's unquoted column list (ClickHouseClient.cs:899) looks like an oversight relative to the other insert paths and should use EncloseColumnName().

Note that #316 (EncloseColumnName skips escaping when input starts and ends with the quote char) interacts with option 1: a name like `weird` that legitimately begins and ends with a backtick is passed through unescaped. That is a separate defect, but a fix here would inherit it.

Link

Same bug reported upstream for clickhouse-java client-v2: ClickHouse/clickhouse-java#3089

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions