On Thu, Aug 29, 2019 at 3:17 PM Jeevan Ladhe <jeevan.la...@enterprisedb.com> 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) {