Re: BufFileRead() error signalling

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