pg_basebackup, tablespace mapping and path canonicalization

Started by Ian Barwickalmost 11 years ago4 messages
#1Ian Barwick
ian@2ndquadrant.com
1 attachment(s)

Hi

I stumbled on what appears to be inconsistent handling of double slashes
in tablespace paths when using pg_basebackup with the -T/--tablespace-mapping
option:

ibarwick:postgresql (master)$ mkdir /tmp//foo-old
ibarwick:postgresql (master)$ $PG_HEAD/psql 'dbname=postgres port=9595'
psql (9.5devel)
Type "help" for help.

postgres=# CREATE TABLESPACE foo LOCATION '/tmp//foo-old';
CREATE TABLESPACE
postgres=# \db
List of tablespaces
Name | Owner | Location
------------+----------+--------------
foo | ibarwick | /tmp/foo-old
pg_default | ibarwick |
pg_global | ibarwick |
(3 rows)

So far so good. However attempting to take a base backup (on the same
machine) and remap the tablespace directory:

ibarwick:postgresql (master)$ $PG_HEAD/pg_basebackup -p9595 --pgdata=/tmp//backup --tablespace-mapping=/tmp//foo-old=/tmp//foo-new

produces the following message:

pg_basebackup: directory "/tmp/foo-old" exists but is not empty

which, while undeniably true, is unexpected and could potentially encourage someone
to hastily delete "/tmp/foo-old" after confusing it with the new directory.

The double-slash in the old tablespace path is the culprit:

ibarwick:postgresql (master)$ $PG_HEAD/pg_basebackup -p9595 --pgdata=/tmp//backup --tablespace-mapping=/tmp/foo-old=/tmp//foo-new
NOTICE: pg_stop_backup complete, all required WAL segments have been archived

The documentation does state:

To be effective, olddir must exactly match the path specification of the
tablespace as it is currently defined.

which I understood to mean that e.g. tildes would not be expanded, but it's
somewhat surprising that the path is not canonicalized in the same way
it is pretty much everywhere else (including in "CREATE TABLESPACE").

The attached patch adds the missing canonicalization; I can't see any
reason not to do this. Thoughts?

Regards

Ian Barwick

--
Ian Barwick http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, RemoteDBA, Training & Services

Attachments:

pg_basebackup-canonicalize.patchtext/plain; charset=UTF-8; name=pg_basebackup-canonicalize.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
new file mode 100644
index fbf7106..349bd90
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
*************** tablespace_list_append(const char *arg)
*** 199,204 ****
--- 199,207 ----
  		exit(1);
  	}
  
+ 	canonicalize_path(cell->old_dir);
+ 	canonicalize_path(cell->new_dir);
+ 
  	if (tablespace_dirs.tail)
  		tablespace_dirs.tail->next = cell;
  	else
#2Robert Haas
robertmhaas@gmail.com
In reply to: Ian Barwick (#1)
Re: pg_basebackup, tablespace mapping and path canonicalization

On Thu, Feb 5, 2015 at 10:21 PM, Ian Barwick <ian@2ndquadrant.com> wrote:

I stumbled on what appears to be inconsistent handling of double slashes
in tablespace paths when using pg_basebackup with the -T/--tablespace-mapping
option:

ibarwick:postgresql (master)$ mkdir /tmp//foo-old

[...]

The attached patch adds the missing canonicalization; I can't see any
reason not to do this. Thoughts?

Seems OK to me. Anyone think differently?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

#3Bruce Momjian
bruce@momjian.us
In reply to: Robert Haas (#2)
Re: pg_basebackup, tablespace mapping and path canonicalization

On Fri, Feb 6, 2015 at 08:25:42AM -0500, Robert Haas wrote:

On Thu, Feb 5, 2015 at 10:21 PM, Ian Barwick <ian@2ndquadrant.com> wrote:

I stumbled on what appears to be inconsistent handling of double slashes
in tablespace paths when using pg_basebackup with the -T/--tablespace-mapping
option:

ibarwick:postgresql (master)$ mkdir /tmp//foo-old

[...]

The attached patch adds the missing canonicalization; I can't see any
reason not to do this. Thoughts?

Seems OK to me. Anyone think differently?

Patch applied.

--
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

#4Ian Barwick
ian@2ndquadrant.com
In reply to: Bruce Momjian (#3)
Re: pg_basebackup, tablespace mapping and path canonicalization

On 29/04/15 09:12, Bruce Momjian wrote:

On Fri, Feb 6, 2015 at 08:25:42AM -0500, Robert Haas wrote:

On Thu, Feb 5, 2015 at 10:21 PM, Ian Barwick <ian@2ndquadrant.com> wrote:

I stumbled on what appears to be inconsistent handling of double slashes
in tablespace paths when using pg_basebackup with the -T/--tablespace-mapping
option:

ibarwick:postgresql (master)$ mkdir /tmp//foo-old

[...]

The attached patch adds the missing canonicalization; I can't see any
reason not to do this. Thoughts?

Seems OK to me. Anyone think differently?

Patch applied.

Thanks!

Regards

Ian Barwick

--
Ian Barwick 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