Re: pg_rewind failure by file deletion in source server

On Thu, Jun 18, 2015 at 10:55 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 06/16/2015 02:04 AM, Michael Paquier wrote:
>>
>> In order to reduce the risk of failure to a minimum and to preserve
>> the performance of the tool when using --source-server, I think that
>> we should add some check using pg_stat_file to see if a file is still
>> present or not, and if it is missing we can safely skip it thanks to
>> minRecoveryPoint. Now the problem is that pg_stat_file fails
>> automatically if the file targeted is missing. Hence, to avoid a bunch
>> of round trips with the server with one call to pg_stat_dir per file,
>> I think that we should add some if_not_exists option to pg_stat_file,
>> defaulting to false, to skip the error related to the file missing and
>> have it return NULL in this case. Then we could use this filter on the
>> file path in libpq_executeFileMap() to fetch only the file chunks that
>> actually exist on the server.
>
>
> You'll also need to add the option to pg_read_binary_file, though, because
> even if you do a test with pg_stat_file() just before reading the file,
> there's a race condition: someone might still delete file between
> pg_stat_file() and pg_read_file().

I propose to return NULL values if the file does not exist and
if_not_exists = true for both of them. Does that sound fine?

> Listing the directories with pg_ls_dir() has the same problem.

(After some discussion on IM with Heikki on this one).
This is actually more tricky because pg_ls_dir() does not return '.'
or '..' that we could use to identify that the directory actually
exists or not when it is empty. Hence I think that we should add two
options to pg_ls_dir:
- include_self, default to false. If set to true, '.' is added in the
list of items.
- if_not_exists, to bypass error that a folder does not exist, default
at false. If if_not_exists = true and include_self = true, returning
only '.' would mean that the folder exist but that it is empty. If
if_not_exists = true and include_self = false, no rows are returned.
We could as well ERROR as well if both options are set like that. I am
fine with any of them as long as behavior is properly documented.

> As does pg_tablespace_location().

NULL if tablespace path does not exist anymore. Is that fine.

>> Note that we could as well use some plpgsql-ing to do the same, but
>> the extension of pg_stat_file looks more useful to me. Thoughts?
>
>
> Hmm. You'll need to add the option to all of those functions. Maybe it's
> nevertheless the simplest approach.

With plpgsql you could use a try/catch/raising block to do the work.
But it still looks better to me to have alternative options with the
in-core functions.

I am fine to spend time on all those things and provide test cases,
let's just get a precise picture of what we want first.
Regards,
--
Michael