Peter, On Thu, Jan 23, 2014 at 2:06 AM, Peter Eisentraut <pete...@gmx.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
