On Thu, Aug 29, 2019 at 3:17 PM Jeevan Ladhe <[email protected]>
wrote:
> Hi Jeevan
>
> I had a look at the patch and this seems correct to me.
>
Thanks, Jeevan Ladhe.
>
> Few minor comments:
>
> + /* Check fread() error. */
> + CHECK_FREAD_ERROR(fp, pathbuf);
> +
>
> The comments above the macro call at both the places are not necessary as
> your macro name itself is self-explanatory.
>
> ----------
> + /*
> + * If file is truncated, then we will hit
> + * end-of-file error in which case we don't
> + * want to error out, instead just pad it with
> + * zeros.
> + */
> + if (feof(fp))
>
> The if block does not do the truncation right away, so I think the comment
> above can be reworded to explain why we reset cnt?
>
Fixed both comments in the attached patch.
--
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index c91f66d..3aea470 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -90,6 +90,18 @@ static char *statrelpath = NULL;
*/
#define THROTTLING_FREQUENCY 8
+/*
+ * Checks whether we encountered any error in fread(). fread() doesn't give
+ * any clue what has happened, so we check with ferror(). Also, neither
+ * fread() nor ferror() set errno, so we just throw a generic error.
+ */
+#define CHECK_FREAD_ERROR(fp, filename) \
+do { \
+ if (ferror(fp)) \
+ ereport(ERROR, \
+ (errmsg("could not read from file \"%s\"", filename))); \
+} while (0)
+
/* The actual number of bytes, transfer of which may cause sleep. */
static uint64 throttling_sample;
@@ -543,6 +555,8 @@ perform_base_backup(basebackup_options *opt)
break;
}
+ CHECK_FREAD_ERROR(fp, pathbuf);
+
if (len != wal_segment_size)
{
CheckXLogRemoved(segno, tli);
@@ -1478,6 +1492,20 @@ sendFile(const char *readfilename, const char *tarfilename, struct stat *statbuf
if (fread(buf + BLCKSZ * i, 1, BLCKSZ, fp) != BLCKSZ)
{
+ /*
+ * If the file is truncated, then we will hit
+ * an end-of-file error in which case we don't
+ * want to error out, instead just pad the rest
+ * of the file with zeros. However, we want to
+ * send all the bytes read so far, and thus set
+ * the cnt accordingly.
+ */
+ if (feof(fp))
+ {
+ cnt = BLCKSZ * i;
+ break;
+ }
+
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not reread block %d of file \"%s\": %m",
@@ -1529,7 +1557,7 @@ sendFile(const char *readfilename, const char *tarfilename, struct stat *statbuf
len += cnt;
throttle(cnt);
- if (len >= statbuf->st_size)
+ if (feof(fp) || len >= statbuf->st_size)
{
/*
* Reached end of file. The file could be longer, if it was
@@ -1540,6 +1568,8 @@ sendFile(const char *readfilename, const char *tarfilename, struct stat *statbuf
}
}
+ CHECK_FREAD_ERROR(fp, readfilename);
+
/* If the file was truncated while we were sending it, pad it with zeros */
if (len < statbuf->st_size)
{
