Support for pg_service.conf #4962

manandre · 2023-03-01T22:54:50Z

test/Npgsql.Tests/ConnectionStringBuilderTests.cs

Brar · 2023-03-06T05:35:57Z

src/Npgsql/PostgresEnvironment.cs

+
+    static string? GetDefaultSystemPostgresDir() => ExecuteCommandSync("pg_config", "--sysconfdir");
+
+    static string? ExecuteCommandSync(string command, string arguments)


Since this already contains 'Sync' and uses APIs that can block I guess that you also plan an Async variant (also for GetConfigFromIniFile()) - no?
The internal part could use a boolean async flag like we do it all the time in Npgsql (e. g. see KerberosUsernameProvider for how to do this for Process)

Done.
I have shared the code between both usages.

manandre · 2023-03-08T18:31:28Z

The CI checks seem blocked in an infinite wait, even after a new commit on the PR branch...
Can someone help me to solve this ?

vonzshik · 2023-03-08T18:32:43Z

The CI checks seem blocked in an infinite wait, even after a new commit on the PR branch... Can someone help me to solve this ?

It's probably because of merge conflict.

manandre · 2023-03-08T18:51:29Z

The CI checks seem blocked in an infinite wait, even after a new commit on the PR branch... Can someone help me to solve this ?

It's probably because of merge conflict.

Yes, my local git repo was corrupted, so I was not able to pull from upstream and so see the conflict...
Thanks for your help.

manandre · 2023-03-13T19:00:17Z

@vonzshik @Brar @roji
The PR is now ready for (final) review.
Your comments are welcome.

vonzshik · 2023-03-13T19:01:22Z

@manandre Great, I'll take a look tomorrow.

src/Npgsql/PostgresEnvironment.cs

src/Npgsql/NpgsqlConnectionStringBuilder.cs

test/Npgsql.Tests/ConnectionStringBuilderTests.cs

Brar · 2023-03-14T17:09:58Z

src/Npgsql/KerberosUsernameProvider.cs

-            UseShellExecute = false
-        };
+            return includeRealm ? _principalWithRealm : _principalWithoutRealm;



Maybe leave the part above here as sync function and move the part below to a local async function (as it was before).
That way access to the cached values works without async state machine.

vonzshik

Looks quite good.
@roji whenever you have a time.

src/Npgsql/NpgsqlConnectionStringBuilder.cs

manandre · 2023-08-05T13:48:24Z

@roji Can you please tell me whether something prevents this PR from being merged?

roji · 2023-11-07T20:36:34Z

@NinoFloris to verify that there are no size regressions for NativeAOT.

NinoFloris

Unfortunately we're pulling in FileStream again with this PR.

Could you copy the workaround from #5234 to break the internal dependency on FileStream that the convenience File apis like File.ReadAllLines have? (FileStream itself references System.Uri again and so on. It's quite wasteful)

You know it's gone by looking at the NativeAOT github action summary and opening our Size by Namespace panel. System.IO should be around 30kb.

Alternatively you can use sizoscope https://github.com/MichalStrehovsky/sizoscope and load in the mstat file attached to the action run.

I'd be happy to help if you run into any issues.

manandre · 2023-11-07T23:33:14Z

@NinoFloris I have applied the same workaround, please tell me whether it solves the issue here. I am not sure how to read the sizoscopeX output...

NinoFloris · 2023-11-09T17:38:02Z

@manandre to get the dependency analysis going you should also download the npgsql.scan.dgml.xml. Then in sizoscope -> open you select both the mstat and the xml.

Once you're in double clicking a type should pop up another window allowing you to traverse to roots keeping the type alive.

If you want to know what this PR added compared to baseline you can take an mstat & .xml from an actions run on main, then in sizoscope(X) load it via the tools -> diff option in the menu.

In either case I took a look and it seems we're now always rooting System.Diagnostics.Process, which itself brings in a lot, including filestream it seems. This PR is adding about 400kb to the minimal binary just to read pg_config --sysconfdir, that's too much for too little added value IMO.

Would we be able to get that info some other way, or be able to make reading it via pg_config optional?

manandre · 2023-11-09T21:09:01Z

@NinoFloris Thanks for the tutorial ;)
I have removed the dependency to System.Linq as a desperate attempt to reduce binary size.
For System.Diagnostics.Process I do not understand as it is already used in the KerberosUsernameProvider class, it should not appear as new, should it?

NinoFloris · 2023-11-09T21:24:32Z

We're only conditionally referencing it via this mechanism

npgsql/src/Npgsql/Internal/IntegratedSecurityHandler.cs

Line 28 in d05ef7e

    
           => KerberosUsernameProvider.GetUsername(async, includeRealm, connectionLogger, cancellationToken);

So as long as a project using the NpgsqlSlimDataSourceBuilder doesn't call EnableIntegratedSecurity() we don't reference the code that ends up using Process.

manandre · 2023-11-12T15:06:55Z

Oh! I was not aware that NativeAOT is able to optimize such paths.
Here, the sysconfdir is only required when the Service field is set in the connection string or if the PGSERVICE environment variable is set. Why does NativeAOT is not able to optimize this path?

NinoFloris · 2023-11-15T18:10:08Z

So the idea is that the NativeAOT scanner figures out whether these derived types are actually constructed. As long as there is no reachable path to a concrete constructor call of the derived type it can trim its code and its dependencies as well.

Here, the sysconfdir is only required when the Service field is set in the connection string or if the PGSERVICE environment variable is set. Why does NativeAOT is not able to optimize this path?

These things can only be reasoned about at runtime, at which point the code has to be included, just in case.

NinoFloris · 2023-11-15T18:15:46Z

We're aiming to release Npgsql and EFCore.Pg tomorrow so I'm going to move it out to 9.0.

We'll have a bit more time to think about workarounds. Though I hope we see more size improvements in .NET 9.0, ideally when System.Diagnostics.Process is pulled in as well (at least we'll have time to raise the issue as well).

manandre force-pushed the 4950-pg-service-conf branch from 445bfdf to 21e195f Compare March 5, 2023 20:03

vonzshik reviewed Mar 5, 2023

View reviewed changes

test/Npgsql.Tests/ConnectionStringBuilderTests.cs Show resolved Hide resolved

manandre mentioned this pull request Mar 5, 2023

Support for pg_service.conf #4950

Open

Brar reviewed Mar 6, 2023

View reviewed changes

manandre force-pushed the 4950-pg-service-conf branch from 4a98532 to 5492bb9 Compare March 7, 2023 21:49

manandre force-pushed the 4950-pg-service-conf branch from 61ddce6 to 0607b3e Compare March 8, 2023 18:49

manandre marked this pull request as ready for review March 8, 2023 22:26

manandre requested a review from roji as a code owner March 8, 2023 22:26

manandre requested review from Brar and vonzshik and removed request for Brar, roji and vonzshik March 9, 2023 18:57

vonzshik requested changes Mar 14, 2023

View reviewed changes

Brar reviewed Mar 14, 2023

View reviewed changes

vonzshik approved these changes Mar 22, 2023

View reviewed changes

src/Npgsql/NpgsqlConnectionStringBuilder.cs Outdated Show resolved Hide resolved

manandre force-pushed the 4950-pg-service-conf branch from 6c78361 to 4b6a2a4 Compare August 5, 2023 13:10

manandre force-pushed the 4950-pg-service-conf branch from 4b6a2a4 to adafb88 Compare October 1, 2023 13:32

NinoFloris closed this Nov 7, 2023

NinoFloris reopened this Nov 7, 2023

NinoFloris reviewed Nov 7, 2023

View reviewed changes

Support for pg_service.conf

b65e6e1

manandre added 2 commits November 7, 2023 23:57

Remove dependency on FileStream for NativeAOT

17ee0cc

Simplify collection initialization

2c3f2fe

manandre force-pushed the 4950-pg-service-conf branch from adafb88 to 2c3f2fe Compare November 7, 2023 23:05

Remove dependency to System.Linq

71aedbb


		static string? GetDefaultSystemPostgresDir() => ExecuteCommandSync("pg_config", "--sysconfdir");

		static string? ExecuteCommandSync(string command, string arguments)

Support for pg_service.conf #4962

Are you sure you want to change the base?

Support for pg_service.conf #4962

Uh oh!

Conversation

manandre commented Mar 1, 2023

Uh oh!

Uh oh!

Brar Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

manandre Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

manandre commented Mar 8, 2023

Uh oh!

vonzshik commented Mar 8, 2023

Uh oh!

manandre commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manandre commented Mar 13, 2023

Uh oh!

vonzshik commented Mar 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Brar Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

manandre Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

vonzshik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

manandre commented Aug 5, 2023

Uh oh!

roji commented Nov 7, 2023

Uh oh!

NinoFloris left a comment

Choose a reason for hiding this comment

Uh oh!

manandre commented Nov 7, 2023

Uh oh!

NinoFloris commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manandre commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NinoFloris commented Nov 9, 2023

Uh oh!

manandre commented Nov 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NinoFloris commented Nov 15, 2023

Uh oh!

NinoFloris commented Nov 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

manandre commented Mar 8, 2023 •

edited

Loading

NinoFloris commented Nov 9, 2023 •

edited

Loading

manandre commented Nov 9, 2023 •

edited

Loading

manandre commented Nov 12, 2023 •

edited

Loading