Horton
Menu

Re: Have better wording for snapshot file reading failure

Michael Paquier <michael@paquier.xyz>

View thread
From: Michael Paquier <michael@paquier.xyz>
To: Andres Freund <andres@anarazel.de>
Cc: Daniel Gustafsson <daniel@yesql.se>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2023-09-14T07:29:22Z
Lists: pgsql-hackers

Attachments

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

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Improve error message for snapshot import in snapmgr.c, take two

  2. Revert "Improve error message on snapshot import in snapmgr.c"

  3. Improve error message on snapshot import in snapmgr.c