Fixing pg_basebackup with tablespaces found in $PGDATA
Hi,
As much as I've seen people frown upon $subject, it still happens in the
wild, and Magnus seems to agree that the current failure mode of our
pg_basebackup tool when confronted to the situation is a bug.
So here's a fix, attached.
To reproduce, mkdir -p $PGDATA/tbs/foo then CREATE TABLESPACE there, and
then pg_basebackup your server. If doing so from the same server, as I
did, then pick the tar format, as here:
pg_basebackup -Ft -z -c fast -v -X fetch -D /tmp/backup
Then use tar to see that the base backup contains the whole content of
your foo tablespace, and if you did create another tablespace within
$PGDATA/pg_tblspc (which is the other common way to trigger that issue)
then add it to what you want to see:
tar tzvf /tmp/backup/base.tar.gz pg_tblspc tbs/foo pg_tblspc/bar
Note that empty directories are expected, so tar should output their
entries. Those directories are where you need to be restoring the
tablespace tarballs.
When using pg_basebackup in plain mode, the error is that you get a copy
of all your tablespaces first, then the main PGDATA is copied over and
as the destination directories already do exists (and not empty) the
whole backup fails there.
The bug should be fixed against all revisions of pg_basebackup, though I
didn't try to apply this very patch on all target branches.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachments:
basebackup.patchtext/x-patchDownload
*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
***************
*** 45,51 **** typedef struct
} basebackup_options;
! static int64 sendDir(char *path, int basepathlen, bool sizeonly);
static int64 sendTablespace(char *path, bool sizeonly);
static bool sendFile(char *readfilename, char *tarfilename,
struct stat * statbuf, bool missing_ok);
--- 45,52 ----
} basebackup_options;
! static int64 sendDir(char *path, int basepathlen, int rootpathlen,
! bool sizeonly, List *tablespaces);
static int64 sendTablespace(char *path, bool sizeonly);
static bool sendFile(char *readfilename, char *tarfilename,
struct stat * statbuf, bool missing_ok);
***************
*** 100,105 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
--- 101,114 ----
XLogRecPtr endptr;
TimeLineID endtli;
char *labelfile;
+ char cwd[MAXPGPATH];
+ int rootpathlen;
+
+ /* we need to compute rootpathlen to allow for skipping tablespaces
+ * installed within PGDATA
+ */
+ getcwd(cwd, MAXPGPATH);
+ rootpathlen = strlen(cwd) + 1;
backup_started_in_recovery = RecoveryInProgress();
***************
*** 165,171 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
/* Add a node for the base directory at the end */
ti = palloc0(sizeof(tablespaceinfo));
! ti->size = opt->progress ? sendDir(".", 1, true) : -1;
tablespaces = lappend(tablespaces, ti);
/* Send tablespace header */
--- 174,181 ----
/* Add a node for the base directory at the end */
ti = palloc0(sizeof(tablespaceinfo));
! ti->size = opt->progress ?
! sendDir(".", 1, rootpathlen, true, tablespaces) : -1;
tablespaces = lappend(tablespaces, ti);
/* Send tablespace header */
***************
*** 191,197 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
sendFileWithContent(BACKUP_LABEL_FILE, labelfile);
/* ... then the bulk of the files ... */
! sendDir(".", 1, false);
/* ... and pg_control after everything else. */
if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0)
--- 201,207 ----
sendFileWithContent(BACKUP_LABEL_FILE, labelfile);
/* ... then the bulk of the files ... */
! sendDir(".", 1, rootpathlen, false, tablespaces);
/* ... and pg_control after everything else. */
if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0)
***************
*** 779,785 **** sendTablespace(char *path, bool sizeonly)
size = 512; /* Size of the header just added */
/* Send all the files in the tablespace version directory */
! size += sendDir(pathbuf, strlen(path), sizeonly);
return size;
}
--- 789,795 ----
size = 512; /* Size of the header just added */
/* Send all the files in the tablespace version directory */
! size += sendDir(pathbuf, strlen(path), 0, sizeonly, NIL);
return size;
}
***************
*** 788,796 **** sendTablespace(char *path, bool sizeonly)
* Include all files from the given directory in the output tar stream. If
* 'sizeonly' is true, we just calculate a total length and return it, without
* actually sending anything.
*/
static int64
! sendDir(char *path, int basepathlen, bool sizeonly)
{
DIR *dir;
struct dirent *de;
--- 798,810 ----
* Include all files from the given directory in the output tar stream. If
* 'sizeonly' is true, we just calculate a total length and return it, without
* actually sending anything.
+ *
+ * Omit any directory listed in tablepaces, so as to avoid backuping
+ * tablespaces twice when users did create their tablespaces inside PGDATA.
*/
static int64
! sendDir(char *path, int basepathlen, int rootpathlen,
! bool sizeonly, List *tablespaces)
{
DIR *dir;
struct dirent *de;
***************
*** 931,936 **** sendDir(char *path, int basepathlen, bool sizeonly)
--- 945,954 ----
}
else if (S_ISDIR(statbuf.st_mode))
{
+ bool skip_this_dir = false;
+ int pathbuflen = strlen(pathbuf);
+ ListCell *lc;
+
/*
* Store a directory entry in the tar file so we can get the
* permissions right.
***************
*** 939,946 **** sendDir(char *path, int basepathlen, bool sizeonly)
_tarWriteHeader(pathbuf + basepathlen + 1, NULL, &statbuf);
size += 512; /* Size of the header just added */
! /* call ourselves recursively for a directory */
! size += sendDir(pathbuf, basepathlen, sizeonly);
}
else if (S_ISREG(statbuf.st_mode))
{
--- 957,981 ----
_tarWriteHeader(pathbuf + basepathlen + 1, NULL, &statbuf);
size += 512; /* Size of the header just added */
! /* call ourselves recursively for a directory, only when that
! * directory is not listed in the tablespaces list
! */
! foreach(lc, tablespaces)
! {
! tablespaceinfo *ti = (tablespaceinfo *) lfirst(lc);
!
! /* we need to compare the relative parts only */
! skip_this_dir = ti->path
! && pathbuflen > 2 /* ./ */
! && strlen(ti->path) > rootpathlen
! && strcmp(ti->path + rootpathlen, pathbuf + 2) == 0;
!
! if (skip_this_dir)
! break;
! }
! if (!skip_this_dir)
! size += sendDir(pathbuf, basepathlen, rootpathlen,
! sizeonly, tablespaces);
}
else if (S_ISREG(statbuf.st_mode))
{
On Wed, Jan 1, 2014 at 11:53 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr>wrote:
Hi,
As much as I've seen people frown upon $subject, it still happens in the
wild, and Magnus seems to agree that the current failure mode of our
pg_basebackup tool when confronted to the situation is a bug.So here's a fix, attached.
To reproduce, mkdir -p $PGDATA/tbs/foo then CREATE TABLESPACE there, and
then pg_basebackup your server. If doing so from the same server, as I
did, then pick the tar format, as here:pg_basebackup -Ft -z -c fast -v -X fetch -D /tmp/backup
Then use tar to see that the base backup contains the whole content of
your foo tablespace, and if you did create another tablespace within
$PGDATA/pg_tblspc (which is the other common way to trigger that issue)
then add it to what you want to see:tar tzvf /tmp/backup/base.tar.gz pg_tblspc tbs/foo pg_tblspc/bar
Note that empty directories are expected, so tar should output their
entries. Those directories are where you need to be restoring the
tablespace tarballs.When using pg_basebackup in plain mode, the error is that you get a copy
of all your tablespaces first, then the main PGDATA is copied over and
as the destination directories already do exists (and not empty) the
whole backup fails there.The bug should be fixed against all revisions of pg_basebackup, though I
didn't try to apply this very patch on all target branches.
I had a second round of thought about this, and I don't think this one is
going to work. Unfortunately, it's part of the advice I gave you yesterday
that was bad I think :)
We can't get away with just comparing the relative part of the pathname.
Because it will fail if there is another path with exactly the same length,
containing the tablespace.
I think we might want to store a value in the tablespaceinfo struct
indicating whether it's actually inside PGDATA (since we have the full path
at that point), and then skip it based on that instead. Or store and pass
the value of getcwd() perhaps.
Or am I thinking wrong now instead? :)
I've attached a slightly updated patch - I changed around a bit of logic
order and updated some comments during my review. And added error-checking.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachments:
basebackup_v2.patchtext/x-patch; charset=US-ASCII; name=basebackup_v2.patchDownload
*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
***************
*** 45,51 **** typedef struct
} basebackup_options;
! static int64 sendDir(char *path, int basepathlen, bool sizeonly);
static int64 sendTablespace(char *path, bool sizeonly);
static bool sendFile(char *readfilename, char *tarfilename,
struct stat * statbuf, bool missing_ok);
--- 45,52 ----
} basebackup_options;
! static int64 sendDir(char *path, int basepathlen, int rootpathlen,
! bool sizeonly, List *tablespaces);
static int64 sendTablespace(char *path, bool sizeonly);
static bool sendFile(char *readfilename, char *tarfilename,
struct stat * statbuf, bool missing_ok);
***************
*** 100,105 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
--- 101,119 ----
XLogRecPtr endptr;
TimeLineID endtli;
char *labelfile;
+ char cwd[MAXPGPATH];
+ int rootpathlen;
+
+ /*
+ * We need to compute rootpathlen to allow for skipping tablespaces
+ * installed within PGDATA.
+ */
+ if (!getcwd(cwd, MAXPGPATH))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not determine current directory: %m")));
+
+ rootpathlen = strlen(cwd) + 1;
backup_started_in_recovery = RecoveryInProgress();
***************
*** 165,171 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
/* Add a node for the base directory at the end */
ti = palloc0(sizeof(tablespaceinfo));
! ti->size = opt->progress ? sendDir(".", 1, true) : -1;
tablespaces = lappend(tablespaces, ti);
/* Send tablespace header */
--- 179,186 ----
/* Add a node for the base directory at the end */
ti = palloc0(sizeof(tablespaceinfo));
! ti->size = opt->progress ?
! sendDir(".", 1, rootpathlen, true, tablespaces) : -1;
tablespaces = lappend(tablespaces, ti);
/* Send tablespace header */
***************
*** 191,197 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
sendFileWithContent(BACKUP_LABEL_FILE, labelfile);
/* ... then the bulk of the files ... */
! sendDir(".", 1, false);
/* ... and pg_control after everything else. */
if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0)
--- 206,212 ----
sendFileWithContent(BACKUP_LABEL_FILE, labelfile);
/* ... then the bulk of the files ... */
! sendDir(".", 1, rootpathlen, false, tablespaces);
/* ... and pg_control after everything else. */
if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0)
***************
*** 778,785 **** sendTablespace(char *path, bool sizeonly)
_tarWriteHeader(TABLESPACE_VERSION_DIRECTORY, NULL, &statbuf);
size = 512; /* Size of the header just added */
! /* Send all the files in the tablespace version directory */
! size += sendDir(pathbuf, strlen(path), sizeonly);
return size;
}
--- 793,805 ----
_tarWriteHeader(TABLESPACE_VERSION_DIRECTORY, NULL, &statbuf);
size = 512; /* Size of the header just added */
! /*
! * Send all the files in the tablespace version directory.
! * Since we don't protect against nested tablespaces anywhere
! * other than the root directory, no need to pass rootpathlen
! * and tablespaces list.
! */
! size += sendDir(pathbuf, strlen(path), 0, sizeonly, NIL);
return size;
}
***************
*** 788,796 **** sendTablespace(char *path, bool sizeonly)
* Include all files from the given directory in the output tar stream. If
* 'sizeonly' is true, we just calculate a total length and return it, without
* actually sending anything.
*/
static int64
! sendDir(char *path, int basepathlen, bool sizeonly)
{
DIR *dir;
struct dirent *de;
--- 808,820 ----
* Include all files from the given directory in the output tar stream. If
* 'sizeonly' is true, we just calculate a total length and return it, without
* actually sending anything.
+ *
+ * Omit any directory listed in tablepaces, so as to avoid backuping
+ * tablespaces twice when users created their tablespaces inside PGDATA.
*/
static int64
! sendDir(char *path, int basepathlen, int rootpathlen,
! bool sizeonly, List *tablespaces)
{
DIR *dir;
struct dirent *de;
***************
*** 931,936 **** sendDir(char *path, int basepathlen, bool sizeonly)
--- 955,964 ----
}
else if (S_ISDIR(statbuf.st_mode))
{
+ bool skip_this_dir = false;
+ int pathbuflen = strlen(pathbuf);
+ ListCell *lc;
+
/*
* Store a directory entry in the tar file so we can get the
* permissions right.
***************
*** 939,946 **** sendDir(char *path, int basepathlen, bool sizeonly)
_tarWriteHeader(pathbuf + basepathlen + 1, NULL, &statbuf);
size += 512; /* Size of the header just added */
! /* call ourselves recursively for a directory */
! size += sendDir(pathbuf, basepathlen, sizeonly);
}
else if (S_ISREG(statbuf.st_mode))
{
--- 967,1000 ----
_tarWriteHeader(pathbuf + basepathlen + 1, NULL, &statbuf);
size += 512; /* Size of the header just added */
! /*
! * call ourselves recursively for a directory, unless that
! * directory is listed in the tablespaces list (which indicates
! * that this is a tablespace that happens to be placed inside
! * PGADATA)
! */
! foreach(lc, tablespaces)
! {
! tablespaceinfo *ti = (tablespaceinfo *) lfirst(lc);
!
! /*
! * ti->path will have the complete absolute path to the
! * tablespace target, whereas pathbuf holds a relative
! * path (from the data directory, including "./").
! * Compare the relative parts only */
! */
! if (ti->path &&
! pathbuflen > 2 && /* ./ */
! strlen(ti->path) > rootpathlen &&
! strcmp(ti->path + rootpathlen, pathbuf + 2) == 0)
! {
! skip_this_dir = true;
! break;
! }
! }
! if (!skip_this_dir)
! size += sendDir(pathbuf, basepathlen, rootpathlen,
! sizeonly, tablespaces);
}
else if (S_ISREG(statbuf.st_mode))
{
Magnus Hagander <magnus@hagander.net> writes:
We can't get away with just comparing the relative part of the pathname.
Because it will fail if there is another path with exactly the same length,
containing the tablespace.
Actually… yeah.
I think we might want to store a value in the tablespaceinfo struct
indicating whether it's actually inside PGDATA (since we have the full path
at that point), and then skip it based on that instead. Or store and pass
the value of getcwd() perhaps.
I think it's best to stuff in the tablespaceinfo struct either NIL or
the relative path of the tablespace when found in $PGDATA, as done in
the attached.
I've attached a slightly updated patch - I changed around a bit of logic
order and updated some comments during my review. And added error-checking.
Thanks! I started again from your version for v3.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachments:
basebackup_v3.patchtext/x-patchDownload
*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
***************
*** 45,51 **** typedef struct
} basebackup_options;
! static int64 sendDir(char *path, int basepathlen, bool sizeonly);
static int64 sendTablespace(char *path, bool sizeonly);
static bool sendFile(char *readfilename, char *tarfilename,
struct stat * statbuf, bool missing_ok);
--- 45,51 ----
} basebackup_options;
! static int64 sendDir(char *path, int basepathlen, bool sizeonly, List *tablespaces);
static int64 sendTablespace(char *path, bool sizeonly);
static bool sendFile(char *readfilename, char *tarfilename,
struct stat * statbuf, bool missing_ok);
***************
*** 72,77 **** typedef struct
--- 72,78 ----
{
char *oid;
char *path;
+ char *rpath; /* relative path within PGDATA, or nil. */
int64 size;
} tablespaceinfo;
***************
*** 100,105 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
--- 101,119 ----
XLogRecPtr endptr;
TimeLineID endtli;
char *labelfile;
+ char cwd[MAXPGPATH];
+ int rootpathlen;
+
+ /*
+ * We need to compute rootpathlen to allow for skipping tablespaces
+ * installed within PGDATA.
+ */
+ if (!getcwd(cwd, MAXPGPATH))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not determine current directory: %m")));
+
+ rootpathlen = strlen(cwd);
backup_started_in_recovery = RecoveryInProgress();
***************
*** 119,124 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
--- 133,139 ----
{
char fullpath[MAXPGPATH];
char linkpath[MAXPGPATH];
+ char *relpath = NULL;
int rllen;
/* Skip special stuff */
***************
*** 145,153 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
--- 160,178 ----
}
linkpath[rllen] = '\0';
+ /*
+ * Relpath is the relative path of the tablespace linkpath when
+ * the realname is found within PGDATA, or NULL.
+ */
+ if (rllen > rootpathlen
+ && strncmp(linkpath, cwd, rootpathlen) == 0
+ && linkpath[rootpathlen] == '/')
+ relpath = linkpath + rootpathlen + 1;
+
ti = palloc(sizeof(tablespaceinfo));
ti->oid = pstrdup(de->d_name);
ti->path = pstrdup(linkpath);
+ ti->rpath = relpath ? pstrdup(relpath) : NULL;
ti->size = opt->progress ? sendTablespace(fullpath, true) : -1;
tablespaces = lappend(tablespaces, ti);
#else
***************
*** 165,171 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
/* Add a node for the base directory at the end */
ti = palloc0(sizeof(tablespaceinfo));
! ti->size = opt->progress ? sendDir(".", 1, true) : -1;
tablespaces = lappend(tablespaces, ti);
/* Send tablespace header */
--- 190,196 ----
/* Add a node for the base directory at the end */
ti = palloc0(sizeof(tablespaceinfo));
! ti->size = opt->progress ? sendDir(".", 1, true, tablespaces) : -1;
tablespaces = lappend(tablespaces, ti);
/* Send tablespace header */
***************
*** 191,197 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
sendFileWithContent(BACKUP_LABEL_FILE, labelfile);
/* ... then the bulk of the files ... */
! sendDir(".", 1, false);
/* ... and pg_control after everything else. */
if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0)
--- 216,222 ----
sendFileWithContent(BACKUP_LABEL_FILE, labelfile);
/* ... then the bulk of the files ... */
! sendDir(".", 1, false, tablespaces);
/* ... and pg_control after everything else. */
if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0)
***************
*** 778,785 **** sendTablespace(char *path, bool sizeonly)
_tarWriteHeader(TABLESPACE_VERSION_DIRECTORY, NULL, &statbuf);
size = 512; /* Size of the header just added */
! /* Send all the files in the tablespace version directory */
! size += sendDir(pathbuf, strlen(path), sizeonly);
return size;
}
--- 803,815 ----
_tarWriteHeader(TABLESPACE_VERSION_DIRECTORY, NULL, &statbuf);
size = 512; /* Size of the header just added */
! /*
! * Send all the files in the tablespace version directory.
! * Since we don't protect against nested tablespaces anywhere
! * other than the root directory, no need to pass rootpathlen
! * and tablespaces list.
! */
! size += sendDir(pathbuf, strlen(path), sizeonly, NIL);
return size;
}
***************
*** 788,796 **** sendTablespace(char *path, bool sizeonly)
* Include all files from the given directory in the output tar stream. If
* 'sizeonly' is true, we just calculate a total length and return it, without
* actually sending anything.
*/
static int64
! sendDir(char *path, int basepathlen, bool sizeonly)
{
DIR *dir;
struct dirent *de;
--- 818,829 ----
* Include all files from the given directory in the output tar stream. If
* 'sizeonly' is true, we just calculate a total length and return it, without
* actually sending anything.
+ *
+ * Omit any directory listed in tablepaces, so as to avoid backuping
+ * tablespaces twice when users created their tablespaces inside PGDATA.
*/
static int64
! sendDir(char *path, int basepathlen, bool sizeonly, List *tablespaces)
{
DIR *dir;
struct dirent *de;
***************
*** 931,936 **** sendDir(char *path, int basepathlen, bool sizeonly)
--- 964,972 ----
}
else if (S_ISDIR(statbuf.st_mode))
{
+ bool skip_this_dir = false;
+ ListCell *lc;
+
/*
* Store a directory entry in the tar file so we can get the
* permissions right.
***************
*** 939,946 **** sendDir(char *path, int basepathlen, bool sizeonly)
_tarWriteHeader(pathbuf + basepathlen + 1, NULL, &statbuf);
size += 512; /* Size of the header just added */
! /* call ourselves recursively for a directory */
! size += sendDir(pathbuf, basepathlen, sizeonly);
}
else if (S_ISREG(statbuf.st_mode))
{
--- 975,1003 ----
_tarWriteHeader(pathbuf + basepathlen + 1, NULL, &statbuf);
size += 512; /* Size of the header just added */
! /*
! * call ourselves recursively for a directory, unless it happens
! * to be a separate tablespace installed within PGDATA.
! */
! foreach(lc, tablespaces)
! {
! tablespaceinfo *ti = (tablespaceinfo *) lfirst(lc);
!
! /*
! * ti->rpath is the tablespace relative path within PGDATA, or
! * NULL if the tablespace has been properly installed
! * somewhere else.
! *
! * Don't forget to omit the leading "./" in comparing:
! */
! if (ti->rpath && strcmp(ti->rpath, pathbuf + 2) == 0)
! {
! skip_this_dir = true;
! break;
! }
! }
! if (!skip_this_dir)
! size += sendDir(pathbuf, basepathlen, sizeonly, tablespaces);
}
else if (S_ISREG(statbuf.st_mode))
{
--On 1. Januar 2014 23:53:46 +0100 Dimitri Fontaine
<dimitri@2ndQuadrant.fr> wrote:
Hi,
As much as I've seen people frown upon $subject, it still happens in the
wild, and Magnus seems to agree that the current failure mode of our
pg_basebackup tool when confronted to the situation is a bug.So here's a fix, attached.
I've seen having tablespaces under PGDATA as a policy within several data
centres in the past. The main reasoning behind this seems that they
strictly separate platform and database administration and for database
inventory reasons. They are regularly surprised if you tell them to not use
tablespaces in such a way, since they absorbed this practice over the years
from other database systems. So +1 for fixing this.
--
Thanks
Bernd
--
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/02/2014 06:53 AM, Dimitri Fontaine wrote:
As much as I've seen people frown upon $subject, it still happens in the
wild
I met a new case of it a couple of weeks ago, so I can certainly confirm
that.
--
Craig Ringer 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 Thu, Jan 2, 2014 at 11:34 PM, Bernd Helmle <mailings@oopsware.de> wrote:
--On 1. Januar 2014 23:53:46 +0100 Dimitri Fontaine <dimitri@2ndQuadrant.fr>
wrote:Hi,
As much as I've seen people frown upon $subject, it still happens in the
wild, and Magnus seems to agree that the current failure mode of our
pg_basebackup tool when confronted to the situation is a bug.So here's a fix, attached.
I've seen having tablespaces under PGDATA as a policy within several data
centres in the past. The main reasoning behind this seems that they strictly
separate platform and database administration and for database inventory
reasons. They are regularly surprised if you tell them to not use
tablespaces in such a way, since they absorbed this practice over the years
from other database systems. So +1 for fixing this.
Same here. +1 for fixing it. Having tablespaces in PGDATA itself is
most of the time part of some obscure policy making easier to manage
all the data in one folder...
--
Michael
--
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 2, 2014 at 2:06 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr>wrote:
Magnus Hagander <magnus@hagander.net> writes:
We can't get away with just comparing the relative part of the pathname.
Because it will fail if there is another path with exactly the samelength,
containing the tablespace.
Actually… yeah.
I think we might want to store a value in the tablespaceinfo struct
indicating whether it's actually inside PGDATA (since we have the fullpath
at that point), and then skip it based on that instead. Or store and pass
the value of getcwd() perhaps.I think it's best to stuff in the tablespaceinfo struct either NIL or
the relative path of the tablespace when found in $PGDATA, as done in
the attached.I've attached a slightly updated patch - I changed around a bit of logic
order and updated some comments during my review. And addederror-checking.
Thanks! I started again from your version for v3.
Applied a fairly heavily edited version of this one. I also backpatched it
to 9.1 and up.
Thanks!
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
Applied a fairly heavily edited version of this one. I also backpatched it
to 9.1 and up.
Thanks a lot!
Did some reviewing and re-testing here, I like using DataDir and
IS_DIR_SEP better than what I did, of course ;-)
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
--
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 2, 2014 at 03:34:04PM +0100, Bernd Helmle wrote:
--On 1. Januar 2014 23:53:46 +0100 Dimitri Fontaine
<dimitri@2ndQuadrant.fr> wrote:Hi,
As much as I've seen people frown upon $subject, it still happens in the
wild, and Magnus seems to agree that the current failure mode of our
pg_basebackup tool when confronted to the situation is a bug.So here's a fix, attached.
I've seen having tablespaces under PGDATA as a policy within several
data centres in the past. The main reasoning behind this seems that
they strictly separate platform and database administration and for
database inventory reasons. They are regularly surprised if you tell
them to not use tablespaces in such a way, since they absorbed this
practice over the years from other database systems. So +1 for
fixing this.
FYI, this setup also causes problems for pg_upgrade. There is a recent
thread about that that I will reply to. The problem is that pre-9.2
servers get a mismatch between the symlink and the pg_tablespace path
when they rename the old cluster.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers