Here's some perf data for this PR.

tl;dr I'm seeing 2.5%-3% RPS improvement in the Fortunes Platform benchmark. Memory allocations are down 33% in number of allocations, 17% in bytes.

RPS runs

dotnet run -- --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/platform.benchmarks.yml --scenario fortunes --profile aspnet-citrine-lin --application.framework net6.0 --application.options.outputFiles Z:\projects\npgsql\src\Npgsql\bin\Release\net6.0\Npgsql.dll --variable warmup=45 --variable duration=120

| db                  |         |
| ------------------- | ------- |
| CPU Usage (%)       | 39      |
| Cores usage (%)     | 1,093   |
| Working Set (MB)    | 45      |
| Build Time (ms)     | 1,825   |
| Start Time (ms)     | 358     |
| Published Size (KB) | 914,277 |


| application           |                                 |
| --------------------- | ------------------------------- |
| CPU Usage (%)         | 96                              |
| Cores usage (%)       | 2,688                           |
| Working Set (MB)      | 463                             |
| Private Memory (MB)   | 1,582                           |
| Build Time (ms)       | 2,590                           |
| Start Time (ms)       | 1,585                           |
| Published Size (KB)   | 90,962                          |
| .NET Core SDK Version | 6.0.100-preview.6.21276.12      |
| ASP.NET Core Version  | 6.0.0-preview.6.21313.1+56ddfd2 |
| .NET Runtime Version  | 6.0.0-preview.6.21313.1+4bb1318 |


| load                   |            |
| ---------------------- | ---------- |
| CPU Usage (%)          | 31         |
| Cores usage (%)        | 872        |
| Working Set (MB)       | 37         |
| Private Memory (MB)    | 363        |
| Start Time (ms)        | 0          |
| First Request (ms)     | 73         |
| Requests/sec           | 479,858    |
| Requests               | 57,630,609 |
| Mean latency (ms)      | 1.12       |
| Max latency (ms)       | 21.80      |
| Bad responses          | 0          |
| Socket errors          | 0          |
| Read throughput (MB/s) | 622.83     |
| Latency 50th (ms)      | 1.01       |
| Latency 75th (ms)      | 1.21       |
| Latency 90th (ms)      | 1.50       |
| Latency 99th (ms)      | 3.80       |

dotnet run -- --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/platform.benchmarks.yml --scenario fortunes --profile aspnet-citrine-lin --application.framework net6.0 --application.options.outputFiles Z:\projects\npgsql\src\Npgsql\bin\Release\net6.0\Npgsql.dll --variable warmup=45 --variable duration=120

| db                  |         |
| ------------------- | ------- |
| CPU Usage (%)       | 39      |
| Cores usage (%)     | 1,100   |
| Working Set (MB)    | 45      |
| Build Time (ms)     | 1,702   |
| Start Time (ms)     | 358     |
| Published Size (KB) | 914,277 |


| application           |                                 |
| --------------------- | ------------------------------- |
| CPU Usage (%)         | 96                              |
| Cores usage (%)       | 2,699                           |
| Working Set (MB)      | 453                             |
| Private Memory (MB)   | 1,581                           |
| Build Time (ms)       | 2,629                           |
| Start Time (ms)       | 1,602                           |
| Published Size (KB)   | 90,962                          |
| .NET Core SDK Version | 6.0.100-preview.6.21276.12      |
| ASP.NET Core Version  | 6.0.0-preview.6.21313.1+56ddfd2 |
| .NET Runtime Version  | 6.0.0-preview.6.21313.1+4bb1318 |


| load                   |            |
| ---------------------- | ---------- |
| CPU Usage (%)          | 32         |
| Cores usage (%)        | 897        |
| Working Set (MB)       | 38         |
| Private Memory (MB)    | 363        |
| Start Time (ms)        | 0          |
| First Request (ms)     | 74         |
| Requests/sec           | 492,215    |
| Requests               | 59,114,682 |
| Mean latency (ms)      | 1.09       |
| Max latency (ms)       | 34.83      |
| Bad responses          | 0          |
| Socket errors          | 0          |
| Read throughput (MB/s) | 638.87     |
| Latency 50th (ms)      | 0.99       |
| Latency 75th (ms)      | 1.18       |
| Latency 90th (ms)      | 1.46       |
| Latency 99th (ms)      | 3.91       |

Memory profiling results (for 50k iterations):

/cc @sebastienros @benaadams @ajcvickers

tl;dr I'm seeing 2.5%-3% RPS improvement in the Fortunes Platform benchmark. Memory allocations are down 33% in number of allocations, 17% in bytes.

What about the Multiple queries test as that runs 20 queries sequentially (also 21% of composite score)

Did a fairly thorough review of the changes, not sure yet why we can't skip parsing entirely when positional mode is triggered.

One important issue is whether there's a semicolon in there ("legacy batching") - we don't know that until we parse. We could say that in positional mode we don't care: legacy batching simply isn't supported (use the upcoming batching API instead), and if you do it, PG will error. But that leaves the case of no parameters at all... We do need to parse there since legacy batching is valid.

So we could indeed skip parsing only when there are positional parameters - but we'd still need to do it for no parameters, which seems inconsistent/weird. My logic here was that if we get parsing/rewriting out of the hot path (the prepared case) - which this PR does - then it's better to just always parse for the non-prepared case for consistency etc. Had there been a way to totally get rid of parsing (e.g. the opt-in path), I'd have like that, but given that we keep it in...

Does that make sense?

Explicit preparation for multiplexing would really make my day for instance

I'm assuming you want this in order to "pin" certain SQLs and make sure they stay prepared forever (and not subject to the LRU logic of auto-preparation), right? Otherwise explicit preparation simply isn't very useful given that Dapper/EF don't support it etc. In any case, it's probably possible to make preparation and multiplexing work, by deferring the preparation to execution time.

I think we could really improve named param/legacy batching perf when auto prepare is enabled.

Is that important? My mindset is that starting 6.0, the recommended pattern would be positional parameters with the new batching API, and anything else would be sub-optimal. In other words, I'd rather not spend time on optimizing SqlQueryParser (which is already not bad) when there's a far better API that avoids it entirely.

Breaking change: having an NpgsqlParameter which isn't referenced in the SQL no longer works (silly)

Just for positional or named as well?

Both - though the note is a bit too concise/imprecise :)

In SqlQueryParser, if nothing needed to be rewritten (no parameter placeholders, no semicolon), we just take the user SQL and param list as-is. This is what happens for unprepared positional parameters (no legacy batching supported) - we only parse, no rewriting. This means that if you have unreferenced positional parameters, we just send them as-is, and PG complains because the Parse and Bind messages mismatch.

The same optimization also kicks in if you have a SQL without placeholders, but some named parameters in the list - we just send everything as-is. So in named parameter mode, if there's at least one placeholder (or legacy batching), we will rewrite and everything will be OK (we pick out only the referenced parameters). But if it's a single statement with no placeholders, but parameters are present (named or not), it will fail.

I could add a check to not rewrite only when doing positional, though the whole thing is a bit ridiculous (why are you putting that parameter in there if it isn't referenced...).

Thanks for the thorough reviewing @NinoFloris!

Breaking change: having an NpgsqlParameter which isn't referenced in the SQL no longer works (silly)

Just for positional or named as well?

What I wrote above wasn't accurate - PG actually doesn't have a problem with parameters in Bind which aren't referenced in Parse (the problem was specifically because the parameter in the test had a null value without NpgsqlDbType, so PG couldn't infer the type).

Regardless, I changed things so that SqlQueryParser only avoids rewriting for positional parameters (in the named or zero parameters cases it always rewrites). But once the command is prepared all that goes away anyway.

One important issue is whether there's a semicolon in there ("legacy batching") - we don't know that until we parse. We could say that in positional mode we don't care: legacy batching simply isn't supported (use the upcoming batching API instead), and if you do it, PG will error. But that leaves the case of no parameters at all... We do need to parse there since legacy batching is valid.

So we could indeed skip parsing only when there are positional parameters - but we'd still need to do it for no parameters, which seems inconsistent/weird. My logic here was that if we get parsing/rewriting out of the hot path (the prepared case) - which this PR does - then it's better to just always parse for the non-prepared case for consistency etc. Had there been a way to totally get rid of parsing (e.g. the opt-in path), I'd have like that, but given that we keep it in...

Ok I get where you're coming from, but I still think we should go down the fast path (no parsing) when we notice we're in positional mode. IMO there should be some 'zero overhead' way to execute a query without preparation. Speaking for myself there are queries today that I'd rather not prepare (due to dynamic compilation) but would still like to execute with the lowest latency, and as they're not tiny queries parsing does add measurable overhead, sorry...

Is that important? My mindset is that starting 6.0, the recommended pattern would be positional parameters with the new batching API, and anything else would be sub-optimal. In other words, I'd rather not spend time on optimizing SqlQueryParser (which is already not bad) when there's a far better API that avoids it entirely.

I'd say this is hopelessly optimistic ;) I can see legacy batching being entirely replaced (though dapper will take its time) but named parameters are so useful that I don't see people (or me) dropping them any time soon * for handwritten sql * (orms is another matter). Making people choose between them and perf feels like we're going for a false choice. Once batching is supported it would be entirely reasonable to add a flag (I know) to disable legacy batching entirely. And at that point we can just directly lookup statements in the preparation cache without parsing, for unprepared cases it would still not be free but it could become more straight forward than what's there today.

Thoughts?

[...] I still think we should go down the fast path (no parsing) when we notice we're in positional mode. IMO there should be some 'zero overhead' way to execute a query without preparation.

I'm not totally against doing that (in fact it was written that way at some point). But at the end of the day, there's already so much more overhead with non-prepared execution, that the parsing/rewriting is unlikely to have a noticeable effect... I'd like to see some numbers showing it's worth it first...

I'd say this is hopelessly optimistic ;) I can see legacy batching being entirely replaced (though dapper will take its time) but named parameters are so useful that I don't see people (or me) dropping them any time soon * for handwritten sql * (orms is another matter).

I definitely don't expect everyone to switch away from legacy batching and named parameters - I'm just saying they'll need to do it if they want the absolute best possible perf, that's all. I mean, putting this in proportion - the parsing/rewriting overhead is quite small at the end of the day, and unlikely to matter to all but the most perf-hungry users. And those guys really should just use the optimized API, no?

And at that point we can just directly lookup statements in the preparation cache without parsing, for unprepared cases it would still not be free but it could become more straight forward than what's there today.

For named parameters, it's a bit more complicated than that - even for already-prepared commands. I could sent SELECT @p1, @p2, and execute it once with parameters in the order [@p1, @p2], and another time in the order [@p2, @p1]. Today this is taken care of by always doing the parsing/rewriting (which also orders the parameters correctly); of course we could optimize that further, saving some sort of mapping structure which says "the parameter with name @p2 needs to go in the second position in Bind". But at that point we're introducing a lot of complexity (and potential bugs) for optimizing a slow path, when you have the option of just using positional parameters for ultimate perf...

To summarize, I'm definitely not against optimizing the slower paths - but I'd rather avoid lots of complexity (the driver is already pretty complex). I think that we can also treat some of the above outside of this PR, as later optimizations, no?

so much more overhead with non-prepared execution

From the pg side you mean? Well the thing is, dynamic sql execution puts you between a rock and a hard place, you can (auto) prepare everything and fill your pg memory and npgsql caches with nonsense or you choose to do it unprepared. Isn't it worth it to try and get that perf overhead as low as possible for a general purpose driver?

I definitely don't expect everyone to switch away from legacy batching and named parameters - I'm just saying they'll need to do it if they want the absolute best possible perf, that's all. I mean, putting this in proportion - the parsing/rewriting overhead is quite small at the end of the day, and unlikely to matter to all but the most perf-hungry users. And those guys really should just use the optimized API, no?

Sure I just wanted make sure that was what was behind your initial thoughts on it.

of course we could optimize that further, saving some sort of mapping structure which says "the parameter with name @p2 needs to go in the second position in Bind".

That would have been my initial guess yeah, good to know this isn't how it works today ;)

so much more overhead with non-prepared execution

From the pg side you mean?

Yeah - planning, 2 additional wire protocol messages, all that stuff.

Well the thing is, dynamic sql execution puts you between a rock and a hard place, you can (auto) prepare everything and fill your pg memory and npgsql caches with nonsense or you choose to do it unprepared. Isn't it worth it to try and get that perf as low as possible for a general purpose driver?

Well, if your dynamic SQLs tend to repeat themselves, then auto-preparation should work for that scenario (just like non-dynamic). If you're really producing a lot of one-off SQLs which will never repeat, then it definitely doesn't make sense to prepare. Whether that scenario is worth optimizing depends on how much overhead the SQL parsing/rewriting actually adds for those cases... In other words, you can see the impact of removing this for prepared statements precisely because it's already quite optimized - I'm just not sure you'll see that for unprepared.

Can always benchmark and see though.

@benaadams

What about the Multiple queries test as that runs 20 queries sequentially (also 21% of composite score)

First, thanks for suggesting - that uncovered a regression I accidentally introduced... should be fixed (waiting to confirm on the perf lab soon).

I'm seeing very little effect on multiple queries, and that make sense, since that scenario already benefits from an optimization today: if you re-execute the same NpgsqlCommand instance multiple times and it's prepared (either explicitly or auto), then subsequent executions take a very fast path (no need to parse/rewrite again).

So basically this PR only improves things for the first time you execute on a given NpgsqlCommand instance, and multiple queries is all about reusing the instance many times.

I'll run single query though.

The approach in this PR has the problem of tying together the detection of raw mode (absence of legacy semicolon batching) with the preparation SQL cache. The SQL cache is connection-specific, and in multiplexing there is no connection until we're in the write loop; adding SQL parsing/rewriting there (e.g. for the named parameter case) caused significant perf regressions (25k->18k RPS in multiple query).

#3852 is an alternative approach that does not suffer from this problem, but does parse/rewrite for the 0-param case.