Re: [PATCH] Relocation of tablespaces in pg_basebackup

Currently pg_basebackup is pretty invasive when using tablespaces, at
least using the plain format. This since it requires the tablespace to
be written to the same location as on the server beeing backed up. This
both breaks backing up locally using -Fp (since the tablespace would
be written to the same location) and requires the backup user to have
write permissions in locations it shouldn't need to have access to.

This patch adds the ability to relocate tablespaces by adding the
command line argument --tablespace (-T) which takes a required argument
in the format "oid:tablespacedir". After all tablespaces are fetched
this code updates the symlink to point to the new tablespace location.

I would have loved to be able to pass tablespacename:tablespacedir
though, but sadly I wasn't able to figure out how to retrieve that
information without creating another connection to the database.

This feature might be missing because of some other limitation I fail
to see, if so let me know. Please be gentle, this is my first patch ;-)

On 01/09/2014 06:58 PM, Steeve Lennmark wrote:
> This patch adds the ability to relocate tablespaces by adding the
> command line argument --tablespace (-T) which takes a required argument
> in the format "oid:tablespacedir". After all tablespaces are fetched
> this code updates the symlink to point to the new tablespace location.
>
> I would have loved to be able to pass tablespacename:tablespacedir
> though, but sadly I wasn't able to figure out how to retrieve that
> information without creating another connection to the database.

This feature would be a nice addition to pg_basebackup, and I agree with
that it would be preferable to use names of oids if possible.

> This feature might be missing because of some other limitation I fail
> to see, if so let me know. Please be gentle, this is my first patch ;-)

It seems like you have attached the wrong patch. The only attachment I
see is 0001-SQL-assertions-prototype.patch.

Best regards,
Andreas

--
Andreas Karlsson

Yes, apparently my virgin flight crashed and burn. I here attach the
correct file!

//Steeve

On Thu, Jan 9, 2014 at 7:16 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:

> On 01/09/2014 06:58 PM, Steeve Lennmark wrote:
> > This patch adds the ability to relocate tablespaces by adding the
>
>> command line argument --tablespace (-T) which takes a required argument
>> in the format "oid:tablespacedir". After all tablespaces are fetched
>> this code updates the symlink to point to the new tablespace location.
>>
>> I would have loved to be able to pass tablespacename:tablespacedir
>> though, but sadly I wasn't able to figure out how to retrieve that
>> information without creating another connection to the database.
>>
>
> This feature would be a nice addition to pg_basebackup, and I agree with
> that it would be preferable to use names of oids if possible.
>
>
> This feature might be missing because of some other limitation I fail
>> to see, if so let me know. Please be gentle, this is my first patch ;-)
>>
>
> It seems like you have attached the wrong patch. The only attachment I see
> is 0001-SQL-assertions-prototype.patch.
>
> Best regards,
> Andreas
>
> --
> Andreas Karlsson
>

On Thu, Jan 9, 2014 at 6:58 PM, Steeve Lennmark
<steevel(at)handeldsbanken(dot)se>wrote:

> Currently pg_basebackup is pretty invasive when using tablespaces, at
> least using the plain format. This since it requires the tablespace to
> be written to the same location as on the server beeing backed up. This
> both breaks backing up locally using -Fp (since the tablespace would
> be written to the same location) and requires the backup user to have
> write permissions in locations it shouldn't need to have access to.
>

Yeah, this has been sitting on my TODO for a long time :) Glad to see
someone is picking it up.

This patch adds the ability to relocate tablespaces by adding the
> command line argument --tablespace (-T) which takes a required argument
> in the format "oid:tablespacedir". After all tablespaces are fetched
> this code updates the symlink to point to the new tablespace location.
>
> I would have loved to be able to pass tablespacename:tablespacedir
> though, but sadly I wasn't able to figure out how to retrieve that
> information without creating another connection to the database.
>

You could also use the format "olddir:newdir", because you do know that.
It's not the name of the tablespace. but I think it's still more
usefriendly than using the oid.

This feature might be missing because of some other limitation I fail
> to see, if so let me know. Please be gentle, this is my first patch ;-)
>

Nope, I think it's just been limited on time.

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

On Thu, Jan 9, 2014 at 7:18 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:

> On Thu, Jan 9, 2014 at 6:58 PM, Steeve Lennmark <steevel(at)handeldsbanken(dot)se
> > wrote:
>
>> This patch adds the ability to relocate tablespaces by adding the
>> command line argument --tablespace (-T) which takes a required argument
>> in the format "oid:tablespacedir". After all tablespaces are fetched
>> this code updates the symlink to point to the new tablespace location.
>>
>> I would have loved to be able to pass tablespacename:tablespacedir
>> though, but sadly I wasn't able to figure out how to retrieve that
>> information without creating another connection to the database.
>>
>
> You could also use the format "olddir:newdir", because you do know that.
> It's not the name of the tablespace. but I think it's still more
> usefriendly than using the oid.
>

That's a much better solution, I attached a patch with the updated code.

# SELECT oid, pg_tablespace_location(oid) FROM pg_tablespace;
[...]
16388 | /tmp/tblspc1
16389 | /tmp/tblspc2

$ pg_basebackup -Xs -D backup/data -T /tmp/tblspc1:$(pwd)/backup/t1 -T
/tmp/tblspc2:$(pwd)/backup/t2

This produces the following now:
$ ls backup/; ls -l backup/data/pg_tblspc/
data t1 t2
lrwxrwxrwx 1 steevel users 23 Jan 9 20:41 16388 -> /home/steevel/backup/t1
lrwxrwxrwx 1 steevel users 23 Jan 9 20:41 16389 -> /home/steevel/backup/t2

--
Steeve Lennmark

Hi Steeve,
> Il 09/01/14 22:10, Steeve Lennmark ha scritto:
>
> That's a much better solution, I attached a patch with the updated code.
>
> # SELECT oid, pg_tablespace_location(oid) FROM pg_tablespace;
> [...]
> 16388 | /tmp/tblspc1
> 16389 | /tmp/tblspc2

I'd suggest, a similar solution to the one we have adopted in Barman (if
you don't know it: www.pgbarman.org), that is:

--tablespace NAME:LOCATION [--tablespace NAME:location]

I prefer this over the location on the master as this might change over
time (at least more frequently than the tablespace name) and over servers.

> $ pg_basebackup -Xs -D backup/data -T /tmp/tblspc1:$(pwd)/backup/t1 -T
> /tmp/tblspc2:$(pwd)/backup/t2

With the above example, it would become:

$ pg_basebackup -Xs -D backup/data -T tblspc1:$(pwd)/backup/t1 -T
tblspc2:$(pwd)/backup/t2

Thanks,
Gabriele

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele(dot)bartolini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

On Thu, Jan 9, 2014 at 10:29 PM, Gabriele Bartolini <
gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:

> Hi Steeve,
> > Il 09/01/14 22:10, Steeve Lennmark ha scritto:
> >
> > That's a much better solution, I attached a patch with the updated code.
> >
> > # SELECT oid, pg_tablespace_location(oid) FROM pg_tablespace;
> > [...]
> > 16388 | /tmp/tblspc1
> > 16389 | /tmp/tblspc2
>
> I'd suggest, a similar solution to the one we have adopted in Barman (if
> you don't know it: www.pgbarman.org), that is:
>
> --tablespace NAME:LOCATION [--tablespace NAME:location]
>
> I prefer this over the location on the master as this might change over
> time (at least more frequently than the tablespace name) and over servers.

I'm a barman user myself so that was actually my initial thought. If
there aren't some kind of hidden internal that I've missed I don't see
a way to convert an OID (only have OID and path at this stage) to a
tablespace name. This solution, even though not optimal, is a lot
better than my initial one where I used the OID directly.

> > $ pg_basebackup -Xs -D backup/data -T /tmp/tblspc1:$(pwd)/backup/t1 -T
> > /tmp/tblspc2:$(pwd)/backup/t2
>
> With the above example, it would become:
>
> $ pg_basebackup -Xs -D backup/data -T tblspc1:$(pwd)/backup/t1 -T
> tblspc2:$(pwd)/backup/t2
>

Yeah, that would be my favourite solution.

Regards,
Steeve
--
Steeve Lennmark

Hi Steeve,

Il 09/01/14 22:38, Steeve Lennmark ha scritto:
> I'm a barman user myself so that was actually my initial thought.

Ah! Very good!
> If there aren't some kind of hidden internal that I've missed I don't see
> a way to convert an OID (only have OID and path at this stage) to a
> tablespace name. This solution, even though not optimal, is a lot
> better than my initial one where I used the OID directly.

Try:

SELECT spcname, oid, pg_tablespace_location(oid) FROM pg_tablespace

Thanks,
Gabriele

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele(dot)bartolini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

On Fri, Jan 10, 2014 at 12:25 PM, Gabriele Bartolini <
gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:

> Hi Steeve,
>
> Il 09/01/14 22:38, Steeve Lennmark ha scritto:
> > I'm a barman user myself so that was actually my initial thought.
>
> Ah! Very good!
> > If there aren't some kind of hidden internal that I've missed I don't see
> > a way to convert an OID (only have OID and path at this stage) to a
> > tablespace name. This solution, even though not optimal, is a lot
> > better than my initial one where I used the OID directly.
>
> Try:
>
> SELECT spcname, oid, pg_tablespace_location(oid) FROM pg_tablespace
>
>
That would require a second connection to the database. You cannot run that
query from the walsender session. And that's exactly the issue that Steeve
pointed out in his first email.

I think it's better to let pg_basebackup work at the lower level, and then
leave it to higher level tools to be able to do the mapping to names.

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

On 2014-01-10 12:27:23 +0100, Magnus Hagander wrote:
> On Fri, Jan 10, 2014 at 12:25 PM, Gabriele Bartolini <
> gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:
>
> > Hi Steeve,
> >
> > Il 09/01/14 22:38, Steeve Lennmark ha scritto:
> > > I'm a barman user myself so that was actually my initial thought.
> >
> > Ah! Very good!
> > > If there aren't some kind of hidden internal that I've missed I don't see
> > > a way to convert an OID (only have OID and path at this stage) to a
> > > tablespace name. This solution, even though not optimal, is a lot
> > > better than my initial one where I used the OID directly.
> >
> > Try:
> >
> > SELECT spcname, oid, pg_tablespace_location(oid) FROM pg_tablespace
> >
> >
> That would require a second connection to the database. You cannot run that
> query from the walsender session. And that's exactly the issue that Steeve
> pointed out in his first email.

Theoretically nothing is stopping us from providing a command outputting
that information - it's a global catalog, so we can access it without
problems.

> I think it's better to let pg_basebackup work at the lower level, and then
> leave it to higher level tools to be able to do the mapping to names.

That doesn't negate this argument though. Not really convinced either
way yet.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 01/09/2014 10:10 PM, Steeve Lennmark wrote:
> That's a much better solution, I attached a patch with the updated code.
>
> # SELECT oid, pg_tablespace_location(oid) FROM pg_tablespace;
> [...]
> 16388 | /tmp/tblspc1
> 16389 | /tmp/tblspc2
>
> $ pg_basebackup -Xs -D backup/data -T /tmp/tblspc1:$(pwd)/backup/t1 -T
> /tmp/tblspc2:$(pwd)/backup/t2
>
> This produces the following now:
> $ ls backup/; ls -l backup/data/pg_tblspc/
> data t1 t2
> lrwxrwxrwx 1 steevel users 23 Jan 9 20:41 16388 -> /home/steevel/backup/t1
> lrwxrwxrwx 1 steevel users 23 Jan 9 20:41 16389 -> /home/steevel/backup/t2

Looked at the patch quickly and noticed that it does not support paths
containing colons. Is that an acceptable limitation? The $PATH variable
in most UNIX shells does not support paths with colons either so such
naming of directories is already discouraged.

Feel free to add the patch to the upcoming commitfest when you feel it
is ready for a review.

--
Andreas Karlsson

Andreas Karlsson wrote:
> On 01/09/2014 10:10 PM, Steeve Lennmark wrote:
> >That's a much better solution, I attached a patch with the updated code.
>
> Looked at the patch quickly and noticed that it does not support
> paths containing colons. Is that an acceptable limitation?

Well, clearly it won't work on Windows when tablespaces are on different
drives, so it doesn't sound so acceptable.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Jan 13, 2014 at 4:29 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:

> On 01/09/2014 10:10 PM, Steeve Lennmark wrote:
>
>> That's a much better solution, I attached a patch with the updated code.
>>
>> # SELECT oid, pg_tablespace_location(oid) FROM pg_tablespace;
>> [...]
>> 16388 | /tmp/tblspc1
>> 16389 | /tmp/tblspc2
>>
>> $ pg_basebackup -Xs -D backup/data -T /tmp/tblspc1:$(pwd)/backup/t1 -T
>> /tmp/tblspc2:$(pwd)/backup/t2
>>
>> This produces the following now:
>> $ ls backup/; ls -l backup/data/pg_tblspc/
>> data t1 t2
>> lrwxrwxrwx 1 steevel users 23 Jan 9 20:41 16388 ->
>> /home/steevel/backup/t1
>> lrwxrwxrwx 1 steevel users 23 Jan 9 20:41 16389 ->
>> /home/steevel/backup/t2
>>
>
> Looked at the patch quickly and noticed that it does not support paths
> containing colons. Is that an acceptable limitation? The $PATH variable in
> most UNIX shells does not support paths with colons either so such naming
> of directories is already discouraged.
>

I thought of this too and wrote a patch for that yesterday, I've
attached an updated version which supports passing in a path with
escaped colons.

Feel free to add the patch to the upcoming commitfest when you feel it is
> ready for a review.

Done!

Thanks,
--
Steeve Lennmark

On Mon, Jan 13, 2014 at 6:13 AM, Steeve Lennmark
<steevel(at)handeldsbanken(dot)se>wrote:

> On Mon, Jan 13, 2014 at 4:29 AM, Andreas Karlsson <andreas(at)proxel(dot)se>wrote:
>
>> On 01/09/2014 10:10 PM, Steeve Lennmark wrote:
>>
>>> That's a much better solution, I attached a patch with the updated code.
>>>
>>> # SELECT oid, pg_tablespace_location(oid) FROM pg_tablespace;
>>> [...]
>>> 16388 | /tmp/tblspc1
>>> 16389 | /tmp/tblspc2
>>>
>>> $ pg_basebackup -Xs -D backup/data -T /tmp/tblspc1:$(pwd)/backup/t1 -T
>>> /tmp/tblspc2:$(pwd)/backup/t2
>>>
>>> This produces the following now:
>>> $ ls backup/; ls -l backup/data/pg_tblspc/
>>> data t1 t2
>>> lrwxrwxrwx 1 steevel users 23 Jan 9 20:41 16388 ->
>>> /home/steevel/backup/t1
>>> lrwxrwxrwx 1 steevel users 23 Jan 9 20:41 16389 ->
>>> /home/steevel/backup/t2
>>>
>>
>> Looked at the patch quickly and noticed that it does not support paths
>> containing colons. Is that an acceptable limitation? The $PATH variable in
>> most UNIX shells does not support paths with colons either so such naming
>> of directories is already discouraged.
>>
>
> I thought of this too and wrote a patch for that yesterday, I've
> attached an updated version which supports passing in a path with
> escaped colons.
>

Seems I forgot to change the sgml after the syntax change, here's an
updated patch.

--
Steeve Lennmark

Please keep the --help and the options in the SGML table in alphabetical
order within their respective sections.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Eyeballing this patch, three thoughts:

1. I wonder whether ilist.c/h should be moved to src/common so that
frontend code could use it.

2. I wonder whether ilist.c should gain the ability to have
singly-linked lists with a pointer to the tail node for appending to the
end. This code would use it, and also the code doing postgresql.conf
parsing which has head/tail pointers all over the place. I'm sure there
are other uses.

3. How many definitions of atooid() do we have now?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi,

On 2014-01-16 11:25:59 -0300, Alvaro Herrera wrote:
> Eyeballing this patch, three thoughts:
>
> 1. I wonder whether ilist.c/h should be moved to src/common so that
> frontend code could use it.

Sounds like a good idea. There's some debugging checks that elog, but
that should be fixable easily.

> 2. I wonder whether ilist.c should gain the ability to have
> singly-linked lists with a pointer to the tail node for appending to the
> end. This code would use it, and also the code doing postgresql.conf
> parsing which has head/tail pointers all over the place. I'm sure there
> are other uses.

I am not generaly adverse to it, but neither of those usecases seems to
warrant that. They just should use a doubly linked list, it's not like
the memory/runtime overhead is significant. And the implementation
overhead doesn't count either if they use ilist.h.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro,
On Thu, Jan 16, 2014 at 3:20 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:

> Please keep the --help and the options in the SGML table in alphabetical
> order within their respective sections.

Ah, I failed to see that there was sub groups and thought the options
where not alphabetically ordered. This patch fixes that.

--
Steeve Lennmark

Alvaro,
On Thu, Jan 16, 2014 at 3:25 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:

> Eyeballing this patch, three thoughts:
>
> 1. I wonder whether ilist.c/h should be moved to src/common so that
> frontend code could use it.
>

That would be nice, I guess lack of helpers is why a lot of stuff is
using pgdumputils.h from src/bin/pg_dump.

$ git grep -l dumputils.h src/bin/{psql,scripts}
src/bin/psql/command.c
src/bin/psql/copy.c
src/bin/psql/describe.c
src/bin/scripts/clusterdb.c
src/bin/scripts/createdb.c
src/bin/scripts/createuser.c
src/bin/scripts/dropdb.c
src/bin/scripts/dropuser.c
src/bin/scripts/reindexdb.c
src/bin/scripts/vacuumdb.c

> 3. How many definitions of atooid() do we have now?
>

$ git grep '#define atooid' |wc -l
11

I found no obvious .h to include though.

--
Steeve Lennmark

You appear to be generating your patches with git diff --no-prefix.
Don't do that, leave the a/ and b/ in.

My review: Clearly, everyone likes this feature.

I'm tempted to think it should be mandatory to specify this option in
plain mode when tablespaces are present. Otherwise, creating a base
backup is liable to create random files all over the place. Obviously,
there would be backward compatibility concerns.

I'm not totally happy with the choice of ":" as the mapping separator,
because that would always require escaping on Windows. We could make it
analogous to the path handling and make ";" the separator on Windows.
Then again, this is not a path, so maybe it should look like one. We
pick something else altogether, like "=".

The option name "--tablespace" isn't very clear. It ought to be
something like "--tablespace-mapping".

I don't think we should require the new directory to be an absolute
path. It should be relative to the current directory, just like the -D
option does it.

I'm not so worried about the atooid() and linked-list duplication. That
can be addressed at some later point.

I would try to write this patch without using MAXPGPATH. I know
existing code uses it, but we should try to use it less, because it
overallocates memory and requires handling additional error conditions.
For example, you catch overflow in tablespace_list_append() but later
report that as invalid tablespace format. We now have psprintf() to
make coding with dynamic memory allocation easier.

It looks like when you ignore an escaped ":" as a separator, you don't
actually unescape it for use as a directory.

OLDDIR and NEWDIR should be capitalized in the help message.

Somehow, I had the subconscious assumption that this feature would do
prefix matching on the directories, not only complete string matching.
So if I had tablespaces in /mnt/data1, /mnt/data2, /mnt/data3, I could
map them all with -T /mnt:mnt and be done. Not sure if that is useful
to many, but it's worth a thought.

Review style guide for error messages:
http://www.postgresql.org/docs/devel/static/error-style-guide.html

We need to think about how to handle this on platforms without symlinks.
I don't like just printing an error message and moving on. It should be
either pass or fail or an option to choose between them.

Please put the options in the getopt handling in alphabetical order.
It's not very alphabetical now, but between D and F is probably not the
best place. ;-)

Peter,
On Thu, Jan 23, 2014 at 2:06 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>
> I'm tempted to think it should be mandatory to specify this option in
> plain mode when tablespaces are present. Otherwise, creating a base
> backup is liable to create random files all over the place. Obviously,
> there would be backward compatibility concerns.
>

That was my initial thought too, the one thing that speaks FOR a change
in behaviour is that there isn't a lot of people who have moved over to
pg_basebackup yet and even fewer who use multiple tablespaces. For me
at least pg_basebackup isn't an option at the moment since I use more
than one tablespace.

I'm not totally happy with the choice of ":" as the mapping separator,
> because that would always require escaping on Windows. We could make it
> analogous to the path handling and make ";" the separator on Windows.
> Then again, this is not a path, so maybe it should look like one. We
> pick something else altogether, like "=".
>
> The option name "--tablespace" isn't very clear. It ought to be
> something like "--tablespace-mapping".
>

This design choice I made about -T (--tablespace) and colon as
separator was copied from pg_barman, which is the way I back up my
clusters at the moment. Renaming the option to --tablespace-mapping and
changing the syntax to something like "=" is totally fine by me.

> I don't think we should require the new directory to be an absolute
> path. It should be relative to the current directory, just like the -D
> option does it.
>

Accepting a relative path should be fine, I made a failed attempt using
realpath(3) initially but I guess checking for [0] != '/' and
prepending getcwd(3) would suffice.

I would try to write this patch without using MAXPGPATH. I know
> existing code uses it, but we should try to use it less, because it
> overallocates memory and requires handling additional error conditions.
> For example, you catch overflow in tablespace_list_append() but later
> report that as invalid tablespace format. We now have psprintf() to
> make coding with dynamic memory allocation easier.
>

Is overallocating memory in a cli application really an issue though? I
will obviously rewrite the code to fix that if necessary.

It looks like when you ignore an escaped ":" as a separator, you don't
> actually unescape it for use as a directory.
>

+ if (*arg == '\\' && *(arg + 1) == ':')
+ ;

This code handles that case, I could try to make that cleaner.

> Somehow, I had the subconscious assumption that this feature would do
> prefix matching on the directories, not only complete string matching.
> So if I had tablespaces in /mnt/data1, /mnt/data2, /mnt/data3, I could
> map them all with -T /mnt:mnt and be done. Not sure if that is useful
> to many, but it's worth a thought.
>

I like that a lot, but I'm afraid the code would have to get a bit more
complex to add that functionality. It would be an easier rewrite if we
added a hint character, something like -T '/mnt/*:mnt'.

> Review style guide for error messages:
> http://www.postgresql.org/docs/devel/static/error-style-guide.html

I will do that.

We need to think about how to handle this on platforms without symlinks.
> I don't like just printing an error message and moving on. It should be
> either pass or fail or an option to choose between them.
>

I hope someone with experience with those kind of systems can come up
with suggestions on how to solve that. I only run postgres on Linux.

>
> Please put the options in the getopt handling in alphabetical order.
> It's not very alphabetical now, but between D and F is probably not the
> best place. ;-)
>

Done.

//Steeve

New patch attached with the following changes:

On Thu, Jan 23, 2014 at 11:01 AM, Steeve Lennmark <steevel(at)handeldsbanken(dot)se
> wrote:

> On Thu, Jan 23, 2014 at 2:06 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>
>> I'm not totally happy with the choice of ":" as the mapping separator,
>> because that would always require escaping on Windows. We could make it
>> analogous to the path handling and make ";" the separator on Windows.
>> Then again, this is not a path, so maybe it should look like one. We
>> pick something else altogether, like "=".
>>
>> The option name "--tablespace" isn't very clear. It ought to be
>> something like "--tablespace-mapping".
>>
>
> This design choice I made about -T (--tablespace) and colon as
> separator was copied from pg_barman, which is the way I back up my
> clusters at the moment. Renaming the option to --tablespace-mapping and
> changing the syntax to something like "=" is totally fine by me.
>

I changed the directory separator from ":" to "=" but made it
configurable in the code.

> I don't think we should require the new directory to be an absolute
>> path. It should be relative to the current directory, just like the -D
>> option does it.
>>
>
> Accepting a relative path should be fine, I made a failed attempt using
> realpath(3) initially but I guess checking for [0] != '/' and
> prepending getcwd(3) would suffice.
>

Relative paths are now accepted. This code will probably not work on
windows though. I tried setting up Windows 8 with the git version of
postgres but was unsuccessful, so I can't really test any of this.
Help on this subject (Windows) would be much appreciated.

> Somehow, I had the subconscious assumption that this feature would do
>> prefix matching on the directories, not only complete string matching.
>> So if I had tablespaces in /mnt/data1, /mnt/data2, /mnt/data3, I could
>> map them all with -T /mnt:mnt and be done. Not sure if that is useful
>> to many, but it's worth a thought.
>>
>
> I like that a lot, but I'm afraid the code would have to get a bit more
> complex to add that functionality. It would be an easier rewrite if we
> added a hint character, something like -T '/mnt/*:mnt'.
>

This is not implemented as suggested by Peter in his previous comment.
-T /mnt:mnt now relocates all tablespaces under /mnt to the relative
path mnt.

Review style guide for error messages:
>> http://www.postgresql.org/docs/devel/static/error-style-guide.html
>
>
I've updated error messages according to the style guide.

We need to think about how to handle this on platforms without symlinks.
>> I don't like just printing an error message and moving on. It should be
>> either pass or fail or an option to choose between them.
>>
>
> I hope someone with experience with those kind of systems can come up
> with suggestions on how to solve that. I only run postgres on Linux.
>

I would still love some input on this.

Please put the options in the getopt handling in alphabetical order.
>> It's not very alphabetical now, but between D and F is probably not the
>> best place. ;-)
>>
>
> Done.
>

//Steeve

On 1/29/14, 12:07 PM, Steeve Lennmark wrote:
> We need to think about how to handle this on platforms without
> symlinks.
> I don't like just printing an error message and moving on. It
> should be
> either pass or fail or an option to choose between them.
>
> I hope someone with experience with those kind of systems can come up
> with suggestions on how to solve that. I only run postgres on Linux.
>
> I would still love some input on this.

Currently, pg_basebackup aborts if it has to create a symlink on a
platform that doesn't support it. So your code that updates the
symlinks would never come into play anyway. I'd just update that code
with a "shouldn't get here" comment and add an exit(1).

I've been working on your patch. Attached is a version I'd be happy to
commit. Please check that it's okay with you.

I rewrote the option argument parsing logic a little bit to be more
clear and provide more specific error messages.

I reinstated the requirement that both old and new directory are
absolute. After consideration, I think this makes sense because all
tablespace directories are always required to be absolute in other
contexts. (Note: Checking for absolute path by testing the first
character for '/' is not portable.)

I also removed the partial matching. This would have let -T /data1=...
also match /data11, which is clearly confusing. This logic would need
some intelligence about slashes, similar to fnmatch(). This could
perhaps be added later.

Finally, I wrote some test cases for this new functionality. See the
attached patch, which can be applied on top of
<https://commitfest.postgresql.org/action/patch_view?id=1394>.

On 2/15/14, 7:05 PM, Peter Eisentraut wrote:
> I've been working on your patch. Attached is a version I'd be happy to
> commit. Please check that it's okay with you.

Committed after some rebasing.