On Thu, May 28, 2020 at 7:10 PM Michael Paquier <mich...@paquier.xyz> wrote: > On Wed, May 27, 2020 at 11:59:59AM -0400, Alvaro Herrera wrote: > > In the discussion that led to 811b6e36a9e2 I did suggest to use "read > > only M of N" instead, but there wasn't enough discussion on that fine > > point so we settled on what you now call prevalent without a lot of > > support specifically on that. I guess it was enough of an improvement > > over what was there. But like Robert, I too prefer the wording that > > includes "only" and "bytes" over the wording that doesn't. > > > > I'll let it be known that from a translator's point of view, it's a > > ten-seconds job to update a fuzzy string from not including "only" and > > "bytes" to one that does. So let's not make that an argument for not > > changing. > > Using "only" would be fine by me, though I tend to prefer the existing > one. Now I think that we should avoid "bytes" to not have to worry > about pluralization of error messages. This has been a concern in the > past (see e5d11b9 and the likes).
Sorry for the delay in producing a new patch. Here's one that tries to take into account the various feedback in this thread: Ibrar said: > You are checking file->dirty twice, first before calling the function > and within the function too. Same for the Assert. Fixed. Robert, Melanie, Michael and Alvaro put forward various arguments about the form of "short read" and block seek error message. While removing bogus cases of "%m", I changed them all to say "read only %zu of %zu bytes" in the same place. I removed the bogus "%m" from BufFileSeek() and BufFileSeekBlock() callers (its call to BufFileFlush() already throws). I added block numbers to the error messages about failure to seek by block. Following existing practice, I made write failure assume ENOSPC if errno is 0, so that %m would make sense (I am not sure what platform reports out-of-space that way, but there are certainly a lot of copies of that code in the tree so it seemed to be missing here). I didn't change BufFileWrite() to be void, to be friendly to existing callers outside the tree (if there are any), though I removed all the code that checks the return code. We can make it void later. For the future: it feels a bit like we're missing a one line way to say "read this many bytes and error out if you run out".
From ab6f90f5e5f3b6e70de28a4ee734e732c2a8c30b Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Sat, 30 Nov 2019 12:44:47 +1300 Subject: [PATCH] Redesign error handling in buffile.c. Convert error handling in buffile.c functions to raise errors rather than expecting callers to check the return value, and improve the error messages. This fixes various cases that forgot to check for errors, or couldn't check for errors because EOF was indistinguishable from error, or checked for errors unless they fell on certain boundaries which they assumed to be EOF. This had been identified by code inspection, but was later identified as the probable cause of a crash report involving a corrupted hash join spill file when disk space ran out. Back-patch to all releases. For now, BufFileWrite() still returns a redundant value so as not to change the function prototype for the benefit of any extensions that might be using it. Reported-by: Amit Khandekar <amitdkhan...@gmail.com> Reported-by: Alvaro Herrera <alvhe...@2ndquadrant.com> Reviewed-by: Melanie Plageman <melanieplage...@gmail.com> Reviewed-by: Alvaro Herrera <alvhe...@2ndquadrant.com> Reviewed-by: Robert Haas <robertmh...@gmail.com> Reviewed-by: Ibrar Ahmed <ibrar.ah...@gmail.com> Reviewed-by: Michael Paquier <mich...@paquier.xyz> Discussion: https://postgr.es/m/CA%2BhUKGJE04G%3D8TLK0DLypT_27D9dR8F1RQgNp0jK6qR0tZGWOw%40mail.gmail.com --- src/backend/access/gist/gistbuildbuffers.c | 23 ++++----- src/backend/executor/nodeHashjoin.c | 24 ++++------ src/backend/replication/backup_manifest.c | 2 +- src/backend/storage/file/buffile.c | 51 +++++++++++--------- src/backend/utils/sort/logtape.c | 18 ++++--- src/backend/utils/sort/sharedtuplestore.c | 21 ++++---- src/backend/utils/sort/tuplestore.c | 56 ++++++++++------------ 7 files changed, 91 insertions(+), 104 deletions(-) diff --git a/src/backend/access/gist/gistbuildbuffers.c b/src/backend/access/gist/gistbuildbuffers.c index 4b562d8d3f..7ff513a5fe 100644 --- a/src/backend/access/gist/gistbuildbuffers.c +++ b/src/backend/access/gist/gistbuildbuffers.c @@ -757,26 +757,19 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate, static void ReadTempFileBlock(BufFile *file, long blknum, void *ptr) { + size_t nread; + if (BufFileSeekBlock(file, blknum) != 0) - elog(ERROR, "could not seek temporary file: %m"); - if (BufFileRead(file, ptr, BLCKSZ) != BLCKSZ) - elog(ERROR, "could not read temporary file: %m"); + elog(ERROR, "could not seek block %ld in temporary file", blknum); + if ((nread = BufFileRead(file, ptr, BLCKSZ)) != BLCKSZ) + elog(ERROR, "could not read temporary file: read only %zu of %zu bytes", + nread, (size_t) BLCKSZ); } static void WriteTempFileBlock(BufFile *file, long blknum, void *ptr) { if (BufFileSeekBlock(file, blknum) != 0) - elog(ERROR, "could not seek temporary file: %m"); - if (BufFileWrite(file, ptr, BLCKSZ) != BLCKSZ) - { - /* - * the other errors in Read/WriteTempFileBlock shouldn't happen, but - * an error at write can easily happen if you run out of disk space. - */ - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not write block %ld of temporary file: %m", - blknum))); - } + elog(ERROR, "could not seek block %ld temporary file", blknum); + BufFileWrite(file, ptr, BLCKSZ); } diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c index 2cdc38a601..9bb23fef1a 100644 --- a/src/backend/executor/nodeHashjoin.c +++ b/src/backend/executor/nodeHashjoin.c @@ -1043,7 +1043,7 @@ ExecHashJoinNewBatch(HashJoinState *hjstate) if (BufFileSeek(innerFile, 0, 0L, SEEK_SET)) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not rewind hash-join temporary file: %m"))); + errmsg("could not rewind hash-join temporary file"))); while ((slot = ExecHashJoinGetSavedTuple(hjstate, innerFile, @@ -1073,7 +1073,7 @@ ExecHashJoinNewBatch(HashJoinState *hjstate) if (BufFileSeek(hashtable->outerBatchFile[curbatch], 0, 0L, SEEK_SET)) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not rewind hash-join temporary file: %m"))); + errmsg("could not rewind hash-join temporary file"))); } return true; @@ -1219,7 +1219,6 @@ ExecHashJoinSaveTuple(MinimalTuple tuple, uint32 hashvalue, BufFile **fileptr) { BufFile *file = *fileptr; - size_t written; if (file == NULL) { @@ -1228,17 +1227,8 @@ ExecHashJoinSaveTuple(MinimalTuple tuple, uint32 hashvalue, *fileptr = file; } - written = BufFileWrite(file, (void *) &hashvalue, sizeof(uint32)); - if (written != sizeof(uint32)) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not write to hash-join temporary file: %m"))); - - written = BufFileWrite(file, (void *) tuple, tuple->t_len); - if (written != tuple->t_len) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not write to hash-join temporary file: %m"))); + BufFileWrite(file, (void *) &hashvalue, sizeof(uint32)); + BufFileWrite(file, (void *) tuple, tuple->t_len); } /* @@ -1279,7 +1269,8 @@ ExecHashJoinGetSavedTuple(HashJoinState *hjstate, if (nread != sizeof(header)) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not read from hash-join temporary file: %m"))); + errmsg("could not read from hash-join temporary file: read only %zu of %zu bytes", + nread, sizeof(header)))); *hashvalue = header[0]; tuple = (MinimalTuple) palloc(header[1]); tuple->t_len = header[1]; @@ -1289,7 +1280,8 @@ ExecHashJoinGetSavedTuple(HashJoinState *hjstate, if (nread != header[1] - sizeof(uint32)) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not read from hash-join temporary file: %m"))); + errmsg("could not read from hash-join temporary file: read only %zu of %zu bytes", + nread, header[1] - sizeof(uint32)))); ExecForceStoreMinimalTuple(tuple, tupleSlot, true); return tupleSlot; } diff --git a/src/backend/replication/backup_manifest.c b/src/backend/replication/backup_manifest.c index 807c5f16b4..261ef31f28 100644 --- a/src/backend/replication/backup_manifest.c +++ b/src/backend/replication/backup_manifest.c @@ -319,7 +319,7 @@ SendBackupManifest(backup_manifest_info *manifest) if (BufFileSeek(manifest->buffile, 0, 0L, SEEK_SET)) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not rewind temporary file: %m"))); + errmsg("could not rewind temporary file"))); /* Send CopyOutResponse message */ pq_beginmessage(&protobuf, 'H'); diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c index 35e8f12e62..dab748535e 100644 --- a/src/backend/storage/file/buffile.c +++ b/src/backend/storage/file/buffile.c @@ -99,7 +99,7 @@ static BufFile *makeBufFile(File firstfile); static void extendBufFile(BufFile *file); static void BufFileLoadBuffer(BufFile *file); static void BufFileDumpBuffer(BufFile *file); -static int BufFileFlush(BufFile *file); +static void BufFileFlush(BufFile *file); static File MakeNewSharedSegment(BufFile *file, int segment); /* @@ -434,7 +434,14 @@ BufFileLoadBuffer(BufFile *file) file->curOffset, WAIT_EVENT_BUFFILE_READ); if (file->nbytes < 0) + { file->nbytes = 0; + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not read file \"%s\": %m", + FilePathName(thisfile)))); + } + /* we choose not to advance curOffset here */ if (file->nbytes > 0) @@ -484,13 +491,22 @@ BufFileDumpBuffer(BufFile *file) bytestowrite = (int) availbytes; thisfile = file->files[file->curFile]; + errno = 0; bytestowrite = FileWrite(thisfile, file->buffer.data + wpos, bytestowrite, file->curOffset, WAIT_EVENT_BUFFILE_WRITE); if (bytestowrite <= 0) - return; /* failed to write */ + { + /* if write didn't set errno, assume no disk space */ + if (errno == 0) + errno = ENOSPC; + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not write to \"%s\" : %m", + FilePathName(thisfile)))); + } file->curOffset += bytestowrite; wpos += bytestowrite; @@ -522,7 +538,8 @@ BufFileDumpBuffer(BufFile *file) /* * BufFileRead * - * Like fread() except we assume 1-byte element size. + * Like fread() except we assume 1-byte element size and report I/O errors via + * ereport(). */ size_t BufFileRead(BufFile *file, void *ptr, size_t size) @@ -530,12 +547,7 @@ BufFileRead(BufFile *file, void *ptr, size_t size) size_t nread = 0; size_t nthistime; - if (file->dirty) - { - if (BufFileFlush(file) != 0) - return 0; /* could not flush... */ - Assert(!file->dirty); - } + BufFileFlush(file); while (size > 0) { @@ -569,7 +581,8 @@ BufFileRead(BufFile *file, void *ptr, size_t size) /* * BufFileWrite * - * Like fwrite() except we assume 1-byte element size. + * Like fwrite() except we assume 1-byte element size and report errors via + * ereport(). */ size_t BufFileWrite(BufFile *file, void *ptr, size_t size) @@ -585,11 +598,7 @@ BufFileWrite(BufFile *file, void *ptr, size_t size) { /* Buffer full, dump it out */ if (file->dirty) - { BufFileDumpBuffer(file); - if (file->dirty) - break; /* I/O error */ - } else { /* Hmm, went directly from reading to writing? */ @@ -621,19 +630,15 @@ BufFileWrite(BufFile *file, void *ptr, size_t size) /* * BufFileFlush * - * Like fflush() + * Like fflush(), except that I/O errors are reported with ereport(). */ -static int +static void BufFileFlush(BufFile *file) { if (file->dirty) - { BufFileDumpBuffer(file); - if (file->dirty) - return EOF; - } - return 0; + Assert(!file->dirty); } /* @@ -642,6 +647,7 @@ BufFileFlush(BufFile *file) * Like fseek(), except that target position needs two values in order to * work when logical filesize exceeds maximum value representable by off_t. * We do not support relative seeks across more than that, however. + * I/O errors are reported by ereport(). * * Result is 0 if OK, EOF if not. Logical position is not moved if an * impossible seek is attempted. @@ -699,8 +705,7 @@ BufFileSeek(BufFile *file, int fileno, off_t offset, int whence) return 0; } /* Otherwise, must reposition buffer, so flush any dirty data */ - if (BufFileFlush(file) != 0) - return EOF; + BufFileFlush(file); /* * At this point and no sooner, check for seek past last segment. The diff --git a/src/backend/utils/sort/logtape.c b/src/backend/utils/sort/logtape.c index 666a7c0e81..db5357793b 100644 --- a/src/backend/utils/sort/logtape.c +++ b/src/backend/utils/sort/logtape.c @@ -262,12 +262,12 @@ ltsWriteBlock(LogicalTapeSet *lts, long blocknum, void *buffer) } /* Write the requested block */ - if (BufFileSeekBlock(lts->pfile, blocknum) != 0 || - BufFileWrite(lts->pfile, buffer, BLCKSZ) != BLCKSZ) + if (BufFileSeekBlock(lts->pfile, blocknum) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not write block %ld of temporary file: %m", + errmsg("could not seek block %ld of temporary file", blocknum))); + BufFileWrite(lts->pfile, buffer, BLCKSZ); /* Update nBlocksWritten, if we extended the file */ if (blocknum == lts->nBlocksWritten) @@ -283,12 +283,18 @@ ltsWriteBlock(LogicalTapeSet *lts, long blocknum, void *buffer) static void ltsReadBlock(LogicalTapeSet *lts, long blocknum, void *buffer) { - if (BufFileSeekBlock(lts->pfile, blocknum) != 0 || - BufFileRead(lts->pfile, buffer, BLCKSZ) != BLCKSZ) + size_t nread; + + if (BufFileSeekBlock(lts->pfile, blocknum) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not read block %ld of temporary file: %m", + errmsg("could not seek to block %ld of temporary file", blocknum))); + if ((nread = BufFileRead(lts->pfile, buffer, BLCKSZ)) != BLCKSZ) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not read block %ld of temporary file: read only %zu of %zu bytes", + blocknum, nread, (size_t) BLCKSZ))); } /* diff --git a/src/backend/utils/sort/sharedtuplestore.c b/src/backend/utils/sort/sharedtuplestore.c index c3ab494a45..6537a4303b 100644 --- a/src/backend/utils/sort/sharedtuplestore.c +++ b/src/backend/utils/sort/sharedtuplestore.c @@ -196,14 +196,9 @@ static void sts_flush_chunk(SharedTuplestoreAccessor *accessor) { size_t size; - size_t written; size = STS_CHUNK_PAGES * BLCKSZ; - written = BufFileWrite(accessor->write_file, accessor->write_chunk, size); - if (written != size) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not write to temporary file: %m"))); + BufFileWrite(accessor->write_file, accessor->write_chunk, size); memset(accessor->write_chunk, 0, size); accessor->write_pointer = &accessor->write_chunk->data[0]; accessor->sts->participants[accessor->participant].npages += @@ -555,6 +550,7 @@ sts_parallel_scan_next(SharedTuplestoreAccessor *accessor, void *meta_data) if (!eof) { SharedTuplestoreChunk chunk_header; + size_t nread; /* Make sure we have the file open. */ if (accessor->read_file == NULL) @@ -570,14 +566,15 @@ sts_parallel_scan_next(SharedTuplestoreAccessor *accessor, void *meta_data) if (BufFileSeekBlock(accessor->read_file, read_page) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not read from shared tuplestore temporary file"), - errdetail_internal("Could not seek to next block."))); - if (BufFileRead(accessor->read_file, &chunk_header, - STS_CHUNK_HEADER_SIZE) != STS_CHUNK_HEADER_SIZE) + errmsg("could not seek block %u in shared tuplestore temporary file", + read_page))); + nread = BufFileRead(accessor->read_file, &chunk_header, + STS_CHUNK_HEADER_SIZE); + if (nread != STS_CHUNK_HEADER_SIZE) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not read from shared tuplestore temporary file"), - errdetail_internal("Short read while reading chunk header."))); + errmsg("could not read from shared tuplestore temporary file: read only %zu of %zu bytes", + nread, STS_CHUNK_HEADER_SIZE))); /* * If this is an overflow chunk, we skip it and any following diff --git a/src/backend/utils/sort/tuplestore.c b/src/backend/utils/sort/tuplestore.c index ebb1da0746..452a85a423 100644 --- a/src/backend/utils/sort/tuplestore.c +++ b/src/backend/utils/sort/tuplestore.c @@ -515,7 +515,7 @@ tuplestore_select_read_pointer(Tuplestorestate *state, int ptr) SEEK_SET) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not seek in tuplestore temporary file: %m"))); + errmsg("could not seek in tuplestore temporary file"))); } else { @@ -525,7 +525,7 @@ tuplestore_select_read_pointer(Tuplestorestate *state, int ptr) SEEK_SET) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not seek in tuplestore temporary file: %m"))); + errmsg("could not seek in tuplestore temporary file"))); } break; default: @@ -866,7 +866,7 @@ tuplestore_puttuple_common(Tuplestorestate *state, void *tuple) SEEK_SET) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not seek in tuplestore temporary file: %m"))); + errmsg("could not seek in tuplestore temporary file"))); state->status = TSS_WRITEFILE; /* @@ -970,7 +970,7 @@ tuplestore_gettuple(Tuplestorestate *state, bool forward, SEEK_SET) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not seek in tuplestore temporary file: %m"))); + errmsg("could not seek in tuplestore temporary file"))); state->status = TSS_READFILE; /* FALLTHROUGH */ @@ -1034,7 +1034,7 @@ tuplestore_gettuple(Tuplestorestate *state, bool forward, SEEK_CUR) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not seek in tuplestore temporary file: %m"))); + errmsg("could not seek in tuplestore temporary file"))); Assert(!state->truncated); return NULL; } @@ -1051,7 +1051,7 @@ tuplestore_gettuple(Tuplestorestate *state, bool forward, SEEK_CUR) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not seek in tuplestore temporary file: %m"))); + errmsg("could not seek in tuplestore temporary file"))); tup = READTUP(state, tuplen); return tup; @@ -1253,7 +1253,7 @@ tuplestore_rescan(Tuplestorestate *state) if (BufFileSeek(state->myfile, 0, 0L, SEEK_SET) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not seek in tuplestore temporary file: %m"))); + errmsg("could not seek in tuplestore temporary file"))); break; default: elog(ERROR, "invalid tuplestore state"); @@ -1318,7 +1318,7 @@ tuplestore_copy_read_pointer(Tuplestorestate *state, SEEK_SET) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not seek in tuplestore temporary file: %m"))); + errmsg("could not seek in tuplestore temporary file"))); } else { @@ -1327,7 +1327,7 @@ tuplestore_copy_read_pointer(Tuplestorestate *state, SEEK_SET) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not seek in tuplestore temporary file: %m"))); + errmsg("could not seek in tuplestore temporary file"))); } } else if (srcptr == state->activeptr) @@ -1474,7 +1474,8 @@ getlen(Tuplestorestate *state, bool eofOK) if (nbytes != 0 || !eofOK) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not read from tuplestore temporary file: %m"))); + errmsg("could not read from tuplestore temporary file: read only %zu of %zu bytes", + nbytes, sizeof(len)))); return 0; } @@ -1511,22 +1512,10 @@ writetup_heap(Tuplestorestate *state, void *tup) /* total on-disk footprint: */ unsigned int tuplen = tupbodylen + sizeof(int); - if (BufFileWrite(state->myfile, (void *) &tuplen, - sizeof(tuplen)) != sizeof(tuplen)) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not write to tuplestore temporary file: %m"))); - if (BufFileWrite(state->myfile, (void *) tupbody, - tupbodylen) != (size_t) tupbodylen) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not write to tuplestore temporary file: %m"))); + BufFileWrite(state->myfile, (void *) &tuplen, sizeof(tuplen)); + BufFileWrite(state->myfile, (void *) tupbody, tupbodylen); if (state->backward) /* need trailing length word? */ - if (BufFileWrite(state->myfile, (void *) &tuplen, - sizeof(tuplen)) != sizeof(tuplen)) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not write to tuplestore temporary file: %m"))); + BufFileWrite(state->myfile, (void *) &tuplen, sizeof(tuplen)); FREEMEM(state, GetMemoryChunkSpace(tuple)); heap_free_minimal_tuple(tuple); @@ -1539,20 +1528,25 @@ readtup_heap(Tuplestorestate *state, unsigned int len) unsigned int tuplen = tupbodylen + MINIMAL_TUPLE_DATA_OFFSET; MinimalTuple tuple = (MinimalTuple) palloc(tuplen); char *tupbody = (char *) tuple + MINIMAL_TUPLE_DATA_OFFSET; + size_t nread; USEMEM(state, GetMemoryChunkSpace(tuple)); /* read in the tuple proper */ tuple->t_len = tuplen; - if (BufFileRead(state->myfile, (void *) tupbody, - tupbodylen) != (size_t) tupbodylen) + nread = BufFileRead(state->myfile, (void *) tupbody, tupbodylen); + if (nread != (size_t) tupbodylen) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not read from tuplestore temporary file: %m"))); + errmsg("could not read from tuplestore temporary file: read only %zu of %zu bytes", + nread, (size_t) tupbodylen))); if (state->backward) /* need trailing length word? */ - if (BufFileRead(state->myfile, (void *) &tuplen, - sizeof(tuplen)) != sizeof(tuplen)) + { + nread = BufFileRead(state->myfile, (void *) &tuplen, sizeof(tuplen)); + if (nread != sizeof(tuplen)) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not read from tuplestore temporary file: %m"))); + errmsg("could not read from tuplestore temporary file: read only %zu of %zu bytes", + nread, sizeof(tuplen)))); + } return (void *) tuple; } -- 2.20.1
