-
Notifications
You must be signed in to change notification settings - Fork 819
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Connection string builder #85
Connection string builder #85
Conversation
NpgsqlConnectionStringBuilder now defaults to protocol version Unknown. Make sure NpgsqlConnectionStringBuilder never writes "<key>=<empty value>" to a connection string. Add a constructor NpgsqlConnection(NpgsqlConnectionStringBuilder) Store password bytes in an encapulation that wipes old password out before storing new one. Remove most of the defaults; initialize the private fields instead. Don't put values into base unless they are different from the value default, and also from that type's default value.
Great!
|
Hi Glen! I think this PR is almost ready to go. Just some comments:
|
On Tue, Nov 12, 2013 at 9:51 AM, Francisco Figueiredo Jr. <
Any value not explicitly represented in the connection string will result I felt that it would be confusing to not represent values (such as Port) You'll notice I removed a bunch of explicit defaults, because if the
GMail seems to have messed up the numbered list in the quoted text... -Glen
|
Got it! Thank you for the explanations, Glen. I was a little worried about the complexity of the SetValue() method. But now I understand your motivations. |
On Wed, Nov 13, 2013 at 8:25 AM, Francisco Figueiredo Jr. <
I was concerned with that, too. Hopefully I offset the complexity with -Glen
|
Great! Thanks for the comments in the code. They will help a lot future changes. Another thing I notice is about the performance NpgsqlConnectionStringBuilder. In a test I did here, the creation of NpgsqlConnectionStringBuilder from a connection string improved 349%! Awesome! On the down side, the performance of SetValue method had a performance drop of 38%. I think the call to TypeDefaultValue (which calls Activator.GetInstance()) method could be the culpirit ? This is the test I used in the SpeedTests class:
And these are the results I got running from master and from your PR:
I'm thinking about the scenario where the user sets a lot of value through accessors or properties. Or maybe I'm over thinking as usual... :) |
Add a class to describe a connection string value, which handles default values and some conversions. All values now get a descriptor with defaults. Remove field initializers, set all defaults using descriptors. Eliminate some redundant keyword table lookups. Simplify and make more efficient SetValue(). Get values via this[] directly from the private fields rather than base class storage.
On Thu, Nov 14, 2013 at 8:48 AM, Francisco Figueiredo Jr. <
Slower!!! We can't have that. I added another test, maybe you can try it out on your system... It tests
So after a bunch more reworking, I was able to get both of your tests to In addition, I discovered a couple bugs that slipped past. Clone() was -Glen
|
Awesome, Glen! You are an optimization master!! |
Connection string builder
Francisco,
I was having trouble with Npgsql's ability to try protocol version 3, and then fall back to version 2, since NpgsqlConnectionStringBuilder defaulted to version 3. I changed ConnectionStringBuilder to work properly here, , as well as adding a constructor to NpgsqlConnection.
So now, Npgsql once again attempts to automatically fall back to version 2 even when using ConnectionStringBuilder to set up the connection. However, it's broken in another way as well, which I have fixed in PR #83.
-Glen