Re: Have better wording for snapshot file reading failure

On Wed, Sep 13, 2023 at 3:32 PM Yogesh Sharma
<yogesh(dot)sharma(at)catprosystems(dot)com> wrote:
>
> On 9/13/23 02:10, Bharath Rupireddy wrote:
> > Hi,
> >
> > When a snapshot file reading fails in ImportSnapshot(), it errors out
> > with "invalid snapshot identifier". This message better suits for
> > snapshot identifier parsing errors which is being done just before the
> > file reading. The attached patch adds a generic file reading error
> > message with path to help distinguish if the issue is with snapshot
> > identifier parsing or file reading.
> >
> I suggest error message to include "snapshot" keyword in message, like this:
>
> errmsg("could not open snapshot file \"%s\" for reading: %m",
>
> and also tweak other messages accordingly.

-1. The path includes the pg_snapshots there which is enough to give
the clue, so no need to say "could not open snapshot file". AFAICS,
this is the typical messaging followed across postgres code for
AllocateFile failures.

[1]
/* Define pathname of exported-snapshot files */
#define SNAPSHOT_EXPORT_DIR "pg_snapshots"

/* OK, read the file */
snprintf(path, MAXPGPATH, SNAPSHOT_EXPORT_DIR "/%s", idstr);

f = AllocateFile(path, PG_BINARY_R);
if (!f)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open file \"%s\" for reading: %m",
path)));

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com