Re: [HACKERS] Fixing pg_basebackup with tablespaces found in $PGDATA
On Thu, Jan 2, 2014 at 03:34:04PM +0100, Bernd Helmle wrote: > > > --On 1. Januar 2014 23:53:46 +0100 Dimitri Fontaine > 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 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
Re: [HACKERS] Fixing pg_basebackup with tablespaces found in $PGDATA
Magnus Hagander 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
Re: [HACKERS] Fixing pg_basebackup with tablespaces found in $PGDATA
On Thu, Jan 2, 2014 at 2:06 PM, Dimitri Fontaine wrote: > Magnus Hagander 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. > > 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/
Re: [HACKERS] Fixing pg_basebackup with tablespaces found in $PGDATA
On Thu, Jan 2, 2014 at 11:34 PM, Bernd Helmle wrote: > > > --On 1. Januar 2014 23:53:46 +0100 Dimitri Fontaine > 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
Re: [HACKERS] Fixing pg_basebackup with tablespaces found in $PGDATA
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
Re: [HACKERS] Fixing pg_basebackup with tablespaces found in $PGDATA
--On 1. Januar 2014 23:53:46 +0100 Dimitri Fontaine 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
Re: [HACKERS] Fixing pg_basebackup with tablespaces found in $PGDATA
Magnus Hagander 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
*** 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;
}
---
Re: [HACKERS] Fixing pg_basebackup with tablespaces found in $PGDATA
On Wed, Jan 1, 2014 at 11:53 PM, Dimitri Fontaine 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/
*** 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_VERSI
[HACKERS] 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
*** 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 tablepac
