Greetings,
* Ashutosh Bapat (ashutosh.bapat@enterprisedb.com) wrote:
> On Thu, May 17, 2018 at 11:45 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Thu, May 17, 2018 at 12:44 PM, Andres Freund <andres@anarazel.de> wrote:
> >> Hi,
> >>
> >> On 2018-05-10 09:50:03 +0800, Craig Ringer wrote:
> >>> while ((src = (RewriteMappingFile *) hash_seq_search(&seq_status)) != NULL)
> >>> {
> >>> if (FileSync(src->vfd, WAIT_EVENT_LOGICAL_REWRITE_SYNC) != 0)
> >>> - ereport(ERROR,
> >>> + ereport(PANIC,
> >>> (errcode_for_file_access(),
> >>> errmsg("could not fsync file \"%s\": %m", src->path)));
> >>
> >> To me this (and the other callers) doesn't quite look right. First, I
> >> think we should probably be a bit more restrictive about when PANIC
> >> out. It seems like we should PANIC on ENOSPC and EIO, but possibly not
> >> others. Secondly, I think we should centralize the error handling. It
> >> seems likely that we'll acrue some platform specific workarounds, and I
> >> don't want to copy that knowledge everywhere.
> >
> > Maybe something like:
> >
> > ereport(promote_eio_to_panic(ERROR), ...)
>
> Well, searching for places where error is reported with level PANIC,
> using word PANIC would miss these instances. People will have to
> remember to search with promote_eio_to_panic. May be handle the errors
> inside FileSync() itself or a wrapper around that.
No, that search wouldn't miss those instances- such a search would find
promote_eio_to_panic() and then someone would go look up the uses of
that function. That hardly seems like a serious issue for folks hacking
on PG.
I'm not saying that having a wrapper around FileSync() would be bad or
having it handle things, but I don't agree with the general notion that
we can't have a function which handles the complicated bits about the
kind of error because someone grep'ing the source for PANIC might have
to do an additional lookup.
Thanks!
Stephen
