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
Attachments:
walmethod-fsync.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index 331d0e7275..7867a56ee1 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -896,7 +896,7 @@ tar_finish(void)
return false;
}
- /* A tarfile always ends with two empty blocks */
+ /* A tarfile always ends with two empty blocks */
MemSet(zerobuf, 0, sizeof(zerobuf));
if (!tar_data->compression)
{
@@ -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;
+ }
if (close(tar_data->fd) != 0)
return false;
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/>
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
Attachments:
walmethod-fsync-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index 331d0e7275..fbfee05a5a 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -865,7 +865,8 @@ tar_close(Walfile f, WalCloseMethod method)
return -1;
/* Always fsync on close, so the padding gets fsynced */
- tar_sync(f);
+ if (tar_sync(f) < 0)
+ return -1;
/* Clean up and done */
pg_free(tf->pathname);
@@ -896,7 +897,7 @@ tar_finish(void)
return false;
}
- /* A tarfile always ends with two empty blocks */
+ /* A tarfile always ends with two empty blocks */
MemSet(zerobuf, 0, sizeof(zerobuf));
if (!tar_data->compression)
{
@@ -957,7 +958,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;
+ }
if (close(tar_data->fd) != 0)
return false;
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