Re: Moving tablespaces

Do we have any documentation about how to move a tablespace to a new
directory? If not, I think we should write some.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

On Sun, Dec 4, 2011 at 00:43, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Do we have any documentation about how to move a tablespace to a new
> directory?  If not, I think we should write some.

Do we have any support for doing it? (Yes, it works, but anything that
requires manual hacking of system catalogs really can't be considered
supported, can it?)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander wrote:
> On Sun, Dec 4, 2011 at 00:43, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Do we have any documentation about how to move a tablespace to a new
> > directory? ?If not, I think we should write some.
>
> Do we have any support for doing it? (Yes, it works, but anything that
> requires manual hacking of system catalogs really can't be considered
> supported, can it?)

True. It is something we just don't support? They have to dump, edit
the dump, and reload, to change a tablespace directory? Yikes. Is that
the state we are in? Has no one complained about this? They just use
symlinks?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

On Sun, Dec 4, 2011 at 17:12, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Magnus Hagander wrote:
>> On Sun, Dec 4, 2011 at 00:43, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> > Do we have any documentation about how to move a tablespace to a new
>> > directory? ?If not, I think we should write some.
>>
>> Do we have any support for doing it? (Yes, it works, but anything that
>> requires manual hacking of system catalogs really can't be considered
>> supported, can it?)
>
> True.  It is something we just don't support?  They have to dump, edit
> the dump, and reload, to change a tablespace directory?  Yikes.  Is that
> the state we are in?  Has no one complained about this?  They just use
> symlinks?

AFAIK, we don't. What you can do is take the db offline, change the
symlink, start it up agin and manually change
pg_tablespace.spclocation. That seems quite ugly though. And if you
forget one step, everything seems to work, but you have two
inconsistent definitions of the tablespace.

And IIRC, we don't actually *use* spclocation anywhere. How about we
just get rid of them as independents? We could either:

1) Remove the column. Rely on the symlink. Create a
pg_get_tablespace_location(oid) function, that could be used by
pg_dumpall and friends, that just reads the symlink.

2) Forcibly update the spclocation column when we start the server to
be whatever the symlink points to. That will at least automatically
restore the system to being consistent.

Option 1 would also make it a lot easier to in a supported way allow
tablespaces to have different locations on replica masters and slaves.
A tool like pg_basebackup could easily provide for something like
--relocate-tablespace mytblspc=/new/path and just rewrite the symlink
on the fly. But we cannot modify the pg_tablespace system catalog to
be different on the slave and the master...

It does seem rather obvious to me that this would be a win, so I'm
most likely missing something here. So please shoot a hole in the
theory for me ;)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> And IIRC, we don't actually *use* spclocation anywhere.

Just for pg_dump, I think.

> How about we
> just get rid of them as independents? We could either:

> 1) Remove the column. Rely on the symlink. Create a
> pg_get_tablespace_location(oid) function, that could be used by
> pg_dumpall and friends, that just reads the symlink.

Hm, how portable is symlink-reading? If we can actually do that
without big headaches, then +1.

> 2) Forcibly update the spclocation column when we start the server to
> be whatever the symlink points to. That will at least automatically
> restore the system to being consistent.

-1, running a transaction at that level will be a giant pita.
And how would you do it at all on a hot standby slave?

regards, tom lane

On Sun, Dec 4, 2011 at 17:41, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> And IIRC, we don't actually *use* spclocation anywhere.
>
> Just for pg_dump, I think.

pg_dumpall :-)

It's also used in pg_upgrade and pg_basebackup, but those are easily
dealt with if we define a function for it.

>> How about we
>> just get rid of them as independents? We could either:
>
>> 1) Remove the column. Rely on the symlink. Create a
>> pg_get_tablespace_location(oid) function, that could be used by
>> pg_dumpall and friends, that just reads the symlink.
>
> Hm, how portable is symlink-reading?  If we can actually do that
> without big headaches, then +1.

We already use readlink in a couple of places - including getting
called from find_my_exec. It's also used in pg_basebackup. The use in
find_my_exec is conditional on HAVE_READLINK, but not the one in
backend/replication/basebackup.c. An oversight from my side when
putting that in, but it shows that every single bf member we have now
has readlink - else the whole compilation would fail, AFAICT.

It's not directly portable to win32, but we have a wrapper function
for it in port/ already.

So I think we're fairly committed to having readlink already.

>> 2) Forcibly update the spclocation column when we start the server to
>> be whatever the symlink points to. That will at least automatically
>> restore the system to being consistent.
>
> -1, running a transaction at that level will be a giant pita.
> And how would you do it at all on a hot standby slave?

yeah, it would break that usage scenario, so I'm -1 for that one as well.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 12/04/2011 11:41 AM, Tom Lane wrote:
>
>> 1) Remove the column. Rely on the symlink. Create a
>> pg_get_tablespace_location(oid) function, that could be used by
>> pg_dumpall and friends, that just reads the symlink.
> Hm, how portable is symlink-reading? If we can actually do that
> without big headaches, then +1.
>

I wondered that, specifically about Windows junction points, but we seem
to have support for it already in dirmod.c::pgreadlink(). Surely there's
no other currently supported platform where it would even be a question?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 12/04/2011 11:41 AM, Tom Lane wrote:
>> Hm, how portable is symlink-reading? If we can actually do that
>> without big headaches, then +1.

> I wondered that, specifically about Windows junction points, but we seem
> to have support for it already in dirmod.c::pgreadlink(). Surely there's
> no other currently supported platform where it would even be a question?

readlink is required by Single Unix Spec v2 (1997), which is what we've
been treating as our baseline expectation for Unix-oid platforms for
awhile now. Given that we dealt with the Windows side already, I don't
see a problem with making this assumption. At worst we'd end up needing
a couple more emulations in src/port, since surely there's *some* way to
do it on any platform with symlinks.

regards, tom lane

On Sun, Dec 4, 2011 at 18:07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 12/04/2011 11:41 AM, Tom Lane wrote:
>>> Hm, how portable is symlink-reading?  If we can actually do that
>>> without big headaches, then +1.
>
>> I wondered that, specifically about Windows junction points, but we seem
>> to have support for it already in dirmod.c::pgreadlink(). Surely there's
>> no other currently supported platform where it would even be a question?
>
> readlink is required by Single Unix Spec v2 (1997), which is what we've
> been treating as our baseline expectation for Unix-oid platforms for
> awhile now.  Given that we dealt with the Windows side already, I don't
> see a problem with making this assumption.  At worst we'd end up needing
> a couple more emulations in src/port, since surely there's *some* way to
> do it on any platform with symlinks.

AFAICT, it should be as simple as the attached.

Doesn't include the required fixes for pg_upgrade, I'll get on those next.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> + snprintf(sourcepath, sizeof(sourcepath), "pg_tblspc/%d", tablespaceOid);

%u for an OID, please. Otherwise seems reasonably sane on first glance.

regards, tom lane

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> AFAICT, it should be as simple as the attached.

Oh, one other thought is that the function body has to be
conditionalized on HAVE_READLINK (the fact that you forgot that
somewhere else isn't an excuse for not doing it here). IIRC,
only the two built-in tablespaces can exist when not HAVE_READLINK,
so it seems sufficient to handle those cases and then PG_RETURN_NULL
(or maybe throw error) when not HAVE_READLINK.

regards, tom lane

On Tue, Dec 6, 2011 at 16:12, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> AFAICT, it should be as simple as the attached.
>
> Oh, one other thought is that the function body has to be
> conditionalized on HAVE_READLINK (the fact that you forgot that
> somewhere else isn't an excuse for not doing it here).  IIRC,
> only the two built-in tablespaces can exist when not HAVE_READLINK,
> so it seems sufficient to handle those cases and then PG_RETURN_NULL
> (or maybe throw error) when not HAVE_READLINK.

Hmm. good point.

Throwing an error seems a lot more safe in this case than just
returning NULL. Since it's a situtation that really shouldn't happen.
Maybe an assert, but I think a regular ereport(ERROR) would be the
best.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Throwing an error seems a lot more safe in this case than just
> returning NULL. Since it's a situtation that really shouldn't happen.
> Maybe an assert, but I think a regular ereport(ERROR) would be the
> best.

Not an assert, since it's trivially user-triggerable.

regards, tom lane

On Tue, Dec 6, 2011 at 16:17, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Throwing an error seems a lot more safe in this case than just
>> returning NULL. Since it's a situtation that really shouldn't happen.
>> Maybe an assert, but I think a regular ereport(ERROR) would be the
>> best.
>
> Not an assert, since it's trivially user-triggerable.

Right.

There is some nice precedent in the CREATE TABLESPACE command (though
dependent on HAVE_SYMLINK and not HAVE_READLINK), so I'm just going to
copy the error message from there.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> There is some nice precedent in the CREATE TABLESPACE command (though
> dependent on HAVE_SYMLINK and not HAVE_READLINK), so I'm just going to
> copy the error message from there.

Fair enough.

Looking at the existing readlink use in port/exec.c, it strikes me that
another thing you'd better do is include a check for buffer overrun,
ie the test needs to be more like

rllen = readlink(fname, link_buf, sizeof(link_buf));
if (rllen < 0 || rllen >= sizeof(link_buf))
... fail ...

Also, you're assuming that the result is already null-terminated,
which is incorrect.

regards, tom lane

On Tue, Dec 6, 2011 at 17:07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> There is some nice precedent in the CREATE TABLESPACE command (though
>> dependent on HAVE_SYMLINK and not HAVE_READLINK), so I'm just going to
>> copy the error message from there.
>
> Fair enough.
>
> Looking at the existing readlink use in port/exec.c, it strikes me that
> another thing you'd better do is include a check for buffer overrun,
> ie the test needs to be more like
>
>                rllen = readlink(fname, link_buf, sizeof(link_buf));
>                if (rllen < 0 || rllen >= sizeof(link_buf))
>                        ... fail ...

Seems reasonable, yeah. I'll go put a similar check in the
basebackup.c file as well when I'm done here.

> Also, you're assuming that the result is already null-terminated,
> which is incorrect.

No, I'm not - I'm MemSet()ing the whole buffer to 0 before I start.
But I'll change that to work the same way as the on in port/exec.c,
for consistency.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Wed, Dec 7, 2011 at 10:05, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Tue, Dec 6, 2011 at 17:07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>> There is some nice precedent in the CREATE TABLESPACE command (though
>>> dependent on HAVE_SYMLINK and not HAVE_READLINK), so I'm just going to
>>> copy the error message from there.
>>
>> Fair enough.
>>
>> Looking at the existing readlink use in port/exec.c, it strikes me that
>> another thing you'd better do is include a check for buffer overrun,
>> ie the test needs to be more like
>>
>>                rllen = readlink(fname, link_buf, sizeof(link_buf));
>>                if (rllen < 0 || rllen >= sizeof(link_buf))
>>                        ... fail ...
>
> Seems reasonable, yeah. I'll go put a similar check in the
> basebackup.c file as well when I'm done here.

To close this thread (hopefully): Fixed and applied.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/