Skip to content

Conversation

@roji
Copy link
Member

@roji roji commented May 31, 2022

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).

@roji roji changed the title WIP on NpgsqlDataSource Basic implementation of NpgsqlDataSource Jun 1, 2022
@roji roji force-pushed the DbDataSource branch 3 times, most recently from 6875019 to f039659 Compare June 6, 2022 17:16
@roji roji marked this pull request as ready for review June 6, 2022 17:24
@roji roji requested a review from vonzshik as a code owner June 6, 2022 17:24
@roji
Copy link
Member Author

roji commented Jun 6, 2022

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.

Copy link
Contributor

@vonzshik vonzshik left a 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.

@roji
Copy link
Member Author

roji commented Jun 19, 2022

Thanks for the review @vonzshik!

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 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>
@roji roji enabled auto-merge (squash) June 19, 2022 20:38
@roji roji merged commit ff6d793 into npgsql:main Jun 19, 2022
@roji roji deleted the DbDataSource branch June 19, 2022 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants