On Wed, Sep 13, 2023 at 07:09:32PM -0700, Andres Freund wrote:
> On 2023-09-13 19:07:24 -0700, Andres Freund wrote:
>> Huh. I don't think this is a good idea - and certainly not in the back
>> branches. The prior message made more sense, imo. The fact that the snapshot
>> identifier is a file is an implementation detail, no snapshot with the
>> identifier being exported is a user level detail. Hence that being mentioned
>> in the error message.
>>
>> I can see an argument for treating ENOENT different than other errors though,
>> and using the standard file opening error message for anything other than
>> ENOENT.
>
> Oh, and given that this actually changes the error code for an invalid
> snapshot, I think this needs to be reverted. It's not that unlikely that
> there's code out there that depends on getting ERRCODE_INVALID_PARAMETER_VALUE
> when the snapshot doesn't exist.
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.
Saying that, I'm OK with reverting to the previous behavior on
back-branches if you feel strongly about that.
--
Michael