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
