Greetings, * Ashutosh Bapat (ashutosh.ba...@enterprisedb.com) wrote: > On Thu, May 17, 2018 at 11:45 PM, Robert Haas <robertmh...@gmail.com> wrote: > > On Thu, May 17, 2018 at 12:44 PM, Andres Freund <and...@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
signature.asc
Description: PGP signature