Thread: BufFileRead() error signalling
Hi,
As noted by Amit Khandhekar yesterday[1], BufFileLoad() silently eats
pread()'s error and makes them indistinguishable from EOF.
Some observations:
1. BufFileRead() returns size_t (not ssize_t), so it's an
fread()-style interface, not a read()-style interface that could use
-1 to signal errors. Unlike fread(), it doesn't seem to have anything
corresponding to feof()/ferror()/clearerr() that lets the caller
distinguish between EOF and error, so our hash and tuplestore spill
code simply trusts that if there is a 0 size read where it expects a
tuple boundary, it must be EOF.
2. BufFileWrite() is the same, but just like fwrite(), a short write
must always mean error, so there is no problem here.
3. The calling code assumes that unexpected short read or write sets
errno, which isn't the documented behaviour of fwrite() and fread(),
so there we're doing something a bit different (which is fine, just
pointing it out; we're sort of somewhere between the <stdio.h> and
<unistd.h> functions, in terms of error reporting).
I think the choices are: (1) switch to ssize_t and return -1, (2) add
at least one of BufFileEof(), BufFileError(), (3) have BufFileRead()
raise errors with elog(). I lean towards (2), and I doubt we need
BufFileClear() because the only thing we ever do in client code on
error is immediately burn the world down.
If we simply added an error flag to track if FileRead() had ever
signalled an error, we could change nodeHashjoin.c to do something
along these lines:
- if (nread == 0) /* end of file */
+ if (!BufFileError(file) && nread == 0) /* end of file */
... and likewise for tuplestore.c:
- if (nbytes != 0 || !eofOK)
+ if (BufFileError(file) || (nbytes == 0 && !eofOK))
ereport(ERROR,
About the only advantage to the current design I can see if that you
can probably make your query finish faster by pulling out your temp
tablespace USB stick at the right time. Or am I missing some
complication?
[1] https://www.postgresql.org/message-id/CAJ3gD9emnEys%3DR%2BT3OVt_87DuMpMfS4KvoRV6e%3DiSi5PT%2B9f3w%40mail.gmail.com
On Thu, Nov 21, 2019 at 10:50 AM Thomas Munro <thomas.munro@gmail.com> wrote: > As noted by Amit Khandhekar yesterday[1], BufFileLoad() silently eats Erm, Khandekar, sorry for the extra h!
Thomas Munro <thomas.munro@gmail.com> writes:
> As noted by Amit Khandhekar yesterday[1], BufFileLoad() silently eats
> pread()'s error and makes them indistinguishable from EOF.
That's definitely bad.
> I think the choices are: (1) switch to ssize_t and return -1, (2) add
> at least one of BufFileEof(), BufFileError(), (3) have BufFileRead()
> raise errors with elog(). I lean towards (2), and I doubt we need
> BufFileClear() because the only thing we ever do in client code on
> error is immediately burn the world down.
I'd vote for (3), I think. Making the callers responsible for error
checks just leaves us with a permanent hazard of errors-of-omission,
and as you say, there's really no use-case where we'd be trying to
recover from the error.
I think that the motivation for making the caller do it might've
been an idea that the caller could provide a more useful error
message, but I'm not real sure that that's true --- the caller
doesn't know the physical file's name, and it doesn't necessarily
have the right errno either.
Possibly we could address any loss of usefulness by requiring callers
to pass some sort of context identification to BufFileCreate?
regards, tom lane
On Thu, Nov 21, 2019 at 11:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.munro@gmail.com> writes: > > I think the choices are: (1) switch to ssize_t and return -1, (2) add > > at least one of BufFileEof(), BufFileError(), (3) have BufFileRead() > > raise errors with elog(). I lean towards (2), and I doubt we need > > BufFileClear() because the only thing we ever do in client code on > > error is immediately burn the world down. > > I'd vote for (3), I think. Making the callers responsible for error > checks just leaves us with a permanent hazard of errors-of-omission, > and as you say, there's really no use-case where we'd be trying to > recover from the error. Ok. Here is a first attempt at that. I also noticed that some callers of BufFileFlush() eat or disguise I/O errors too, so here's a patch for that, though I'm a little confused about the exact meaning of EOF from BufFileSeek(). > I think that the motivation for making the caller do it might've > been an idea that the caller could provide a more useful error > message, but I'm not real sure that that's true --- the caller > doesn't know the physical file's name, and it doesn't necessarily > have the right errno either. Yeah, the errno is undefined right now since we don't know if there was an error. > Possibly we could address any loss of usefulness by requiring callers > to pass some sort of context identification to BufFileCreate? Hmm. It's an idea. While thinking about the cohesion of this module's API, I thought it seemed pretty strange to have BufFileWrite() using a different style of error reporting, so here's an experimental 0003 patch to make it consistent. I realise that an API change might affect extensions, so I'm not sure if it's a good idea even for master (obviously it's not back-patchable). You could be right that more context would be good at least in the case of ENOSPC: knowing that (say) a hash join or a sort or CTE is implicated would be helpful.
Attachment
You are checking file->dirty twice, first before calling the function and within the function too. Same for the Assert.
Forexample.
size_t
BufFileRead(BufFile *file, void *ptr, size_t size)
{
size_t nread = 0;
size_t nthistime;
if (file->dirty)
{
BufFileFlush(file);
Assert(!file->dirty);
}
static void
BufFileFlush(BufFile *file)
{
if (file->dirty)
BufFileDumpBuffer(file);
Assert(!file->dirty);
The new status of this patch is: Waiting on Author
On Wed, Dec 11, 2019 at 2:07 AM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote: > You are checking file->dirty twice, first before calling the function and within the function too. Same for the Assert.For example. True. Thanks for the review. Before I post a new version, any opinions on whether to back-patch, and whether to do 0003 in master only, or at all?
On Fri, Jan 24, 2020 at 11:12 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Wed, Dec 11, 2019 at 2:07 AM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
> > You are checking file->dirty twice, first before calling the function and within the function too. Same for the
Assert.For example.
>
> True. Thanks for the review. Before I post a new version, any
> opinions on whether to back-patch, and whether to do 0003 in master
> only, or at all?
Rather than answering your actual question, I'd like to complain about this:
if (BufFileRead(file, ptr, BLCKSZ) != BLCKSZ)
- elog(ERROR, "could not read temporary file: %m");
+ elog(ERROR, "could not read temporary file");
I recognize that your commit message explains this change by saying
that this code will now never be reached except as a result of a short
read, but I don't think that will necessarily be clear to future
readers of those code, or people who get the error message. It seems
like if we're going to do do this, the error messages ought to be
changed to complain about a short read rather than an inability to
read for unspecified reasons. However, I wonder why we don't make
BufFileRead() throw all of the errors including complaining about
short reads. I would like to confess my undying (and probably
unrequited) love for the following code from md.c:
errmsg("could not read block
%u in file \"%s\": read only %d of %d bytes",
Now that is an error message! I am not confused! I don't know why that
happened, but I sure know what happened!
I think we should aim for that kind of style everywhere, so that
complaints about reading and writing files are typically reported as
either of these:
could not read file "%s": %m
could not read file "%s": read only %d of %d bytes
There is an existing precedent in twophase.c which works almost this way:
if (r != stat.st_size)
{
if (r < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not read file
\"%s\": %m", path)));
else
ereport(ERROR,
(errmsg("could not read file
\"%s\": read %d of %zu",
path, r,
(Size) stat.st_size)));
}
I'd advocate for a couple more words in the latter message ("only" and
"bytes") but it's still excellent.
OK, now that I've waxed eloquent on that topic, let me have a go at
your actual questions. Regarding back-patching, I don't mind
back-patching error handling patches like this, but I don't feel it's
necessary if we have no evidence that data is actually getting
corrupted as a result of the problem and the chances of it actually
happening seems remote. It's worth keeping in mind that changes to
message strings will tend to degrade translatability unless the new
strings are copies of existing strings. Regarding 0003, it seems good
to me to make BufFileRead() and BufFileWrite() consistent in terms of
error-handling behavior, so +1 for the concept (but I haven't reviewed
the code).
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, Jan 27, 2020 at 10:09:30AM -0500, Robert Haas wrote:
> I recognize that your commit message explains this change by saying
> that this code will now never be reached except as a result of a short
> read, but I don't think that will necessarily be clear to future
> readers of those code, or people who get the error message. It seems
> like if we're going to do do this, the error messages ought to be
> changed to complain about a short read rather than an inability to
> read for unspecified reasons. However, I wonder why we don't make
> BufFileRead() throw all of the errors including complaining about
> short reads. I would like to confess my undying (and probably
> unrequited) love for the following code from md.c:
>
> errmsg("could not read block
> %u in file \"%s\": read only %d of %d bytes",
>
> Now that is an error message! I am not confused! I don't know why that
> happened, but I sure know what happened!
I was briefly looking at 0001, and count -1 from me for the
formulation of the error messages used in those patches.
> I think we should aim for that kind of style everywhere, so that
> complaints about reading and writing files are typically reported as
> either of these:
>
> could not read file "%s": %m
> could not read file "%s": read only %d of %d bytes
That's actually not the best fit, because this does not take care of
the pluralization of the second message if you have only 1 byte to
read ;)
A second point to take into account is that the unification of error
messages makes for less translation work, which is always welcome.
Those points have been discussed on this thread:
https://www.postgresql.org/message-id/20180520000522.GB1603@paquier.xyz
The related commit is 811b6e3, and the pattern we agreed on for a
partial read was:
"could not read file \"%s\": read %d of %zu"
Then, if the syscall had an error we'd fall down to that:
"could not read file \"%s\": %m"
--
Michael
Attachment
On Mon, Jan 27, 2020 at 9:03 PM Michael Paquier <michael@paquier.xyz> wrote: > That's actually not the best fit, because this does not take care of > the pluralization of the second message if you have only 1 byte to > read ;) But ... if you have only one byte to read, you cannot have a short read. > A second point to take into account is that the unification of error > messages makes for less translation work, which is always welcome. > Those points have been discussed on this thread: > https://www.postgresql.org/message-id/20180520000522.GB1603@paquier.xyz I quickly reread that thread and I don't see that there's any firm consensus there in favor of "read %d of %zu" over "read only %d of %zu bytes". Now, if most people prefer the former, so be it, but I don't think that's clear from that thread. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Jan 28, 2020 at 03:51:54PM -0500, Robert Haas wrote: > I quickly reread that thread and I don't see that there's any firm > consensus there in favor of "read %d of %zu" over "read only %d of %zu > bytes". Now, if most people prefer the former, so be it, but I don't > think that's clear from that thread. The argument of consistency falls in favor of the former on HEAD: $ git grep "could not read" | grep "read %d of %zu" | wc -l 59 $ git grep "could not read" | grep "read only %d of %zu" | wc -l 0 -- Michael
Attachment
On Wed, Jan 29, 2020 at 1:26 AM Michael Paquier <michael@paquier.xyz> wrote: > On Tue, Jan 28, 2020 at 03:51:54PM -0500, Robert Haas wrote: > > I quickly reread that thread and I don't see that there's any firm > > consensus there in favor of "read %d of %zu" over "read only %d of %zu > > bytes". Now, if most people prefer the former, so be it, but I don't > > think that's clear from that thread. > > The argument of consistency falls in favor of the former on HEAD: > $ git grep "could not read" | grep "read %d of %zu" | wc -l > 59 > $ git grep "could not read" | grep "read only %d of %zu" | wc -l > 0 True. I didn't realize that 'read %d of %zu' was so widely used. Your grep misses one instance of 'read only %d of %d bytes' because you grepped for %zu specifically, but that doesn't really change the overall picture. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Jan 29, 2020 at 10:01:31AM -0500, Robert Haas wrote: > Your grep misses one instance of 'read only %d of %d bytes' because > you grepped for %zu specifically, but that doesn't really change the > overall picture. Yes, the one in pg_checksums.c. That could actually be changed with a cast to Size. (Note that there is a second one related to writes but there is a precedent in md.c, and a similar one in rewriteheap.c..) Sorry for the digression. -- Michael
Attachment
On Mon, Jan 27, 2020 at 7:09 AM Robert Haas <robertmhaas@gmail.com> wrote:
Rather than answering your actual question, I'd like to complain about this:
if (BufFileRead(file, ptr, BLCKSZ) != BLCKSZ)
- elog(ERROR, "could not read temporary file: %m");
+ elog(ERROR, "could not read temporary file");
I recognize that your commit message explains this change by saying
that this code will now never be reached except as a result of a short
read, but I don't think that will necessarily be clear to future
readers of those code, or people who get the error message. It seems
like if we're going to do do this, the error messages ought to be
changed to complain about a short read rather than an inability to
read for unspecified reasons. However, I wonder why we don't make
BufFileRead() throw all of the errors including complaining about
short reads. I would like to confess my undying (and probably
unrequited) love for the following code from md.c:
errmsg("could not read block
%u in file \"%s\": read only %d of %d bytes",
It would be cool to have the block number in more cases in error
messages. For example, in sts_parallel_scan_next()/* Seek and load the chunk header. */
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.")));
I'm actually in favor of having the block number in this error
message. I think it would be helpful for multi-batch parallel
hashjoin. If a worker reading one SharedTuplestoreChunk encounters an
error and another worker on a different SharedTuplstoreChunk of the
same file does not, I would find it useful to know more about which
block it was on when the error was encountered.
--
Melanie Plageman
Hi Thomas, On 11/29/19 9:46 PM, Thomas Munro wrote: > > Ok. Here is a first attempt at that. It's been a few CFs since this patch received an update, though there has been plenty of discussion. Perhaps it would be best to mark it RwF until you have a chance to produce an update patch? Regards, -- -David david@pgmasters.net
On 2020-Jan-29, Michael Paquier wrote: > On Tue, Jan 28, 2020 at 03:51:54PM -0500, Robert Haas wrote: > > I quickly reread that thread and I don't see that there's any firm > > consensus there in favor of "read %d of %zu" over "read only %d of %zu > > bytes". Now, if most people prefer the former, so be it, but I don't > > think that's clear from that thread. > > The argument of consistency falls in favor of the former on HEAD: > $ git grep "could not read" | grep "read %d of %zu" | wc -l > 59 > $ git grep "could not read" | grep "read only %d of %zu" | wc -l > 0 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. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020-Jan-27, Robert Haas wrote: > OK, now that I've waxed eloquent on that topic, let me have a go at > your actual questions. Regarding back-patching, I don't mind > back-patching error handling patches like this, but I don't feel it's > necessary if we have no evidence that data is actually getting > corrupted as a result of the problem and the chances of it actually > happening seems remote. I do have evidence of postgres crashes because of a problem that could be explained by this bug, so I +1 backpatching this to all supported branches. (The problem I saw is a hash-join spilling data to temp tablespace, which fills up but somehow goes undetected, then when reading the data back it causes heap_fill_tuple to crash.) Thomas, if you're no longer interested in seeing this done, please let me know and I can see to it. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, May 27, 2020 at 12:16 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > I do have evidence of postgres crashes because of a problem that could > be explained by this bug, so I +1 backpatching this to all supported > branches. > > (The problem I saw is a hash-join spilling data to temp tablespace, > which fills up but somehow goes undetected, then when reading the data > back it causes heap_fill_tuple to crash.) FWIW, that seems like a plenty good enough reason for back-patching to me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, May 28, 2020 at 4:16 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2020-Jan-27, Robert Haas wrote: > > OK, now that I've waxed eloquent on that topic, let me have a go at > > your actual questions. Regarding back-patching, I don't mind > > back-patching error handling patches like this, but I don't feel it's > > necessary if we have no evidence that data is actually getting > > corrupted as a result of the problem and the chances of it actually > > happening seems remote. > > I do have evidence of postgres crashes because of a problem that could > be explained by this bug, so I +1 backpatching this to all supported > branches. > > (The problem I saw is a hash-join spilling data to temp tablespace, > which fills up but somehow goes undetected, then when reading the data > back it causes heap_fill_tuple to crash.) Ooh. > Thomas, if you're no longer interested in seeing this done, please let > me know and I can see to it. My indecision on the back-patching question has been resolved by your crash report and a search on codesearch.debian.org. BufFileRead() and BufFileWrite() aren't referenced by any of the extensions they package, so by that standard it's OK to change this stuff in back branches. I'll post a rebased a patch with Robert and Ibrar's changes for last reviews later today.
On 2020-May-28, Thomas Munro wrote: > My indecision on the back-patching question has been resolved by your > crash report and a search on codesearch.debian.org. Great news! > BufFileRead() and BufFileWrite() aren't referenced by any of the > extensions they package, so by that standard it's OK to change this > stuff in back branches. This makes me a bit uncomfortable. For example, https://inst.eecs.berkeley.edu/~cs186/fa03/hwk5/assign5.html (admittedly a very old class) encourages students to use this API to create an aggregate. It might not be the smartest thing in the world, but I'd prefer not to break such things if they exist proprietarily. Can we keep the API unchanged in stable branches and just ereport the errors? > I'll post a rebased a patch with Robert and Ibrar's changes > for last reviews later today. ... walks away wondering about BufFileSeekBlock's API ... (BufFileSeek seems harder to change, due to tuplestore.c) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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). -- Michael
Attachment
On Thu, May 28, 2020 at 7:10 PM Michael Paquier <michael@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".
Attachment
On Fri, Jun 05, 2020 at 06:03:59PM +1200, Thomas Munro wrote:
> 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.
Missing one entry in AppendStringToManifest(). It sounds right to not
change the signature of the routine on back-branches to any ABI
breakages. It think that it could change on HEAD.
Anyway, why are we sure that it is fine to not complain even if
BufFileWrite() does a partial write? fwrite() is mentioned at the top
of the thread, but why is that OK?
> 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".
- 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);
You mean "in temporary file" in the new message, no?
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not write to \"%s\" : %m",
+ FilePathName(thisfile))));
Nit: "could not write [to] file \"%s\": %m" is a more common error
string.
--
Michael
Attachment
On Fri, Jun 5, 2020 at 8:14 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Jun 05, 2020 at 06:03:59PM +1200, Thomas Munro wrote:
> > 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.
>
> Missing one entry in AppendStringToManifest().
Fixed.
> Anyway, why are we sure that it is fine to not complain even if
> BufFileWrite() does a partial write? fwrite() is mentioned at the top
> of the thread, but why is that OK?
It's not OK. If any system call fails, we'll now ereport()
immediately. Now there can't be unhandled or unreported errors, and
it's no longer possible for the caller to confuse EOF with errors.
Hence the change in descriptions:
- * 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().
- * Like fwrite() except we assume 1-byte element size.
+ * Like fwrite() except we assume 1-byte element size and report errors via
+ * ereport().
Stepping back a bit, one of the problems here is that we tried to
model the functions on <stdio.h> fread(), but we didn't provide the
companion ferror() and feof() functions, and then we were a bit fuzzy
on when errno is set, even though the <stdio.h> functions don't
document that. There were various ways forward, but the one that this
patch follows is to switch to our regular error reporting system. The
only thing that really costs us is marginally more vague error
messages. Perhaps that could eventually be fixed by passing in some
more context into calls like BufFileCreateTemp(), for use in error
messages and perhaps also path names.
Does this make sense?
> + elog(ERROR, "could not seek block %ld temporary file", blknum);
>
> You mean "in temporary file" in the new message, no?
Fixed.
> + ereport(ERROR,
> + (errcode_for_file_access(),
> + errmsg("could not write to \"%s\" : %m",
> + FilePathName(thisfile))));
>
> Nit: "could not write [to] file \"%s\": %m" is a more common error
> string.
Fixed.
Attachment
On 2020-Jun-08, Thomas Munro wrote: > Stepping back a bit, one of the problems here is that we tried to > model the functions on <stdio.h> fread(), but we didn't provide the > companion ferror() and feof() functions, and then we were a bit fuzzy > on when errno is set, even though the <stdio.h> functions don't > document that. There were various ways forward, but the one that this > patch follows is to switch to our regular error reporting system. The > only thing that really costs us is marginally more vague error > messages. Perhaps that could eventually be fixed by passing in some > more context into calls like BufFileCreateTemp(), for use in error > messages and perhaps also path names. I think using our standard "exception" mechanism makes sense. As for additional context, I think usefulness of the error messages would be improved by showing the file path (because then user knows which filesystem/tablespace was full, for example), but IMO any additional context on top of that is of marginal additional benefit. If we really cared, we could have errcontext() callbacks in the sites of interest, but that would be a separate patch and perhaps not backpatchable. > > + elog(ERROR, "could not seek block %ld temporary file", blknum); > > > > You mean "in temporary file" in the new message, no? > > Fixed. The wording we use is "could not seek TO block N". (Or used to use, before switching to pread/pwrite in most places, it seems). Thanks! -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Jun 9, 2020 at 2:49 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > I think using our standard "exception" mechanism makes sense. As for > additional context, I think usefulness of the error messages would be > improved by showing the file path (because then user knows which > filesystem/tablespace was full, for example), but IMO any additional > context on top of that is of marginal additional benefit. If we really > cared, we could have errcontext() callbacks in the sites of interest, > but that would be a separate patch and perhaps not backpatchable. Cool. It does show the path, so that'll tell you which file system is full or broken. I thought a bit about the ENOSPC thing, and took that change out. Since commit 1173344e we handle write() returning a positive number less than the full size by predicting that a follow-up call to write() would surely return ENOSPC, without the hassle of trying to write more, or having a separate error message sans %m. But BufFileDumpBuffer() does try again, and only raises an error if the system call returns < 0 (well, it says <= 0, but 0 is impossible according to POSIX, at least assuming you didn't try to write zero bytes, and we already exclude that). So ENOSPC-prediction is unnecessary here. > The wording we use is "could not seek TO block N". (Or used to use, > before switching to pread/pwrite in most places, it seems). Fixed in a couple of places.
Attachment
On Mon, Jun 08, 2020 at 05:50:44PM +1200, Thomas Munro wrote: > On Fri, Jun 5, 2020 at 8:14 PM Michael Paquier <michael@paquier.xyz> wrote:\ >> Anyway, why are we sure that it is fine to not complain even if >> BufFileWrite() does a partial write? fwrite() is mentioned at the >> top >> of the thread, but why is that OK? > > It's not OK. If any system call fails, we'll now ereport() > immediately. Now there can't be unhandled or unreported errors, and > it's no longer possible for the caller to confuse EOF with errors. > Hence the change in descriptions: Oh, OK. I looked at that again this morning and I see your point now. I was wondering if it could be possible to have BufFileWrite() write less data than what is expected with errno=0. The code of HEAD would issue a confusing error message like "could not write: Success" in such a case, still it would fail on ERROR. And I thought that your patch would do a different thing and would cause this code path to not fail in such a case, but the point I missed on the first read of your patch is that BufFileWrite() is written is such a way that we would actually just keep looping until the amount of data to write is written, meaning that we should never see anymore the case where BufFileWrite() returns a size that does not match with the expected size to write. On Tue, Jun 09, 2020 at 12:21:53PM +1200, Thomas Munro wrote: > On Tue, Jun 9, 2020 at 2:49 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> I think using our standard "exception" mechanism makes sense. As for >> additional context, I think usefulness of the error messages would be >> improved by showing the file path (because then user knows which >> filesystem/tablespace was full, for example), but IMO any additional >> context on top of that is of marginal additional benefit. If we really >> cared, we could have errcontext() callbacks in the sites of interest, >> but that would be a separate patch and perhaps not backpatchable. > > Cool. It does show the path, so that'll tell you which file system is > full or broken. There are some places in logtape.c, *tuplestore.c and gist where there is no file path. That would be nice to have, but that's not really the problem of this patch. > I thought a bit about the ENOSPC thing, and took that change out. > Since commit 1173344e we handle write() returning a positive number > less than the full size by predicting that a follow-up call to write() > would surely return ENOSPC, without the hassle of trying to write > more, or having a separate error message sans %m. But > BufFileDumpBuffer() does try again, and only raises an error if the > system call returns < 0 (well, it says <= 0, but 0 is impossible > according to POSIX, at least assuming you didn't try to write zero > bytes, and we already exclude that). So ENOSPC-prediction is > unnecessary here. +1. Makes sense. >> The wording we use is "could not seek TO block N". (Or used to use, >> before switching to pread/pwrite in most places, it seems). > > Fixed in a couple of places. Looks fine to me. Are you planning to send an extra patch to switch BufFileWrite() to void for 14~? -- Michael
Attachment
On Tue, Jun 9, 2020 at 2:32 PM Michael Paquier <michael@paquier.xyz> wrote: > Looks fine to me. Are you planning to send an extra patch to switch > BufFileWrite() to void for 14~? Thanks! Pushed. I went ahead and made it void in master only.
On Tue, Jun 16, 2020 at 05:41:31PM +1200, Thomas Munro wrote: > Thanks! Pushed. I went ahead and made it void in master only. Thanks. -- Michael