Обсуждение: [HACKERS] pg_basebackup fails on Windows when using tablespace mapping
Summary:
Trying to take a `pg_basebackup -T OLDDIR=NEWDIR [etc]` on master (server running the cluster) fails on Windows with error "pg_basebackup: directory "OLDDIR" exists but is not empty".
Version: 9.6.2, installed from Standard EDB package (built with MSVC).
Repro steps:
1. Have a cluster running on Windows (you'll need max_wal_senders at least 2 and wal_level replica for pg_basebackup -X stream)
2. create tablespace testspc location 'somedisk:\some\location'; -- Slash direction is irrelevant
3. run `pg_basebackup -T somedisk:\some\location=somedisk:\new\location -X stream -D somedisk:\some\other_location -h 127.0.0.1 -U postgres`
The error should read:
pg_basebackup: directory "somedisk:\some\location" exists but is not empty
---
This was discussed today in IRC. As a temporary solution it was suggested to add 'canonicalize_path(buf);' before return in /src/port/dirmod.c:336
** DISCLAMER: note that value of r is not adjusted, so this is not a production ready fix in any way. **
This fixed the issue, but as a side effect, pg_tablespace_location displays path in canonicalized format which might look weird to most Windows users.
There was a suggestion to fix this in client instead, but this fix was the simplest one to implement.
This is my first post to Hackers, so please let me know if I missed something or can provide any additional info to help further investigate this issue.
Kind regards,
Nick.
On Tue, Jun 27, 2017 at 1:57 AM, nb <nbedxp@gmail.com> wrote: > Trying to take a `pg_basebackup -T OLDDIR=NEWDIR [etc]` on master (server > running the cluster) fails on Windows with error "pg_basebackup: directory > "OLDDIR" exists but is not empty". Yes, I can see this error easily. > This fixed the issue, but as a side effect, pg_tablespace_location displays > path in canonicalized format which might look weird to most Windows users. > > There was a suggestion to fix this in client instead, but this fix was the > simplest one to implement. At quick glance, I think that this should definitely be a client-only fix. One reason is that pg_basebackup should be able to work with past servers. A second is that this impacts as well the backend code, and readlink may not return an absolute path. At least that's true for posix's version, Postgres uses its own wrapper with junction points.. > This is my first post to Hackers, so please let me know if I missed > something or can provide any additional info to help further investigate > this issue. Thanks for the report! You are giving enough details to have a look at the problem. No need for -X stream in the command. I'll see if I can produce a patch soon. -- Michael
On Tue, Jun 27, 2017 at 12:13 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > At quick glance, I think that this should definitely be a client-only > fix. One reason is that pg_basebackup should be able to work with past > servers. A second is that this impacts as well the backend code, and > readlink may not return an absolute path. At least that's true for > posix's version, Postgres uses its own wrapper with junction points.. The problem is in pg_basebackup.c's get_tablespace_mapping(), which fails to provide a correct comparison for the directory given by caller. In your case the caller of get_tablespace_mapping() uses backslashes as directory value (path value received from backend), and the tablespace mapping uses slashes as canonicalize_path has been called once on it. Because of this inconsistency, the directory of the original tablespace is used, causing the backup to fail as it should. A simple fix is to make sure that the comparison between both things is consistent by using canonicalize_path on the directory value given by caller. Attached is a patch. I am parking that in the next CF so as this does not get forgotten as a bug fix. Perhaps a committer will show up before. Or not. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
Hi,
On Tue, Jun 27, 2017 at 10:13 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Jun 27, 2017 at 12:13 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> At quick glance, I think that this should definitely be a client-only
>> fix. One reason is that pg_basebackup should be able to work with past
>> servers. A second is that this impacts as well the backend code, and
>> readlink may not return an absolute path. At least that's true for
>> posix's version, Postgres uses its own wrapper with junction points..
>
> The problem is in pg_basebackup.c's get_tablespace_mapping(), which
> fails to provide a correct comparison for the directory given by
> caller. In your case the caller of get_tablespace_mapping() uses
> backslashes as directory value (path value received from backend), and
> the tablespace mapping uses slashes as canonicalize_path has been
> called once on it. Because of this inconsistency, the directory of the
> original tablespace is used, causing the backup to fail as it should.
> A simple fix is to make sure that the comparison between both things
> is consistent by using canonicalize_path on the directory value given
> by caller.
>
> Attached is a patch. I am parking that in the next CF so as this does
> not get forgotten as a bug fix. Perhaps a committer will show up
> before. Or not.
I am still seeing the issue with the attached patch. I had a quick
look into the patch. It seems to me like you have canonicalized the
tablespace path to convert win32 slashes to unix type of slashes but
that is not being passed to strcmp() function and probably that could
be the reason why the issue is still existing. Thanks.
for (cell = tablespace_dirs.head; cell; cell = cell->next)
- if (strcmp(dir, cell->old_dir) == 0)
+ if (strcmp(canon_dir, cell->old_dir) == 0)
--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
On Tue, Jun 27, 2017 at 7:46 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > I am still seeing the issue with the attached patch. I had a quick > look into the patch. It seems to me like you have canonicalized the > tablespace path to convert win32 slashes to unix type of slashes but > that is not being passed to strcmp() function and probably that could > be the reason why the issue is still existing. Thanks. > > for (cell = tablespace_dirs.head; cell; cell = cell->next) > - if (strcmp(dir, cell->old_dir) == 0) > + if (strcmp(canon_dir, cell->old_dir) == 0) Thanks. I had the correct version on my Windows box actually, just messed up the attachment. -- 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 Tue, Jun 27, 2017 at 6:36 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Jun 27, 2017 at 7:46 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: >> I am still seeing the issue with the attached patch. I had a quick >> look into the patch. It seems to me like you have canonicalized the >> tablespace path to convert win32 slashes to unix type of slashes but >> that is not being passed to strcmp() function and probably that could >> be the reason why the issue is still existing. Thanks. >> >> for (cell = tablespace_dirs.head; cell; cell = cell->next) >> - if (strcmp(dir, cell->old_dir) == 0) >> + if (strcmp(canon_dir, cell->old_dir) == 0) > > Thanks. I had the correct version on my Windows box actually, just > messed up the attachment. > -- Okay. I have once again reviewed your patch and tested it on Windows as well as Linux. The patch LGTM. I am now marking it as Ready For Committer. Thanks. -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Sep 10, 2017 at 12:28 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > On Tue, Jun 27, 2017 at 6:36 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Tue, Jun 27, 2017 at 7:46 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: >>> I am still seeing the issue with the attached patch. I had a quick >>> look into the patch. It seems to me like you have canonicalized the >>> tablespace path to convert win32 slashes to unix type of slashes but >>> that is not being passed to strcmp() function and probably that could >>> be the reason why the issue is still existing. Thanks. >>> >>> for (cell = tablespace_dirs.head; cell; cell = cell->next) >>> - if (strcmp(dir, cell->old_dir) == 0) >>> + if (strcmp(canon_dir, cell->old_dir) == 0) >> >> Thanks. I had the correct version on my Windows box actually, just >> messed up the attachment. >> -- > > Okay. I have once again reviewed your patch and tested it on Windows > as well as Linux. The patch LGTM. I am now marking it as Ready For > Committer. Thanks. Thanks for the review, Ashutosh. -- 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 9/10/17 00:39, Michael Paquier wrote: >> Okay. I have once again reviewed your patch and tested it on Windows >> as well as Linux. The patch LGTM. I am now marking it as Ready For >> Committer. Thanks. > > Thanks for the review, Ashutosh. Committed to master. I suppose this should be backpatched? (I changed the strcpy() to strlcpy() for better karma.) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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 Wed, Nov 1, 2017 at 2:24 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Committed to master. I suppose this should be backpatched? Thanks! Yes this should be back-patched. -- 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 11/1/17 10:29, Michael Paquier wrote: > On Wed, Nov 1, 2017 at 2:24 PM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> Committed to master. I suppose this should be backpatched? > > Thanks! Yes this should be back-patched. OK, done for 10, 9.6, and 9.5. The tablespace mapping feature started in 9.4 (has it been that long already?), but the canonicalization was only added in 9.5. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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, Nov 2, 2017 at 2:05 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 11/1/17 10:29, Michael Paquier wrote: >> On Wed, Nov 1, 2017 at 2:24 PM, Peter Eisentraut >> <peter.eisentraut@2ndquadrant.com> wrote: >>> Committed to master. I suppose this should be backpatched? >> >> Thanks! Yes this should be back-patched. > > OK, done for 10, 9.6, and 9.5. > > The tablespace mapping feature started in 9.4 (has it been that long > already?), but the canonicalization was only added in 9.5. Thank you. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers