Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file

Поиск
Список
Период
Сортировка
От Amit Kapila
Тема Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
Дата
Msg-id CAA4eK1LQKmUo7Ng0oC--iFMm38x1=+_rzYw-o=UGDYNm8BC4Dw@mail.gmail.com
обсуждение исходный текст
Ответ на Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file  (Andrew Dunstan <andrew@dunslane.net>)
Ответы Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
Список pgsql-hackers
Дерево обсуждения
On Tue, Jun 2, 2015 at 10:26 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 05/15/2015 02:21 AM, Amit Kapila wrote:

Find the patch which gets rid of rmtree usage.  I have made it as
a separate function because the same code is used from
create_tablespace_directories() as well.  I thought of extending the
same API for using it from destroy_tablespace_directories() as well,
but due to special handling (especially for ENOENT) in that function,
I left it as of now.






Well, it seems to me the new function is being altogether way too trusting about the nature of what it's being asked to remove. In the first place, the S_ISDIR/rmdir branch should only be for Windows, and secondly in the other branch we should be checking that S_ISLNK is true. It would actually be nice if we could test for a junction point on Windows, but that seems to be a bit difficult.

I think during recovery for tablespace related operations, it is
quite possible to have a directory instead of symlink in some
special cases (see function TablespaceCreateDbspace() and comments
in destroy_tablespace_directories() { ..Try to remove the symlink..}).
Also this new function is being called from create_tablespace_directories()
which uses the code as written in new function, so it doesn't make much
sense to change it Windows and non-Windows specific code.

 
The function should probably return a boolean to indicate any error, rather than void, so that the caller can take action accordingly.

I think returning boolean is good if the function has Windows
and non-Windows specific code, else we might need short, int16
or something like that as there will be three return values
(0 - success, 1 - fail to remove directory, 2 - fail to remove
symlink).

On the whole this function can be mainly used for tablespace
related paths during recovery, it isn't generic enough to be
use from all paths.  I think as proposed name of the new
function (rmsymlink) looks quite generic, so either we can
change it to (rm_tablespace_symlinks) or may be I can just
duplicate the code in read_tablespace_map() related check and
then we don't need this new function.

 
In the present case we should probably be stopping recovery dead in its tracks, but I certainly don't think we should just be ignoring it.


Do you think we should do anything before returning error?
This operation haapens in the beginning of recovery before
replay of any records, so throwing ERROR from here shouldn't
have any impact unless we have done any operation which
can create problem when user next time starts the recovery.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Alvaro Herrera
Дата:
Сообщение: Re: Re: [GENERAL] 9.4.1 -> 9.4.2 problem: could not access status of transaction 1
Следующее
От: Amit Kapila
Дата:
Сообщение: Re: checkpointer continuous flushing