Bruce Momjian writes:
> 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?
Hmm, I think that's actually worse, because the directory above PGDATA
might in fact be a different file system. It's probably better to keep it
within one directory then. And using "dirname" isn't portable anyway.
> > 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.
Still not sure about those temp files. People like to see a possible
exploit in every temp file.
> > 'test -e' is not portable.
>
> OK, changed to -f and -h.
-h isn't portable either. I think you have to do something like
if (test -L "$name") >/dev/null 2>&1 && (test -h "$name") >/dev/null 2>&1
If should also add here that "if ! command" is not portable, you'd need to
write "if command; then : ; else action fi"
> > 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 "$?"
That doesn't matter. You have to write (exit x); exit x so the exit code
is actually seen outside pg_upgrade.
> > You can't nest " inside ` inside ".
>
> Can you show me where? I don't see it.
Line 87 looks suspicious. Especially with the echo and backslash in there
it is not very portable.
> > Moving directories with 'mv' is not necessarily a good idea.
>
> Can you be more specific? Seems pretty portable.
I was only concerned about moving across file systems, which you're not
doing.
> > Should do a lot more error checking.
>
> Suggestions? Already lots in there.
It looks better already.
> > 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.
It's better than using some random tool that might not even be in the
path. You have to run configure anyway, so what's the point in avoiding
it?
--
Peter Eisentraut peter_e@gmx.net
