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