Updates to aspnetcore PG database access #8005
Conversation
roji
force-pushed
the
NpgsqlStuff
branch
3 times, most recently
from
2f5b633
to
448297e
Compare
* Use batching with Sync error barriers in Updates * Use NpgsqlDataSource * Use typed NpgsqlParameter<T> everywhere * Use positional parameter placeholders everywhere ($1, $2)
|
@DamianEdwards pushed your changes from aspnet/Benchmarks#1801 into this, can you please take a look? Will leave this open for some last perf experiments I want to do in the coming days. |
|
@roji looks good! |
These aren't ready to be integrated here. Let's merge this PR, I'll continue experimenting and possibly submit a separate one later. |
|
Note: added another commit here to disable SQL parsing/rewriting (see aspnet/Benchmarks#1590 (comment)). This should provide a small speed bump to Fortunes specifically. |
| #if NPGSQL | ||
| // This disables SQL parsing/rewriting, which requires using positional parameters and NpgsqlBatch everywhere. | ||
| // This helps commands where there are no parameters (Fortunes); when there are parameters, their ParameterName | ||
| // being null already triggers positional parameters and disables parsing) | ||
| // Note that Dapper and EF aren't yet compatible with this mode. | ||
| AppContext.SetSwitch("Npgsql.EnableSqlRewriting", false); | ||
| #endif |
There was a problem hiding this comment.
Is this something we have enabled in our copy of the benchmark, or is it not required there for other reasons?
There was a problem hiding this comment.
We can't do it in our copy since we have Dapper and EF, which this is incompatible with... This one is tricky, since it's an AppContext switched cached at startup, but we don't know whether the application will be used for raw, dapper or EF.
This could be an #if (we already have #if DATABASE, #if NPGSQL vs. MYSQLCONNECTOR)...
There was a problem hiding this comment.
I see. Ideally this would be a property that be set directly on the command. What kind of gains are we talking about by setting it here?
There was a problem hiding this comment.
See aspnet/Benchmarks#1590 (comment).
I did a measurement a long time ago and it wasn't very significant, but we're at the point where every little bit helps (I can measure again at some point).
There was a problem hiding this comment.
Using AppContext switches makes me slightly uneasy as they're usually reserved in .NET for restoring unsupported behaviors, rather than opting-in to legitimate optimizations. I think this is one of those things that's very much in the contentious or at least debatable zone WRT to keeping in the spirit of the benchmark.
There was a problem hiding this comment.
We discussed this offline and I'm OK with this going in now.
|
@nbrady-techempower ready to merge, thanks |
* Some code cleanup * Updates to aspnetcore PG database access * Use batching with Sync error barriers in Updates * Use NpgsqlDataSource * Use typed NpgsqlParameter<T> everywhere * Use positional parameter placeholders everywhere ($1, $2) * Stop UTF8 decoding/reencoding in Fortunes platform * Update Fortunes to use Razor templating * Turn of SQL parsing/rewriting for Fortunes
No description provided.