Re: Have better wording for snapshot file reading failure

Hi,

On 2023-09-14 16:29:22 +0900, Michael Paquier wrote:
> On Thu, Sep 14, 2023 at 01:33:39PM +0900, Michael Paquier wrote:
> > Ahem. This seems to be the only code path that tracks a failure on
> > AllocateFile() where we don't show %m at all, while the error is
> > misleading in basically all the cases as errno holds the extra
> > information telling somebody that something's going wrong, so I don't
> > quite see how it is useful to tell "invalid snapshot identifier" on
> > an EACCES or even ENOENT when opening this file, with zero information
> > about what's happening on top of that? Even on ENOENT, one can be
> > confused with the same error message generated a few lines above: if
> > AllocateFile() fails, the snapshot identifier is correctly shaped, but
> > its file is missing. If ENOENT is considered a particular case with
> > the old message, we'd still not know if this refers to the first
> > failure or the second failure.
>
> I see your point after thinking about it, the new message would show
> up when running a SET TRANSACTION SNAPSHOT with a value id, which is
> not helpful either. Your idea of filtering out ENOENT may be the best
> move to get more information on %m. Still, it looks to me that using
> the same error message for both cases is incorrect.

I wouldn't call it quite incorrect, but it's certainly a good idea to provide
relevant details for the rare case of errors other than ENOENT.

> So, how about a "could not find the requested snapshot" if the snapshot ID
> is valid but its file cannot be found?

I'd probably just go for something like "snapshot \"%s\" does not exist",
similar to what we report for unknown tables etc. Arguably changing the
errcode to ERRCODE_UNDEFINED_OBJECT would make this more precise?

> This new suggestion is only for HEAD. I've reverted a0d87bc & co for
> now.

I think there's really no reason to backpatch this, so that makes sense to me.

Greetings,

Andres Freund