pg_upgrade version checking questions

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

pg_upgrade version checking questions

Tom Lane-2
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

Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Bruce Momjian
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  <[hidden email]>        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 +

Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Bruce Momjian
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  <[hidden email]>        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 +

Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Daniel Gustafsson
On Tuesday, March 19, 2019 7:55 AM, Bruce Momjian <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Peter Eisentraut-6
In reply to this post by Tom Lane-2
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

Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Tom Lane-2
Peter Eisentraut <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Christoph Berg-2
Re: Peter Eisentraut 2019-03-22 <[hidden email]>
> 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

Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Daniel Gustafsson
In reply to this post by Tom Lane-2
On Tuesday, March 19, 2019 12:35 AM, Tom Lane <[hidden email]> 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 (1K) Download Attachment
0003-Default-new-bindir-to-CWD.patch (5K) Download Attachment
0001-Only-allow-upgrades-by-the-same-exact-version-new-bi.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Christoph Berg-2
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?

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


Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Daniel Gustafsson
On Wednesday, March 27, 2019 1:43 PM, Christoph Berg <[hidden email]> 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 (2K) Download Attachment
0002-Check-all-used-executables-v2.patch (1K) Download Attachment
0003-Default-new-bindir-to-exec_path-v2.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Peter Eisentraut-6
On 2019-04-04 15:40, Daniel Gustafsson wrote:

> On Wednesday, March 27, 2019 1:43 PM, Christoph Berg <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Daniel Gustafsson
> On 22 Jul 2019, at 10:46, Peter Eisentraut <[hidden email]> wrote:

>
> On 2019-04-04 15:40, Daniel Gustafsson wrote:
>> On Wednesday, March 27, 2019 1:43 PM, Christoph Berg <[hidden email]> 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 (6K) Download Attachment
0002-Check-all-used-executables-v3.patch (2K) Download Attachment
0001-Only-allow-upgrades-by-the-same-exact-version-new-v3.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Peter Eisentraut-6
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


Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Daniel Gustafsson
> On 24 Jul 2019, at 22:32, Peter Eisentraut <[hidden email]> 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 (768 bytes) Download Attachment
0003-Default-new-bindir-to-exec_path-v4.patch (10K) Download Attachment
0002-Check-all-used-executables-v4.patch (2K) Download Attachment
0001-Only-allow-upgrades-by-the-same-exact-version-new-v4.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Peter Eisentraut-6
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


Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Daniel Gustafsson
> On 27 Jul 2019, at 08:42, Peter Eisentraut <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Thomas Munro-5
On Wed, Jul 31, 2019 at 3:13 AM Daniel Gustafsson <[hidden email]> wrote:

> > On 27 Jul 2019, at 08:42, Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Alvaro Herrera-9
In reply to this post by Daniel Gustafsson
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


Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade version checking questions

Daniel Gustafsson
> On 2 Sep 2019, at 19:59, Alvaro Herrera <[hidden email]> 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

12