theseus-rs / postgresql-embedded

Embed PostgreSQL database
Apache License 2.0
34 stars 5 forks source link

work around hang starting database on windows #67

Closed Dridus closed 1 month ago

Dridus commented 1 month ago

on windows, it appears that for whatever reason pg_ctl start never closes the stdout/stderr pipes, resulting in a timeout if that's configured or hang if not. I'm not sure the precise reason for this, I'm guessing because pg_ctl forks the server and accidentally hands it stdout/stderr or something along those lines.

I was tipped off to this behavior by pg-embed: https://github.com/faokunega/pg-embed/blob/master/src/command_executor.rs#L180-L192

pg-embed just doesn't read the output of subprocesses it executes, but this library appears to (e.g. for database_exists) and so I hacked around it by abandoning the pipe reads 50ms after the subprocess produces an exit status.

this is clearly a horrible hack, and as written it also adds 50ms to every process invocation on windows, not just pg_ctl start. I'm submitting it in the hopes that it's better than broken or at least as a very detailed issue submission.

thanks for the library! sorry for the gross hack

brianheineman commented 1 month ago

Hello @Dridus, thank you for the PR! I noticed that there is also a suggested style change in this PR to remove usage of the custom Result. Would you mind removing that change from this PR as I believe that is a separate issue from the Windows bug. I'd be happy to discuss a change in the interfaces to remove the custom Result in a separate issue/PR for all the API's if you would like; I'd be interested to hear the pro's/con's of the change from your perspective.

Dridus commented 1 month ago

so (un)revised.

my rationale for it was because I was referring to the std::result:: version several times (more in some WIP versions) and it got quite noisy, and I prefer to not pun prelude imported things to minimize confusion. I probably won't put in an issue or PR for that at the moment, but I'll keep it in mind for future if I'm making future suggestions!

Dridus commented 1 month ago

also thanks for the quick PR responses!

brianheineman commented 1 month ago

This change was released in version 0.9.3; thank you for your contribution!