On Thu, Nov 21, 2019 at 11:31 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.mu...@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.
0001-Raise-errors-for-I-O-errors-during-BufFileRead.patch
Description: Binary data
0002-Report-I-O-errors-from-BufFileFlush-via-ereport.patch
Description: Binary data
0003-Align-BufFileWrite-s-error-reporting-with-BufFileRea.patch
Description: Binary data
