Horton

Thread

Commits

  1. Correct handling of fsync failures with tar mode of walmethods.c

  1. Incorrect fsync handling in pg_basebackup's tar_finish

    Michael Paquier <michael@paquier.xyz> — 2018-06-25T02:43:56Z

    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

  2. Re: Incorrect fsync handling in pg_basebackup's tar_finish

    Magnus Hagander <magnus@hagander.net> — 2018-06-25T08:57:29Z

    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.
-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  3. Re: Incorrect fsync handling in pg_basebackup's tar_finish

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2018-06-25T12:18:54Z

    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

  4. Re: Incorrect fsync handling in pg_basebackup's tar_finish

    Michael Paquier <michael@paquier.xyz> — 2018-06-25T13:17:29Z

    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

  5. Re: Incorrect fsync handling in pg_basebackup's tar_finish

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2018-06-25T13:51:27Z

    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

  6. Re: Incorrect fsync handling in pg_basebackup's tar_finish

    Michael Paquier <michael@paquier.xyz> — 2018-06-26T00:58:11Z

    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