Re: BUG #14999: pg_rewind corrupts control file global/pg_control

On Mon, Mar 5, 2018 at 4:54 PM, Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Mar 02, 2018 at 04:35:07PM +0100, Dmitry Dolgov wrote:
>> It maybe a stupid question, but why do you need to reset errno here? Is it to
>> avoid its value being carried from previous calls of `open_target_file`? In
>> this case if you put the code with `errno == EACCESS` under the if condition
>> `if (dstfd < 0)`, then as far as I remember you should always get relevant
>> errno. At the same time maybe it's valid to reset `errno` before `open`, like
>> with `getpriority`:
>
> [ ... reads and feels stupid ...]
>
> Of course all the checks should be where dstno is negative...
>
> I have done a second pass on the patch, and attached is a new version.
> This fixes this handling of errno and addresses some typos.  I have also
> fixed the test case where one of the read-only files was not properly
> switched to do so.  I have also added a commit log message.

@@ -24,7 +24,10 @@
 typedef enum
 {
        FILE_ACTION_CREATE,                     /* create local
directory or symbolic link */
-       FILE_ACTION_COPY,                       /* copy whole file,
overwriting if exists */
+       FILE_ACTION_COPY_CONFIG,        /* copy whole configuration
file, overwriting
+                                                                * if exists */
+       FILE_ACTION_COPY_DATA,          /* copy whole relation file,
overwriting if
+                                                                * exists */
        FILE_ACTION_COPY_TAIL,          /* copy tail from 'oldsize' to
'newsize' */
        FILE_ACTION_NONE,                       /* no action (we might
still copy modified
                                                                 *
blocks based on the parsed WAL) */

Since files other than relation files such as vm, fsm, WAL are
categorize as FILE_ACTION_COPY_CONFIG, I think the name would cause
misreading. For example, we can use FILE_ACTION_COPY_DATA and
FILE_ACTION_COPY_OTHER?

As you mentioned before, with this patch we end up ignoring file-open
error of database files other than relation files. I guess it's a bit
risky. We can enter them to COPY_DATA but I'd defer it to committer.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center