Re: BufFileRead() error signalling

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.