Skip to content
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

Update Azure Postgres code #2778

Merged
merged 5 commits into from
Mar 10, 2025
Merged

Update Azure Postgres code #2778

merged 5 commits into from
Mar 10, 2025

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Mar 7, 2025

The EntityFramwork code to talk to Azure Postgres is incorrect. It needs to be different depending on the version of EF being used.

Fix #2769

The EntityFramwork code to talk to Azure Postgres is incorrect. It needs to be different depending on the version of EF being used.

Fix #2769
@eerhardt eerhardt requested review from davidfowl and roji March 7, 2025 19:15
@eerhardt eerhardt requested a review from IEvangelist as a code owner March 7, 2025 19:15
@dotnetrepoman dotnetrepoman bot added the database Content related to database. label Mar 7, 2025
eerhardt added 2 commits March 7, 2025 15:23

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
eerhardt and others added 2 commits March 10, 2025 10:00

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: David Pine <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@IEvangelist IEvangelist merged commit e49f63f into main Mar 10, 2025
7 checks passed
@IEvangelist IEvangelist deleted the FixAzurePostgresEF branch March 10, 2025 15:33
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks.

Just to make sure - the switch from UsePeriodicPasswordProvider to UsePasswordProvider is in principle good, but can you confirm whether credentials.GetTokenAsync() internally caches the token? If not, that's a new token for each physical connection, which might be a bit much. The lambdas passed to UsePasswordProvider could implement caching to mitigate that if we think that's important.

@eerhardt
Copy link
Member Author

but can you confirm whether credentials.GetTokenAsync() internally caches the token?

I don't believe it caches itself. At least I haven't seen any code that does this.

The lambdas passed to UsePasswordProvider could implement caching to mitigate that if we think that's important.

This would be too much code / implementation for sample/doc code. Instead it would be a great thing for a specific package to deal with Entra ID + Npgsql.

@roji
Copy link
Member

roji commented Mar 10, 2025

This would be too much code / implementation for sample/doc code. Instead it would be a great thing for a npgsql/npgsql#5992.

I guess I agree... It's unfortunate for our basic sample to show something inefficient, but hopefully we'll have a package soon. FWIW the performance of opening a new physical connection (as opposed to getting a pooled one) isn't that critical.

@davidfowl
Copy link
Member

@roji are you working on a package that can switch between normal Postgres and azure? That’s what we need to do to take away all of the pain here and what aspire intends to do if there’s no other alternative

@roji
Copy link
Member

roji commented Mar 11, 2025

@davidfowl I agree - this is something that I raised internally with the Azure folk themselves; I believe there should be an Azure.Npgsql package, and that they should ideally own it. I'll forward you the email thread in case you're interested etc.

@roji
Copy link
Member

roji commented Mar 11, 2025

(oh I see you already got looped in)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Content related to database.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Postgres integration using official docs doesn't work
4 participants