use pg-ctl stop to cleanup before killing the process#16
use pg-ctl stop to cleanup before killing the process#16DShau22 wants to merge 1 commit intougtar:masterfrom
Conversation
e3c64bd to
f055454
Compare
|
Just squashed and force pushed! |
| stdout=subprocess.DEVNULL, | ||
| ) | ||
| elif self.pg_process: | ||
| pg_stop_cmd = ['pg_ctl', 'stop', '-D', self.pg_data_dir, '-m' 'fast'] |
There was a problem hiding this comment.
We need to be able to override pg_ctl in the same way that we do postgres or initdb, in the case the user is using an alternative postgres install that is not in the PATH. This needs a default, and an argument (plus said default) in the constructor. See for example DEFAULT_POSTGRES or DEFAULT_INITDB
| ) | ||
| elif self.pg_process: | ||
| pg_stop_cmd = ['pg_ctl', 'stop', '-D', self.pg_data_dir, '-m' 'fast'] | ||
| self.run_cmd(pg_stop_cmd, level=2, bg=True).wait() |
There was a problem hiding this comment.
I think since run_cmd with level 2 can potentially produce stdout and stderr, instead of .wait() we should call .communicate(). Also, since this command can be caught by the pg process or hang, we should have a timeout at which point we give up and call kill() anyway. Maybe 10 seconds?
The python docs have an example of how to do this properly, catching the TimeoutExpired exception https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate
|
thanks @DShau22, sorry I took so long to get to this, I've been swamped at dayjob |
Proposed fix for this issue
Ran
make testto confirm that all existing tests still pass, andmake checkfor formattingDocs on pg-ctl for reference