Re: pg_upgrade version checking questions
> On 3 Mar 2021, at 09:57, Peter Eisentraut > wrote: > I committed this. I added a pg_strip_crlf() so that there are no newlines in > the error message. Right, that's much better, thanks! -- Daniel Gustafsson https://vmware.com/
Re: pg_upgrade version checking questions
On 02.03.21 22:51, Daniel Gustafsson wrote: The commit message says something about "to ensure the health of the target cluster", which doesn't make sense to me. Maybe find a better wording. Reworded in the attached updated version. The name find_exec() seems not very accurate. It doesn't find anything. Maybe "check"? I'm not wild about check_exec(), but every other name I could think of was drastically worse so I went with check_exec. I'm not sure why the new find_exec() adds EXE. AFAIK, this is only required for stat(), and validate_exec() already does it. Good point, fixed. I committed this. I added a pg_strip_crlf() so that there are no newlines in the error message. I also slightly reworded the error message to make the found and expected value distinguishable.
Re: pg_upgrade version checking questions
> On 2 Mar 2021, at 14:20, Peter Eisentraut > wrote: > > On 23.02.21 17:14, Daniel Gustafsson wrote: >> This exports validate_exec to reduce duplication, and implements a custom >> find_other_exec-like function in pg_upgrade to check each binary for the >> version number. Keeping a local copy of validate_exec is easy to do if it's >> deemed not worth it to export it. > > This looks mostly okay to me. Thanks for reviewing! > The commit message says something about "to ensure the health of the target > cluster", which doesn't make sense to me. Maybe find a better wording. Reworded in the attached updated version. > The name find_exec() seems not very accurate. It doesn't find anything. > Maybe "check"? I'm not wild about check_exec(), but every other name I could think of was drastically worse so I went with check_exec. > I'm not sure why the new find_exec() adds EXE. AFAIK, this is only required > for stat(), and validate_exec() already does it. Good point, fixed. -- Daniel Gustafsson https://vmware.com/ v6-0001-Check-version-of-target-cluster-binaries.patch Description: Binary data
Re: pg_upgrade version checking questions
On 23.02.21 17:14, Daniel Gustafsson wrote: This exports validate_exec to reduce duplication, and implements a custom find_other_exec-like function in pg_upgrade to check each binary for the version number. Keeping a local copy of validate_exec is easy to do if it's deemed not worth it to export it. This looks mostly okay to me. The commit message says something about "to ensure the health of the target cluster", which doesn't make sense to me. Maybe find a better wording. The name find_exec() seems not very accurate. It doesn't find anything. Maybe "check"? I'm not sure why the new find_exec() adds EXE. AFAIK, this is only required for stat(), and validate_exec() already does it.
Re: pg_upgrade version checking questions
> On 27 Jul 2019, at 08:42, Peter Eisentraut > wrote: > > 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.) Turns out I overshot my original estimate of a new 0001 by a hair (by ~530 days or so) but attached is an updated version. This exports validate_exec to reduce duplication, and implements a custom find_other_exec-like function in pg_upgrade to check each binary for the version number. Keeping a local copy of validate_exec is easy to do if it's deemed not worth it to export it. -- Daniel Gustafsson https://vmware.com/ v5-0001-Check-version-of-target-cluster-binaries.patch Description: Binary data
Re: pg_upgrade version checking questions
> On 2 Sep 2019, at 19:59, Alvaro Herrera wrote: > > On 2019-Jul-30, Daniel Gustafsson wrote: > >> I’ll take a stab at tidying all of this up to require less duplication, we’ll >> see where that ends up. > > Hello Daniel, are you submitting a new version soon? I am working on an updated version which unfortunately got a bit delayed, but will be submitted shortly (targeting this week). cheers ./daniel
Re: pg_upgrade version checking questions
On 2019-Jul-30, Daniel Gustafsson wrote: > I’ll take a stab at tidying all of this up to require less duplication, we’ll > see where that ends up. Hello Daniel, are you submitting a new version soon? Thanks, -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pg_upgrade version checking questions
On Wed, Jul 31, 2019 at 3:13 AM Daniel Gustafsson wrote: > > On 27 Jul 2019, at 08:42, Peter Eisentraut > > wrote: > > I have committed 0002, 0003, and 0004. > > Thanks! > > > 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. > > Right, it seemed the cleanest option at the time more or less based on the > issues outlined below. > > > 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.) > > I’ll take a stab at tidying all of this up to require less duplication, we’ll > see where that ends up. Hi Daniel, I've moved this to the next CF, because it sounds like you're working on a new version of 0001. If I misunderstood and you're happy with just 0002-0004 being committed for now, please feel free to mark the September entry 'Committed'. -- Thomas Munro https://enterprisedb.com
Re: pg_upgrade version checking questions
> On 27 Jul 2019, at 08:42, Peter Eisentraut > wrote: > I have committed 0002, 0003, and 0004. Thanks! > 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. Right, it seemed the cleanest option at the time more or less based on the issues outlined below. > 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.) I’ll take a stab at tidying all of this up to require less duplication, we’ll see where that ends up. cheers ./daniel
Re: pg_upgrade version checking questions
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
Re: pg_upgrade version checking questions
> On 24 Jul 2019, at 22:32, Peter Eisentraut > wrote: > > On 2019-07-23 17:30, Daniel Gustafsson wrote: >> 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 think doing nontrivial work in order to print default values in > usage() is bad practice, because in unfortunate cases it would even > prevent you from calling --help. Also, in this case, it would probably > very often exceed the typical line length of --help output and create > some general ugliness. Writing something like "(default: same as this > pg_upgrade)" would probably achieve just about the same. 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. Thanks again for reviewing, much appreciated! cheers ./daniel 0004-Fix-typo-in-pg_upgrade-file-header-v4.patch Description: Binary data 0003-Default-new-bindir-to-exec_path-v4.patch Description: Binary data 0002-Check-all-used-executables-v4.patch Description: Binary data 0001-Only-allow-upgrades-by-the-same-exact-version-new-v4.patch Description: Binary data
Re: pg_upgrade version checking questions
On 2019-07-23 17:30, Daniel Gustafsson wrote: > 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 think doing nontrivial work in order to print default values in usage() is bad practice, because in unfortunate cases it would even prevent you from calling --help. Also, in this case, it would probably very often exceed the typical line length of --help output and create some general ugliness. Writing something like "(default: same as this pg_upgrade)" would probably achieve just about the same. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pg_upgrade version checking questions
> On 22 Jul 2019, at 10:46, Peter Eisentraut > wrote: > > On 2019-04-04 15:40, Daniel Gustafsson wrote: >> On Wednesday, March 27, 2019 1:43 PM, Christoph Berg 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
Re: pg_upgrade version checking questions
On 2019-04-04 15:40, Daniel Gustafsson wrote: > On Wednesday, March 27, 2019 1:43 PM, Christoph Berg 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). 0001-Only-allow-upgrades-by-the-same-exact-version-new-v2.patch I don't understand what this does. Please explain. 0002-Check-all-used-executables-v2.patch I think we'd also need a check for pg_controldata. 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.) 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 -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pg_upgrade version checking questions
On Wednesday, March 27, 2019 1:43 PM, Christoph Berg 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). cheers ./daniel 0001-Only-allow-upgrades-by-the-same-exact-version-new-v2.patch Description: Binary data 0002-Check-all-used-executables-v2.patch Description: Binary data 0003-Default-new-bindir-to-exec_path-v2.patch Description: Binary data
Re: pg_upgrade version checking questions
Re: Daniel Gustafsson 2019-03-26 > 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? On Debian, the incantation is /usr/lib/postgresql/12/bin/pg_upgrade \ -b /usr/lib/postgresql/11/bin \ -B /usr/lib/postgresql/12/bin<-- should be redundant Christoph
Re: pg_upgrade version checking questions
On Tuesday, March 19, 2019 12:35 AM, Tom Lane wrote: > I noticed a few things that seem a bit fishy about pg_upgrade. Attached are three patches which takes a stab at the issues raised here (and the discussion in this thread): 0001 - Enforces the version check to the full version including the minor 0002 - Tests for all the binaries that pg_upgrade executes 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. cheers ./daniel 0002-Check-all-used-executables.patch Description: Binary data 0003-Default-new-bindir-to-CWD.patch Description: Binary data 0001-Only-allow-upgrades-by-the-same-exact-version-new-bi.patch Description: Binary data
Re: pg_upgrade version checking questions
Re: Peter Eisentraut 2019-03-22 <57769959-8960-a9ca-fc9c-4dbb12629...@2ndquadrant.com> > I'm still in favor of defaulting --new-bindir appropriately. It seems > silly not to. We know where the directory is, we don't have to ask anyone. Fwiw I've been wondering why I have to pass that option every time I've been using pg_upgrade. +1 on making it optional/redundant. Christoph
Re: pg_upgrade version checking questions
On 2019-03-19 16:51, Tom Lane wrote: > I'm not really getting your point here. Packagers ordinarily tie > those versions together anyway, I'd expect --- there's no upside > to not doing so, and plenty of risk if one doesn't, because of > exactly the sort of coordinated-changes hazard I'm talking about here. The RPM packages do that, but the Debian packages do not. >>> 3. Actually, I'm kind of wondering why pg_upgrade has a --new-bindir >>> option at all, rather than just insisting on finding the new-version >>> executables in the same directory it is in. This seems like, at best, >>> a hangover from before it got into core. Even if you don't want to >>> remove the option, we could surely provide a useful default setting >>> based on find_my_exec. > >> Previously discussed here: >> https://www.postgresql.org/message-id/flat/1304710184.28821.9.camel%40vanquo.pezone.net >> (Summary: right) > > Mmm. The point that a default is of no particular use to scripts is > still valid. Shall we then remove the useless call to find_my_exec? I'm still in favor of defaulting --new-bindir appropriately. It seems silly not to. We know where the directory is, we don't have to ask anyone. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pg_upgrade version checking questions
Peter Eisentraut writes: > On 2019-03-19 00:35, Tom Lane wrote: >> 2. check_cluster_versions() insists that the target version be the >> same major version as pg_upgrade itself, but is that really good enough? >> As things stand, it looks like pg_upgrade 11.3 would happily use pg_dump >> 11.1, or vice versa. > I'd hesitate to tie this down too much. It's possible that either the > client or the server package cannot currently be upgraded because of > some other dependencies. In fact, a careful packager might as a result > of a change like this tie the client and server packages together with > an exact version match. This has the potential to make the global > dependency hell worse. I'm not really getting your point here. Packagers ordinarily tie those versions together anyway, I'd expect --- there's no upside to not doing so, and plenty of risk if one doesn't, because of exactly the sort of coordinated-changes hazard I'm talking about here. >> 3. Actually, I'm kind of wondering why pg_upgrade has a --new-bindir >> option at all, rather than just insisting on finding the new-version >> executables in the same directory it is in. This seems like, at best, >> a hangover from before it got into core. Even if you don't want to >> remove the option, we could surely provide a useful default setting >> based on find_my_exec. > Previously discussed here: > https://www.postgresql.org/message-id/flat/1304710184.28821.9.camel%40vanquo.pezone.net > (Summary: right) Mmm. The point that a default is of no particular use to scripts is still valid. Shall we then remove the useless call to find_my_exec? regards, tom lane
Re: pg_upgrade version checking questions
On 2019-03-19 00:35, Tom Lane wrote: > 2. check_cluster_versions() insists that the target version be the > same major version as pg_upgrade itself, but is that really good enough? > As things stand, it looks like pg_upgrade 11.3 would happily use pg_dump > 11.1, or vice versa. I'd hesitate to tie this down too much. It's possible that either the client or the server package cannot currently be upgraded because of some other dependencies. In fact, a careful packager might as a result of a change like this tie the client and server packages together with an exact version match. This has the potential to make the global dependency hell worse. > 3. Actually, I'm kind of wondering why pg_upgrade has a --new-bindir > option at all, rather than just insisting on finding the new-version > executables in the same directory it is in. This seems like, at best, > a hangover from before it got into core. Even if you don't want to > remove the option, we could surely provide a useful default setting > based on find_my_exec. Previously discussed here: https://www.postgresql.org/message-id/flat/1304710184.28821.9.camel%40vanquo.pezone.net (Summary: right) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pg_upgrade version checking questions
On Tuesday, March 19, 2019 7:55 AM, Bruce Momjian wrote: > On Tue, Mar 19, 2019 at 02:43:49AM -0400, Bruce Momjian wrote: > > > > 3. Actually, I'm kind of wondering why pg_upgrade has a --new-bindir > > > option at all, rather than just insisting on finding the new-version > > > executables in the same directory it is in. This seems like, at best, > > > a hangover from before it got into core. Even if you don't want to > > > remove the option, we could surely provide a useful default setting > > > based on find_my_exec. (I'm amused to notice that pg_upgrade > > > currently takes the trouble to find out its own path, and then does > > > precisely nothing with the information.) > > > > > > > Good point. You are right that when it was outside of the source tree, > > and even in /contrib, that would not have worked easily. Makes sense to > > at least default to the same directory as pg_upgrade. > > I guess an open question is whether we should remove the --new-bindir > option completely. If the default is made to find the new-version binaries in the same directory, keeping --new-bindir could still be useful for easier testing of pg_upgrade. cheers ./daniel
Re: pg_upgrade version checking questions
On Tue, Mar 19, 2019 at 02:43:49AM -0400, Bruce Momjian wrote: > > 3. Actually, I'm kind of wondering why pg_upgrade has a --new-bindir > > option at all, rather than just insisting on finding the new-version > > executables in the same directory it is in. This seems like, at best, > > a hangover from before it got into core. Even if you don't want to > > remove the option, we could surely provide a useful default setting > > based on find_my_exec. (I'm amused to notice that pg_upgrade > > currently takes the trouble to find out its own path, and then does > > precisely nothing with the information.) > > Good point. You are right that when it was outside of the source tree, > and even in /contrib, that would not have worked easily. Makes sense to > at least default to the same directory as pg_upgrade. I guess an open question is whether we should remove the --new-bindir option completely. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: pg_upgrade version checking questions
On Mon, Mar 18, 2019 at 07:35:17PM -0400, Tom Lane wrote: > While poking around trying to find an explanation for the pg_upgrade > failure described here: > https://www.postgresql.org/message-id/flat/CACmJi2JUhGo2ZxqDkh-EPHNjEN1ZA1S64uHLJFWHBhUuV4492w%40mail.gmail.com > I noticed a few things that seem a bit fishy about pg_upgrade. > I can't (yet) connect any of these to Tomasz' problem, but: > > 1. check_bin_dir() does validate_exec() for pg_dumpall and pg_dump, > but not for pg_restore, though pg_upgrade surely calls that too. > For that matter, it's not validating initdb and vacuumdb, though > it's grown dependencies on those as well. Seems like there's little > point in checking these if we're not going to check all of them. Yes, adding those checks would be nice. I guess I never suspected there would be mixed-version binaries in that directory. > 2. check_cluster_versions() insists that the target version be the > same major version as pg_upgrade itself, but is that really good enough? > As things stand, it looks like pg_upgrade 11.3 would happily use pg_dump > 11.1, or vice versa. With this rule, we cannot safely make any fixes > in minor releases that rely on synchronized changes in the behavior of > pg_upgrade and pg_dump/pg_dumpall/pg_restore. I've not gone looking > to see if we've already made such changes in the past, but even if we > never have, that's a rather tight-looking straitjacket. I think we > should insist that the new_cluster.bin_version be an exact match > to pg_upgrade's own PG_VERSION_NUM. Again, I never considered minor-version changes, so yeah, forcing minor version matching makes sense. > 3. Actually, I'm kind of wondering why pg_upgrade has a --new-bindir > option at all, rather than just insisting on finding the new-version > executables in the same directory it is in. This seems like, at best, > a hangover from before it got into core. Even if you don't want to > remove the option, we could surely provide a useful default setting > based on find_my_exec. (I'm amused to notice that pg_upgrade > currently takes the trouble to find out its own path, and then does > precisely nothing with the information.) Good point. You are right that when it was outside of the source tree, and even in /contrib, that would not have worked easily. Makes sense to at least default to the same directory as pg_upgrade. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
pg_upgrade version checking questions
While poking around trying to find an explanation for the pg_upgrade failure described here: https://www.postgresql.org/message-id/flat/CACmJi2JUhGo2ZxqDkh-EPHNjEN1ZA1S64uHLJFWHBhUuV4492w%40mail.gmail.com I noticed a few things that seem a bit fishy about pg_upgrade. I can't (yet) connect any of these to Tomasz' problem, but: 1. check_bin_dir() does validate_exec() for pg_dumpall and pg_dump, but not for pg_restore, though pg_upgrade surely calls that too. For that matter, it's not validating initdb and vacuumdb, though it's grown dependencies on those as well. Seems like there's little point in checking these if we're not going to check all of them. 2. check_cluster_versions() insists that the target version be the same major version as pg_upgrade itself, but is that really good enough? As things stand, it looks like pg_upgrade 11.3 would happily use pg_dump 11.1, or vice versa. With this rule, we cannot safely make any fixes in minor releases that rely on synchronized changes in the behavior of pg_upgrade and pg_dump/pg_dumpall/pg_restore. I've not gone looking to see if we've already made such changes in the past, but even if we never have, that's a rather tight-looking straitjacket. I think we should insist that the new_cluster.bin_version be an exact match to pg_upgrade's own PG_VERSION_NUM. 3. Actually, I'm kind of wondering why pg_upgrade has a --new-bindir option at all, rather than just insisting on finding the new-version executables in the same directory it is in. This seems like, at best, a hangover from before it got into core. Even if you don't want to remove the option, we could surely provide a useful default setting based on find_my_exec. (I'm amused to notice that pg_upgrade currently takes the trouble to find out its own path, and then does precisely nothing with the information.) Thoughts? regards, tom lane
