Re: About pg_upgrade

Peter Eisentraut wrote:
> 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.

Well, actually, it may not be better or worse. We actually use PGDATA
to point not to the top-level PostgreSQL directory but to the /data part
of that directory, so the old behavior was to put it above /data and the
new code will do exactly that too. Am I confused?

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

Well, yes, if you get the pid, you can create symlink files in /tmp and
overwrite things. How do I handle this properly, probably a directory
in /tmp that I create but I have to set my umask first -- is that a
plan?

>
> > > 'test -e' is not portable.
> >
> > OK, changed to -f and -h.
>
> -h isn't portable either. I think you have to do something like

Oh, great!

>
> if (test -L "$name") >/dev/null 2>&1 && (test -h "$name") >/dev/null 2>&1

I am confused about this, I don't have -L and the code seems to say that
the second part executes only if the first part is OK --- I don't really
need the test, I could just try the 'mv' and report a failure.

> If should also add here that "if ! command" is not portable, you'd need to
> write "if command; then : ; else action fi"

Wow, I have written portable scripts but never went to these lengths ---
are there actually that many broken OS's?

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

So 'return 1' doesn't propogate to the caller?

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

Wow, that command was really stupid --- I just changed it to make the
assignment without the 'echo.'

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

I sure hope so. :-) If not, the script will fail.

>
> > > Should do a lot more error checking.
> >
> > Suggestions? Already lots in there.
>
> It looks better already.

Good.

> > > 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?

But I don't assume they have run configure on the new source tree when
pg_upgrade is run in phase one --- very likely they will grab
pg_upgrade, do phase one, then configure/compile the new code and
install it as part of the upgrade steps.

If they don't have a working 'awk' in their path, they have bigger
problems than pg_upgrade not working --- I know there are quirky
problems with awk (I saw them in pgmonitor) but these are vanilla awk
scripts that would work on any one.

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