Re: About pg_upgrade - Mailing list pgsql-hackers

Peter Eisentraut wrote:
> I was just looking at the latest pg_upgrade revision.  Maybe it works for
> particular installations, but before we can think about releasing this, at
> least the issues below need to be addressed.
> 
> It seems to require that the data directory is under a directory named
> "data" and that your current directory is above that.  This is not
> appropriate for real software.  I suggest using standard -D options and/or
> the PGDATA environment variable.

I have added code to use PGDATA, which can be overridden with a new -D
option, as you suggested.

> The locations of directories like the INFODIR and the OLDDIR should be
> configurable.  Don't just put them where they happen to fall.  (Remember
> that possibly a lot of data will end up there.)

Well, actually, we want the INFODIR and SAVEDATA (was OLDDIR) to be in
the same filesystem as PGDATA.  In fact, we use mv so the data files are
just moved into different directories and not actually moved.  I added
code to put these in the directory _above_ PGDATA.  Does that help?

> The way temp files are allocated looks very insecure.  And does this thing
> even check under what user it's running?

It does now --- It checks for root user and makes sure you can read the
PGDATA directory.

> 'test -e' is not portable.

OK, changed to -f and -h.

> 'grep -q' is not portable.  (At least it doesn't portably behave as you
> might think it does.)

OK, changed to grep "" > /dev/null 2>&1

> Although 'head' is probably portable, it has never been explored, because
> 'sed 1q' is better.

I changed it so "sed -n '1p'" which is a more general solution;  sed
'Xq' only works for the top X lines, i.e. sed '2q' prints lines 1 and 2.

> If you set an exit trap, then 'exit 1' is not portable.  (I'm not kidding,
> see pg_regress.)

Fixed -- I change 'exit 1' to 'return 1' and changed the calls to:
func || exit "$?"

> You can't nest " inside ` inside ".

Can you show me where?  I don't see it.

> Pattern matching with 'expr' should be invoked like this:  expr x"$OBJ" :
> x'pg_', or it might blow up when $OBJ has funny values.

Done.

> Moving directories with 'mv' is not necessarily a good idea.

Can you be more specific?  Seems pretty portable.

> Should do a lot more error checking.

Suggestions?  Already lots in there.

> psql, pg_ctl, etc. should not just be called from the path.  You know the
> install directory, so use that.

Well, we do use the same script on the old install and the new one so I
am not sure if looking at the install or coupling it with 'configure'
run is a good idea --- it may cause more problems than it solves.

> awk should be called as determined by configure.

Again, requires configure handling.

> Poor quality (spelling, wording) of messages.

Improved.  Let me know if I missed something.

> The man page is very confusing.

I have improved it.  All moved to /contrib/pg_upgrade.


--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026