Re: pg_upgrade version checking questions

While poking around trying to find an explanation for the pg_upgrade
failure described here:
/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

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:
> /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 <bruce(at)momjian(dot)us> 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 +

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 <bruce(at)momjian(dot)us> 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 +

On Tuesday, March 19, 2019 7:55 AM, Bruce Momjian <bruce(at)momjian(dot)us> 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

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:
/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

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> 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:
> /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

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:
>> /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: Peter Eisentraut 2019-03-22 <57769959-8960-a9ca-fc9c-4dbb12629b8a(at)2ndquadrant(dot)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

On Tuesday, March 19, 2019 12:35 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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

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?

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

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).

cheers ./daniel

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).

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

> 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

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

> On 24 Jul 2019, at 22:32, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> 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

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

> On 27 Jul 2019, at 08:42, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> 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

On Wed, Jul 31, 2019 at 3:13 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> > On 27 Jul 2019, at 08:42, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> 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

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 Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 2 Sep 2019, at 19:59, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> 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

> On 27 Jul 2019, at 08:42, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> 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/

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.

> On 2 Mar 2021, at 14:20, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> 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/

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.

> On 3 Mar 2021, at 09:57, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> 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/