Обсуждение: [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

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 ItaliaPostgreSQL Training, Services and Supportgabriele.bartolini@2ndQuadrant.it |
www.2ndQuadrant.it

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 ItaliaPostgreSQL Training, Services and Supportgabriele.bartolini@2ndQuadrant.it |
www.2ndQuadrant.it

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

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

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

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

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.