Re: pg_upgrade version checking questions

> On 22 Jul 2019, at 10:46, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>
> On 2019-04-04 15:40, Daniel Gustafsson wrote:
>> On Wednesday, March 27, 2019 1:43 PM, Christoph Berg <myon(at)debian(dot)org> wrote:
>>
>>> Re: Daniel Gustafsson 2019-03-26 pC-NMmh4vQLQP76YTwY4AuoD4OdNw9egikekyQpXFpgqmTlGjIZXOTd2W5RDZPpRski5N3ADRrLYgLk6QUuvmuT5fWC9acPAYyDU1AVxJcU=(at)yesql(dot)se
>>>
>>>> 0003 - Make -B default to CWD and remove the exec_path check
>>>> Defaulting to CWD for the new bindir has the side effect that the default
>>>> sockdir is in the bin/ directory which may be less optimal.
>>>
>>> Hmm, I would have thought that the default for the new bindir is the
>>> directory where pg_upgrade is located, not the CWD, which is likely to
>>> be ~postgres or the like?
>>
>> Yes, thinking on it that's obviously better. The attached v2 repurposes the
>> find_my_exec() check to make the current directory of pg_upgrade the default
>> for new_cluster.bindir (the other two patches are left as they were).

Thanks for reviewing!

> 0001-Only-allow-upgrades-by-the-same-exact-version-new-v2.patch
>
> I don't understand what this does. Please explain.

This patch makes the version check stricter to ensure that pg_upgrade and the
new cluster is of the same major and minor version. The code grabs the full
version from the various formats we have (x.y.z, x.z, xdevel) where we used to
skip the minor rev. This is done to address one of Toms original complaints in
this thread.

> 0002-Check-all-used-executables-v2.patch
>
> I think we'd also need a check for pg_controldata.

Fixed. I also rearranged the new cluster checks to be in alphabetical order
since the list makes more sense then (starting with initdb etc).

> Perhaps this comment could be improved:
>
> /* these are only needed in the new cluster */
>
> to
>
> /* these are only needed for the target version */
>
> (pg_dump runs on the old cluster but has to be of the new version.)

I like this suggestion, fixed with a little bit of wordsmithing.

> 0003-Default-new-bindir-to-exec_path-v2.patch
>
> I don't like how the find_my_exec() code has been moved around. That
> makes the modularity of the code worse. Let's keep it where it was and
> then structure it like this:
>
> if -B was given:
> new_cluster.bindir = what was given for -B
> else:
> # existing block

The reason for moving is that we print default values in usage(), and that
requires the value to be computed before calling usage(). We already do this
for resolving environment values in parseCommandLine(). If we do it in setup,
then we’d have to split out resolving the new_cluster.bindir into it’s own
function exposed to option.c, or do you have any other suggestions there?

I’ve attached all three patches as v3 to be compatible with the CFBot, only
0002 changed so far.

cheers ./daniel