-
Notifications
You must be signed in to change notification settings - Fork 877
Basic implementation of NpgsqlDataSource #4492
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
Conversation
6875019 to
f039659
Compare
|
OK, I think this is more or less ready for review. Note that we don't yet use the recently-merged DbDataSource from runtime. Instead of have a compatibility-shim DbDataSource for older TFMs (as we usually do), and use that for all TFMs for now. Once we take a dependency on a .NET 7.0 preview which contains DbDataSource, we'll use that one. |
vonzshik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all, it looks OK. The thing I'm the most unsure of is that NpgsqlDataSource is a subclass of DbDataSource: there is already quite a bit of code in our pooling implementation, and adding even more things there (as creating connections, commands and batches) seems a bit too much for me. I'm going to think a bit more about this for now, to not block you further.
|
Thanks for the review @vonzshik!
I understand what you're saying. First, there's going to have to be an NpgsqlDataSource which is a subclass of DbDataSource: that's where users would find the Npgsql-specific APIs for configuring the data source (logging, auth...). Now, that doesn't mean that this hierarchy must be the same as the internal one we have for pooling/multihost/multiplexing... In fact, when I started I thought that NpgsqlDataSource would have a a reference to one of the "connector source" types (as they're currently named), rather than have the same hierarchy. But it seemed needlessly complicated and just another layer of indirection, so I ended up merging them. It's also important to note that NpgsqlDataSource itself (the abstract base class) is pretty thin, and all the heavy pooling stuff is in the subclasses. So it feels to me that code is still well-factored in the hierarchy. |
Co-authored-by: Nikita Kazmin <vonzshik@gmail.com>
This PR does more or less the minimum for introducing NpgsqlDataSource. It doesn't implement the nice things which NpgsqlDataSource would allow us to do, e.g. logging/type mapping (but it does remove the tricky lock-free logic in PoolManager).