> On 22 Jul 2019, at 10:46, Peter Eisentraut <peter.eisentr...@2ndquadrant.com> > wrote: > > On 2019-04-04 15:40, Daniel Gustafsson wrote: >> On Wednesday, March 27, 2019 1:43 PM, Christoph Berg <m...@debian.org> wrote: >> >>> Re: Daniel Gustafsson 2019-03-26 >>> pC-NMmh4vQLQP76YTwY4AuoD4OdNw9egikekyQpXFpgqmTlGjIZXOTd2W5RDZPpRski5N3ADRrLYgLk6QUuvmuT5fWC9acPAYyDU1AVxJcU=@yesql.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
0003-Default-new-bindir-to-exec_path-v3.patch
Description: Binary data
0002-Check-all-used-executables-v3.patch
Description: Binary data
0001-Only-allow-upgrades-by-the-same-exact-version-new-v3.patch
Description: Binary data
