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. So, how about a
"could not find the requested snapshot" if the snapshot ID is valid
but its file cannot be found? We don't have any tests for the failure
paths, either, so I've added some.
This new suggestion is only for HEAD. I've reverted a0d87bc & co for
now.
--
Michael