[PATCH] Relocation of tablespaces in pg_basebackup

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@proxel.se> wrote:

On Thu, Jan 9, 2014 at 6:58 PM, Steeve Lennmark
<steevel@handeldsbanken.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@hagander.net> wrote:

On Thu, Jan 9, 2014 at 6:58 PM, Steeve Lennmark <steevel@handeldsbanken.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.bartolini@2ndQuadrant.it | www.2ndQuadrant.it

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jan 9, 2014 at 10:29 PM, Gabriele Bartolini <
gabriele.bartolini@2ndquadrant.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.bartolini@2ndQuadrant.it | www.2ndQuadrant.it

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jan 10, 2014 at 12:25 PM, Gabriele Bartolini <
gabriele.bartolini@2ndquadrant.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.bartolini@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jan 13, 2014 at 4:29 AM, Andreas Karlsson <andreas@proxel.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@handeldsbanken.se>wrote:

On Mon, Jan 13, 2014 at 4:29 AM, Andreas Karlsson <andreas@proxel.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro,
On Thu, Jan 16, 2014 at 3:20 PM, Alvaro Herrera <alvherre@2ndquadrant.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@2ndquadrant.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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers