Re: Disable alternate locations on Win32

The following patch disables alternate locations on Win32 because it
doesn't have symlinks.
--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> The following patch disables alternate locations on Win32 because it
> doesn't have symlinks.

Why not do it with one ifdef in one place?

***************
*** 296,302 ****
--- 301,309 ----
/* Make the symlink, if needed */
if (alt_loc)
{
+ #ifndef WIN32
if (symlink(alt_loc, nominal_loc) != 0)
elog(ERROR, "CREATE DATABASE: could not link '%s' to '%s': %m",
nominal_loc, alt_loc);
+ #else
+ elog(ERROR, "CREATE DATABASE: may not use an alternate location on this platform");
+ #endif
}

Also I wonder if this shouldn't be conditionalized on something like
HAVE_SYMLINKS rather than a hardwired platform check.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > The following patch disables alternate locations on Win32 because it
> > doesn't have symlinks.
>
> Why not do it with one ifdef in one place?

It was done this way because they wanted to test earlier for failure,
and they didn't want the symlink call because they didn't have symlinks.
I didn't totally follow the code.

> if (alt_loc)
> {
> + #ifndef WIN32
> if (symlink(alt_loc, nominal_loc) != 0)
> elog(ERROR, "CREATE DATABASE: could not link '%s' to '%s': %m",
> nominal_loc, alt_loc);
> + #else
> + elog(ERROR, "CREATE DATABASE: may not use an alternate location on this platform");
> + #endif
> }
>
> Also I wonder if this shouldn't be conditionalized on something like
> HAVE_SYMLINKS rather than a hardwired platform check.

Yes, that would be better.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > The following patch disables alternate locations on Win32 because it
> > > doesn't have symlinks.
> >
> > Why not do it with one ifdef in one place?
>
> It was done this way because they wanted to test earlier for failure,
> and they didn't want the symlink call because they didn't have symlinks.
> I didn't totally follow the code.

I checked this again and the test is at the top, while the symlink()
call is at the bottom after the database has already been copied. I
think we do need to error out before we start copying the database.

> > if (alt_loc)
> > {
> > + #ifndef WIN32
> > if (symlink(alt_loc, nominal_loc) != 0)
> > elog(ERROR, "CREATE DATABASE: could not link '%s' to '%s': %m",
> > nominal_loc, alt_loc);
> > + #else
> > + elog(ERROR, "CREATE DATABASE: may not use an alternate location on this platform");
> > + #endif
> > }
> >
> > Also I wonder if this shouldn't be conditionalized on something like
> > HAVE_SYMLINKS rather than a hardwired platform check.
>
> Yes, that would be better.

OK, here is an applied patch that tests for symlink() rather than WIN32.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> Why not do it with one ifdef in one place?

> I checked this again and the test is at the top, while the symlink()
> call is at the bottom after the database has already been copied. I
> think we do need to error out before we start copying the database.

Huh? We haven't copied anything at that point. We have done the
mkdir() though. On spec-compliant platforms it'd be possible to
switch the order of these operations, so that the symlink comes before
mkdir. Does anyone know of a platform on which symlink() fails if the
referenced pathname doesn't exist yet?

regards, tom lane