diff --git a/CHANGELOG.md b/CHANGELOG.md index f5987c3..46ffd65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ - Every tool parameter now has a short description listing allowed values or defaults (`section`, `format`, `order_by`, `limit` and its `max_rows` cap, `offset`, `target`, …), so agents no longer discover them from error round trips. The restricted `tools/list` array grows from 6,780 to 10,516 bytes. - SQL errors are now actionable without exposing data values. Errors for statements submitted to `execute_sql` and `explain_query` include `position`, PostgreSQL's 1-based character position mapped into the submitted statement (previously absent; the server's paging/EXPLAIN wrapper would also have shifted it). SQLSTATE class `42` errors (syntax, undefined/ambiguous objects, privileges) that PostgreSQL positions inside the submitted statement, and `42501` privilege errors, add `server_message` and `server_hint` when not raised inside a routine or internal query; execution-time class `42` errors that can echo row values (`current_setting(col)`, `col::regclass`, `query_to_xml`) stay withheld. Class `23` integrity errors not raised from a routine add `constraint`. All other PostgreSQL messages, hints and details remain withheld, and the fallback summary no longer claims the message is withheld when `server_message` is returned. Existing `code`, `message` and `sql_state` fields are unchanged. - `execute_sql` and `explain_query` accept optional `parameters`: up to 256 JSON scalars bound to `$1..$n`, never interpolated into the SQL. Each value is sent as PostgreSQL `unknown`, so its type is inferred from context like a quoted literal; cast when ambiguous. Objects and arrays return `invalid_parameters`, and new fixed summaries cover too few values (`08P01`) and an undeterminable parameter type (`42P18`, which also results from extra values or a skipped `$n`). Paging with `next_offset` must resend the same `sql` and `parameters`. Existing calls are unchanged. +- Performance: every tool call saves one network round trip. The transaction setup (`SET TRANSACTION READ ONLY`/`READ WRITE`, `standard_conforming_strings`, statement and lock timeouts) now travels in the same batch as the first statement. With 25 ms added to each server response, median `execute_sql` latency fell from 106.8 ms to 80.8 ms. `SqlSession` no longer exposes its connection, transaction or any batch: it builds and owns every batch itself and prepends any pending setup, so no caller can run SQL before the transaction mode is set. - Show a grouped README badge table covering build/tests, security scanning, supply chain, registries, platforms, stack versions, features and project activity. Feature and practice badges link to the documentation or configuration that supports them; stack versions are read live from `global.json` and `Directory.Packages.props`. - Release npm job: install npm 12.2.0 from its registry tarball after a pinned SHA-512 check matching the published `dist.integrity`, replacing the unpinned `npm install --global` flagged by Scorecard Pinned-Dependencies. - Release npm job: accept npm 12's one-element JSON array from `npm view @ version --json`. Previously an already-published version failed with "requires an element of type 'String'" instead of being reported as not republished. diff --git a/README.md b/README.md index b382137..c062934 100644 --- a/README.md +++ b/README.md @@ -510,6 +510,8 @@ An extended run added 3,200 successful queries, 50 errors, 50 JSON plans and one For one synthetic 32-row response over 2,000 warmed serialization calls, allocated bytes per response fell from 36,304 to 27,736 (23.6%) after replacing the temporary JSON document/clone round trip with `SerializeToElement`. This is an allocation measurement, not a throughput or model-token claim. +Each tool call sends the transaction setup (`SET TRANSACTION READ ONLY`/`READ WRITE` and timeouts) in the same network batch as its first statement, so a single-statement call costs two round trips (setup + statement, then `ROLLBACK`/`COMMIT`) instead of three. Through a local proxy adding 25 ms to each server response, the median `execute_sql` latency fell from 106.8 ms to 80.8 ms (60 calls each); savings scale with the network round-trip time and are not a throughput claim. + SDK/provider payload logging is disabled even at debug/trace levels; host diagnostics stay on stderr. Query results, metadata and workload text may still contain sensitive data readable by the role. This is not general-purpose data redaction. See [SECURITY.md](SECURITY.md). diff --git a/src/PostgreSqlMcp.Core/DatabaseLockCheck.cs b/src/PostgreSqlMcp.Core/DatabaseLockCheck.cs index fa27ed2..08be00f 100644 --- a/src/PostgreSqlMcp.Core/DatabaseLockCheck.cs +++ b/src/PostgreSqlMcp.Core/DatabaseLockCheck.cs @@ -106,10 +106,11 @@ private static Task> ReadTextAsync(SqlExecutor executor, string dat IReadOnlyDictionary? parameters, CancellationToken ct) => executor.WithSessionAsync(database, async (session, token) => { - await using var command = new NpgsqlCommand(sql, session.Connection, session.Transaction); + var command = new NpgsqlBatchCommand(sql); if (parameters is not null) foreach (var (name, value) in parameters) command.Parameters.AddWithValue(name, value ?? DBNull.Value); - await using var reader = await command.ExecuteReaderAsync(token).ConfigureAwait(false); + await using SessionReader result = await session.ExecuteReaderAsync([command], System.Data.CommandBehavior.Default, token).ConfigureAwait(false); + NpgsqlDataReader reader = result.Reader; var values = new List(); while (await reader.ReadAsync(token).ConfigureAwait(false)) values.Add(reader.GetString(0)); return values; diff --git a/src/PostgreSqlMcp.Core/SecurityWarnings.cs b/src/PostgreSqlMcp.Core/SecurityWarnings.cs index 64b5600..cf5a2f4 100644 --- a/src/PostgreSqlMcp.Core/SecurityWarnings.cs +++ b/src/PostgreSqlMcp.Core/SecurityWarnings.cs @@ -71,8 +71,9 @@ public static async Task> RolesAsync(DatabaseRegistry regi { List found = await executor.WithSessionAsync(registry.GetDiscoveryDatabase(target), async (session, token) => { - await using var command = new NpgsqlCommand(RoleSql, session.Connection, session.Transaction); - await using var reader = await command.ExecuteReaderAsync(token).ConfigureAwait(false); + await using SessionReader result = await session.ExecuteReaderAsync([new NpgsqlBatchCommand(RoleSql)], + System.Data.CommandBehavior.Default, token).ConfigureAwait(false); + NpgsqlDataReader reader = result.Reader; var values = new List(); while (await reader.ReadAsync(token).ConfigureAwait(false)) values.Add(reader.GetString(0)); return values; diff --git a/src/PostgreSqlMcp.Core/SqlExecutor.cs b/src/PostgreSqlMcp.Core/SqlExecutor.cs index 49efa38..336a016 100644 --- a/src/PostgreSqlMcp.Core/SqlExecutor.cs +++ b/src/PostgreSqlMcp.Core/SqlExecutor.cs @@ -30,11 +30,15 @@ public async Task WithSessionAsync(string database, Func WithSessionAsync(string database, Func _calls.Dispose(); } -public sealed class SqlSession(string database, NpgsqlConnection connection, NpgsqlTransaction transaction, - ServerOptions options, bool readOnly) +/// +/// One server-owned transaction. The connection, transaction and batches are not exposed: every statement goes through +/// , which builds the batch itself and prepends the pending transaction setup +/// (READ ONLY/READ WRITE and timeouts) to the first one, so no caller can run SQL before the transaction mode is set. +/// +public sealed class SqlSession { - public NpgsqlConnection Connection { get; } = connection; - public NpgsqlTransaction Transaction { get; } = transaction; + private readonly string database; + private readonly NpgsqlConnection connection; + private readonly NpgsqlTransaction transaction; + private readonly ServerOptions options; + private readonly bool readOnly; + private string[]? pendingSetup; + + internal SqlSession(string database, NpgsqlConnection connection, NpgsqlTransaction transaction, + ServerOptions options, bool readOnly, string[] setup) + { + this.database = database; + this.connection = connection; + this.transaction = transaction; + this.options = options; + this.readOnly = readOnly; + pendingSetup = setup; + } + + /// + /// Runs as one batch, preceded by the transaction setup if it has not reached the server yet. + /// Dispose the result to release the reader and the batch. + /// + public async Task ExecuteReaderAsync(IReadOnlyList commands, CommandBehavior behavior, + CancellationToken ct, int? timeout = null) + { + var batch = new NpgsqlBatch(connection, transaction) { Timeout = timeout ?? options.QueryTimeout }; + // Each batch command is one extended-protocol statement, so the setup is one command per statement. + string[]? setup = pendingSetup; + if (setup is not null) + foreach (string statement in setup) batch.BatchCommands.Add(new NpgsqlBatchCommand(statement)); + foreach (NpgsqlBatchCommand command in commands) batch.BatchCommands.Add(command); + try + { + NpgsqlDataReader reader = await batch.ExecuteReaderAsync(behavior, ct).ConfigureAwait(false); + pendingSetup = null; + return new SessionReader(batch, reader); + } + catch (PostgresException) when (setup is not null) + { + // The server processed the batch: either the setup ran or the transaction is aborted. Otherwise + // (cancelled before sending, I/O failure) the setup stays pending for any later statement. + pendingSetup = null; + await batch.DisposeAsync().ConfigureAwait(false); + throw; + } + catch + { + await batch.DisposeAsync().ConfigureAwait(false); + throw; + } + } + + /// Runs one fixed control statement and discards its result. + public async Task ExecuteAsync(string sql, CancellationToken ct, int? timeout = null) + { + await using SessionReader result = await ExecuteReaderAsync([new NpgsqlBatchCommand(sql)], CommandBehavior.Default, ct, timeout).ConfigureAwait(false); + } + + /// Marks this session's pooled connections for disposal instead of reuse. + public void ClearPool() => NpgsqlConnection.ClearPool(connection); /// The SQL came from the tool caller, so PostgreSQL error positions are reported relative to it. /// Caller values for $1..$n (see ); exclusive with named parameters. @@ -88,12 +154,13 @@ public async Task QueryAsync(string sql, IReadOnlyDictionary ReadAsync(string sql, IReadOnlyDictionary? parameters, IReadOnlyList? positional, int rowLimit, int offset, bool serverPage, CancellationToken ct) { - await using var command = new NpgsqlCommand(sql, Connection, Transaction) { CommandTimeout = options.QueryTimeout }; + var command = new NpgsqlBatchCommand(sql); if (parameters is not null) foreach (var (name, value) in parameters) command.Parameters.AddWithValue(name, value ?? DBNull.Value); else if (positional is not null) SqlParameters.Bind(command.Parameters, positional); - await using var reader = await command.ExecuteReaderAsync(CommandBehavior.SequentialAccess, ct).ConfigureAwait(false); + await using SessionReader result = await ExecuteReaderAsync([command], CommandBehavior.SequentialAccess, ct).ConfigureAwait(false); + NpgsqlDataReader reader = result.Reader; if (reader.FieldCount > 128) throw new ToolException("result_too_wide", "Select at most 128 columns."); var columns = new ColumnInfo[reader.FieldCount]; for (int col = 0; col < columns.Length; col++) @@ -187,3 +254,23 @@ private async Task ReadAsync(string sql, IReadOnlyDictionaryA result reader that owns the batch it reads from; disposing it releases both. +public sealed class SessionReader : IAsyncDisposable +{ + private readonly NpgsqlBatch batch; + + internal SessionReader(NpgsqlBatch batch, NpgsqlDataReader reader) + { + this.batch = batch; + Reader = reader; + } + + public NpgsqlDataReader Reader { get; } + + public async ValueTask DisposeAsync() + { + try { await Reader.DisposeAsync().ConfigureAwait(false); } + finally { await batch.DisposeAsync().ConfigureAwait(false); } + } +} diff --git a/src/PostgreSqlMcp.Tools/PlanTools.cs b/src/PostgreSqlMcp.Tools/PlanTools.cs index c80c744..b1e699a 100644 --- a/src/PostgreSqlMcp.Tools/PlanTools.cs +++ b/src/PostgreSqlMcp.Tools/PlanTools.cs @@ -66,31 +66,34 @@ public Task ExplainQuery(CancellationToken ct, using JsonDocument baseline = await ReadPlan(session, statement, values, false, token); var hypotheticalIndexes = new List(); // Candidates are independent; send one batch rather than one round trip per index. - await using var batch = new NpgsqlBatch(session.Connection, session.Transaction) { Timeout = options.QueryTimeout }; + var commands = new List(candidates.Length); foreach (string candidate in candidates) { var command = new NpgsqlBatchCommand($"SELECT indexrelid::bigint, indexname FROM {ns}.hypopg_create_index(@candidate)"); command.Parameters.AddWithValue("candidate", candidate); - batch.BatchCommands.Add(command); + commands.Add(command); } - await using (var reader = await batch.ExecuteReaderAsync(CommandBehavior.SequentialAccess, token)) - for (int candidate = 0; candidate < candidates.Length; candidate++) + await using (SessionReader result = await session.ExecuteReaderAsync(commands, CommandBehavior.SequentialAccess, token)) { - int created = 0; - while (await reader.ReadAsync(token)) + NpgsqlDataReader reader = result.Reader; + for (int candidate = 0; candidate < candidates.Length; candidate++) { - if (++created > 1) - throw new ToolException("invalid_index", "Each candidate must describe exactly one CREATE INDEX statement."); - long oid = reader.GetInt64(0); - string name = reader.GetString(1); - if (name.Length > options.MaxCellChars) - throw new ToolException("result_too_large", "Hypothetical index name exceeds the result cell limit; simplify the candidate."); - hypotheticalIndexes.Add(new(candidate, oid, name)); + int created = 0; + while (await reader.ReadAsync(token)) + { + if (++created > 1) + throw new ToolException("invalid_index", "Each candidate must describe exactly one CREATE INDEX statement."); + long oid = reader.GetInt64(0); + string name = reader.GetString(1); + if (name.Length > options.MaxCellChars) + throw new ToolException("result_too_large", "Hypothetical index name exceeds the result cell limit; simplify the candidate."); + hypotheticalIndexes.Add(new(candidate, oid, name)); + } + if (created != 1) + throw new ToolException("invalid_index", $"Candidate {candidate} did not create a hypothetical index. Supply a supported CREATE INDEX definition."); + if (candidate + 1 < candidates.Length && !await reader.NextResultAsync(token)) + throw new ToolException("invalid_index", "HypoPG did not return a result for each candidate."); } - if (created != 1) - throw new ToolException("invalid_index", $"Candidate {candidate} did not create a hypothetical index. Supply a supported CREATE INDEX definition."); - if (candidate + 1 < candidates.Length && !await reader.NextResultAsync(token)) - throw new ToolException("invalid_index", "HypoPG did not return a result for each candidate."); } using JsonDocument evaluated = await ReadPlan(session, statement, values, false, token); PlanSummary before = Summarize(baseline.RootElement); @@ -129,7 +132,7 @@ public Task ExplainQuery(CancellationToken ct, catch { // Clearing marks checked-out connectors for disposal on return to the pool. - NpgsqlConnection.ClearPool(session.Connection); + session.ClearPool(); if (!failed) throw new ToolException("hypopg_cleanup_failed", "Hypothetical index cleanup failed. The connection pool was cleared so this session cannot leak candidates to later calls."); } @@ -165,13 +168,15 @@ private string[] ValidateCandidates(string[]? indexes) private async Task ReadPlan(SqlSession session, string sql, IReadOnlyList? values, bool analyze, CancellationToken ct) { string prefix = analyze ? "EXPLAIN (FORMAT JSON, ANALYZE TRUE, BUFFERS TRUE, VERBOSE FALSE) " : "EXPLAIN (FORMAT JSON, VERBOSE FALSE) "; - await using var command = Command(session, prefix + sql); + var command = new NpgsqlBatchCommand(prefix + sql); // Bound values give PostgreSQL a custom plan for these values, as for the same statement with literals. if (values is not null) SqlParameters.Bind(command.Parameters, values); - NpgsqlDataReader reader; - try { reader = await command.ExecuteReaderAsync(CommandBehavior.SequentialAccess | CommandBehavior.SingleRow, ct); } + // EXPLAIN returns one row; SingleRow is not used because the batch may begin with the session's setup commands. + SessionReader result; + try { result = await session.ExecuteReaderAsync([command], CommandBehavior.SequentialAccess, ct); } catch (PostgresException ex) when (ToolReply.MarkStatementPosition(ex, prefix.Length, sql)) { throw; } - await using var _ = reader; + await using var _ = result; + NpgsqlDataReader reader = result.Reader; if (!await reader.ReadAsync(ct) || reader.IsDBNull(0)) throw new ToolException("invalid_plan", "PostgreSQL returned no JSON plan."); using TextReader text = await reader.GetTextReaderAsync(0, ct); @@ -208,14 +213,8 @@ private async Task ReadPlan(SqlSession session, string sql, IReadO private ToolException OversizedPlan() => new("plan_too_large", $"Planner JSON exceeds MaxResultBytes ({options.MaxResultBytes}); no plan was silently truncated. Narrow the query or explain a smaller subquery, or ask the operator to increase the byte limit. Summary also requires reading the bounded complete planner JSON."); - private NpgsqlCommand Command(SqlSession session, string sql, int? timeout = null) => - new(sql, session.Connection, session.Transaction) { CommandTimeout = timeout ?? options.QueryTimeout }; - - private async Task ExecuteControl(SqlSession session, string sql, CancellationToken ct, int? timeout = null) - { - await using var command = Command(session, sql, timeout); - await command.ExecuteNonQueryAsync(ct); - } + private static Task ExecuteControl(SqlSession session, string sql, CancellationToken ct, int? timeout = null) => + session.ExecuteAsync(sql, ct, timeout); private static string QuoteIdentifier(string value) => "\"" + value.Replace("\"", "\"\"") + "\"";