pg_upgrade version check improvements and small fixes

Dan McGee wrote:

Not sure what the normal process is for patches, but I put together a
few small patches for pg_upgrade after trying to use it earlier today
and staring a non-helpful error message before I finally figured out
what was going on.

Thanks for the detailed report and patches. Let me address each one
individually.

0001 is just a simple typo fix, but didn't want to mix it in with the rest.

I applied this message capitalization fix to 9.0, 9.1, and master (9.2).

0002 moves a function around to be declared in the only place it is
needed, and prevents a "sh: /oasdfpt/pgsql-8.4/bin/pg_config: No such
file or directory" error message when you give it a bogus bindir.

You are right that I was calling pg_ctl before I had validated the bin
directory, and you were right that the function wasn't in an ideal
C file.

What I did was to move the function, and bundle all the
pg_upgrade_support test into that single function, so the function now
either errors out or returns void.

Patch attached and applied to 9.1 and master. Good catch.

0003 is what I really wanted to solve, which was my failure with
pg_upgrade. The call to pg_ctl didn't succeed because the binaries
didn't match the data directory, thus resulting in this:

$ pg_upgrade --check -d /tmp/olddata -D /tmp/newdata -b /usr/bin/ -B /usr/bin/
Performing Consistency Checks
-----------------------------
Checking old data directory (/tmp/olddata) ok
Checking old bin directory (/usr/bin) ok
Checking new data directory (/tmp/newdata) ok
Checking new bin directory (/usr/bin) ok
pg_resetxlog: pg_control exists but is broken or unknown version; ignoring it
Trying to start old server .................ok

Unable to start old postmaster with the command: "/usr/bin/pg_ctl" -l
"/dev/null" -D "/tmp/olddata" -o "-p 5432 -c autovacuum=off -c
autovacuum_freeze_max_age=2000000000" start >> "/dev/null" 2>&1
Perhaps pg_hba.conf was not set to "trust".

The error had nothing to do with "trust" at all; it was simply that I
tried to use 9.0 binaries with an 8.4 data directory. My patch checks
for this and ensures that the -D bindir is the correct version, just
as the -B datadir has to be the correct version.

I had not thought about testing if the bin and data directory were the
same major version, but you are right that it generates an odd error if
they are not.

I changed your sscanf format string because the version can be just
"9.2dev" and there is no need for the minor version. I saw no reason
to test if the binary version matches the pg_upgrade version because we
already test the cluster version, and we test the cluster version is the
same as the binary version.

Patch attached and applied to 9.1 and master.

I'm not on the mailing list nor do I have a lot of free time to keep
up with normal development, but if there are quick things I can do to
get these patches in let me know.

All done!

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Wed, Jun 22, 2011 at 8:54 PM, Bruce Momjian <bruce@momjian.us> wrote:

0003 is what I really wanted to solve, which was my failure with
pg_upgrade. The call to pg_ctl didn't succeed because the binaries
didn't match the data directory, thus resulting in this:

The error had nothing to do with "trust" at all; it was simply that I
tried to use 9.0 binaries with an 8.4 data directory. My patch checks
for this and ensures that the -D bindir is the correct version, just
as the -B datadir has to be the correct version.

I had not thought about testing if the bin and data directory were the
same major version, but you are right that it generates an odd error if
they are not.

I changed your sscanf format string because the version can be just
"9.2dev" and there is no need for the minor version.

No problem- you're going to know way more about this than me, and I
was just going off what I saw in my two installed versions and a quick
look over at the code of pg_ctl to make sure that string was never
translated.

On a side note I don't think I ever mentioned to everyone else why
parsing the version from pg_ctl rather than pg_config was done- this
is so we don't introduce any additional binary requirements. Tom Lane
noted in an earlier cleanup series that pg_config is not really needed
at all in the old cluster, so I wanted to keep it that way, but pg_ctl
has always been required.

  I saw no reason
to test if the binary version matches the pg_upgrade version because we
already test the cluster version, and we test the cluster version is the
same as the binary version.

Duh. That makes sense. Thanks for getting to these so quickly!

To partially toot my own horn but also show where a user like me
encountered this, after some packaging hacking, anyone running Arch
Linux should be able to do a pg_upgrade from here on out by installing
the postgresql-old-upgrade package
(http://www.archlinux.org/packages/extra/x86_64/postgresql-old-upgrade/)
and possible consulting the Arch wiki.

-Dan

Dan McGee wrote:

On Wed, Jun 22, 2011 at 8:54 PM, Bruce Momjian <bruce@momjian.us> wrote:

0003 is what I really wanted to solve, which was my failure with
pg_upgrade. The call to pg_ctl didn't succeed because the binaries
didn't match the data directory, thus resulting in this:

The error had nothing to do with "trust" at all; it was simply that I
tried to use 9.0 binaries with an 8.4 data directory. My patch checks
for this and ensures that the -D bindir is the correct version, just
as the -B datadir has to be the correct version.

I had not thought about testing if the bin and data directory were the
same major version, but you are right that it generates an odd error if
they are not.

I changed your sscanf format string because the version can be just
"9.2dev" and there is no need for the minor version.

No problem- you're going to know way more about this than me, and I
was just going off what I saw in my two installed versions and a quick
look over at the code of pg_ctl to make sure that string was never
translated.

On a side note I don't think I ever mentioned to everyone else why
parsing the version from pg_ctl rather than pg_config was done- this
is so we don't introduce any additional binary requirements. Tom Lane
noted in an earlier cleanup series that pg_config is not really needed
at all in the old cluster, so I wanted to keep it that way, but pg_ctl
has always been required.

Yes, I looked specifically for that. We could have used pg_controldata
too, but pg_ctl seemed just as good.

? I saw no reason
to test if the binary version matches the pg_upgrade version because we
already test the cluster version, and we test the cluster version is the
same as the binary version.

Duh. That makes sense. Thanks for getting to these so quickly!

To partially toot my own horn but also show where a user like me
encountered this, after some packaging hacking, anyone running Arch
Linux should be able to do a pg_upgrade from here on out by installing
the postgresql-old-upgrade package
(http://www.archlinux.org/packages/extra/x86_64/postgresql-old-upgrade/)
and possible consulting the Arch wiki.

Great.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +