Fix command text with batching for OpenTelemetry #5706
Conversation
9ba58d8 to
1a94abd
Compare
3a419ed to
66d0595
Compare
| @@ -57,7 +57,7 @@ async Task Authenticate(string username, NpgsqlTimeout timeout, bool async, Canc | |||
| async Task AuthenticateCleartext(string username, bool async, CancellationToken cancellationToken = default) | |||
| { | |||
| var passwd = await GetPassword(username, async, cancellationToken).ConfigureAwait(false); | |||
| if (passwd == null) | |||
| if (string.IsNullOrEmpty(passwd)) | |||
There was a problem hiding this comment.
It seems like one of the tests somehow changes password from null to an empty string. No idea which one and why only now...
| Debug.Assert(IsWrappedByBatch); | ||
| if (InternalBatchCommands.Count == 1) | ||
| return InternalBatchCommands[0].CommandText; | ||
| // TODO: Potentially cache on connector/command? |
There was a problem hiding this comment.
Yeah, we also allocate a StringBuilder in SqlQueryParser, we could have a common one on the connector indeed...
| if (InternalBatchCommands.Count == 1) | ||
| return InternalBatchCommands[0].CommandText; | ||
| // TODO: Potentially cache on connector/command? | ||
| var sb = new StringBuilder(); |
There was a problem hiding this comment.
Thanks for fixing this!
In the related issue you write "the worst thing there is an additional string allocation". Since StringBuilder starts with a capacity of 16 characters and then doubles when that is exceeded, this can result in a couple of string allocations. Would it be a good idea to calculate the required space up front and set the capacity in the constructor?
There was a problem hiding this comment.
Well, it's definitely not a bad thing to precalculate capacity before allocating StringBuilder, though iterating over each batch command (in addition to the one we already have) does make things a bit slower (whether it's still going to be faster compared to just leaving StringBuilder as is depends on a lot of things and will require a proper benchmark).
I think, probably the best thing here is to just make a predefined capacity (1024?). But since we're also going to backport this to 8.0 (and probably 7.0, didn't really look that closely), it's better to leave it as simple as possible and instead make a separate pr to improve things for 9.0.
There was a problem hiding this comment.
I'm also assuming that once we cache the StringBuilder on the connector, capacity issues no longer matter as the builder will grow once and then just reuse the internal memory etc.
Fixes #5660