Thread: Incorrect fsync handling in pg_basebackup's tar_finish
Hi all,
I was just looking at the code of pg_basebackup, and noticed that we
don't actually check if the two last empty blocks of any tar file
produced are correctly fsync'd or not:
@@ -957,7 +957,10 @@ tar_finish(void)
/* sync the empty blocks as well, since they're after the last file */
if (tar_data->sync)
- fsync(tar_data->fd);
+ {
+ if (fsync(tar_data->fd) != 0)
+ return false;
+ }
That looks incorrect to me, hence shouldn't something like the attached
be done? Magnus and others, any opinions?
Thanks,
--
Michael
Attachment
On Mon, Jun 25, 2018 at 4:43 AM, Michael Paquier <michael@paquier.xyz> wrote:
-- Hi all,
I was just looking at the code of pg_basebackup, and noticed that we
don't actually check if the two last empty blocks of any tar file
produced are correctly fsync'd or not:
@@ -957,7 +957,10 @@ tar_finish(void)
/* sync the empty blocks as well, since they're after the last file */
if (tar_data->sync)
- fsync(tar_data->fd);
+ {
+ if (fsync(tar_data->fd) != 0)
+ return false;
+ }
That looks incorrect to me, hence shouldn't something like the attached
be done? Magnus and others, any opinions?
Yup, that seems like an issue and a correct fix to me.
On Mon, Jun 25, 2018 at 2:27 PM, Magnus Hagander <magnus@hagander.net> wrote:
>
>
> On Mon, Jun 25, 2018 at 4:43 AM, Michael Paquier <michael@paquier.xyz>
> wrote:
>>
>> Hi all,
>>
>> I was just looking at the code of pg_basebackup, and noticed that we
>> don't actually check if the two last empty blocks of any tar file
>> produced are correctly fsync'd or not:
>> @@ -957,7 +957,10 @@ tar_finish(void)
>>
>> /* sync the empty blocks as well, since they're after the last file */
>> if (tar_data->sync)
>> - fsync(tar_data->fd);
>> + {
>> + if (fsync(tar_data->fd) != 0)
>> + return false;
>> + }
>>
>> That looks incorrect to me, hence shouldn't something like the attached
>> be done? Magnus and others, any opinions?
In the same note, in tar_close(), we fsync on close. We're not
checking the status of fsync there. Should we introduce the same check
there as well?
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
On Mon, Jun 25, 2018 at 05:48:54PM +0530, Kuntal Ghosh wrote: > In the same note, in tar_close(), we fsync on close. We're not > checking the status of fsync there. Should we introduce the same check > there as well? Yes, there is a second one. I just looked at walmethods.c and I did not spot any other issues. What do you think about the updated version attached? -- Michael
Attachment
On Mon, Jun 25, 2018 at 6:47 PM, Michael Paquier <michael@paquier.xyz> wrote: > Yes, there is a second one. I just looked at walmethods.c and I did not > spot any other issues. What do you think about the updated version > attached? > -- I've also verified the same. The patch looks good to me. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com
On Mon, Jun 25, 2018 at 07:21:27PM +0530, Kuntal Ghosh wrote: > I've also verified the same. The patch looks good to me. Thanks for confirming. I have pushed the fix down to 10. -- Michael