On 2019-07-25 16:33, Daniel Gustafsson wrote: > Fair enough, those are both excellent points. I’ve shuffled the code around > to > move back the check for exec_path to setup (albeit earlier than before due to > where we perform directory checking). This does mean that the directory > checking in the options parsing must learn to cope with missing directories, > which is a bit unfortunate since it’s already doing a few too many things > IMHO. > To ensure dogfooding, I also removed the use of -B in ‘make check’ for > pg_upgrade, which should bump the coverage. > > Also spotted a typo in a pg_upgrade file header in a file touched by this, so > included it in this thread too as a 0004.
I have committed 0002, 0003, and 0004. The implementation in 0001 (Only allow upgrades by the same exact version new bindir) has a problem. It compares (new_cluster.bin_version != PG_VERSION_NUM), but new_cluster.bin_version is actually just the version of pg_ctl, so this is just comparing the version of pg_upgrade with the version of pg_ctl, which is not wrong, but doesn't really achieve the full goal of having all binaries match. I think a better structure would be to add a version check for each validate_exec() so that each program is checked against pg_upgrade. This should mirror what find_other_exec() in src/common/exec.c does. In a better world we would use find_other_exec() directly, but then we can't support -B. Maybe expand find_other_exec() to support this, or make a private copy for pg_upgrade to support this. (Also, we have two copies of validate_exec() around. Maybe this could all be unified.) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
