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

When a snapshot file fails to be read in ImportSnapshot(), it would
issue an ERROR as "invalid snapshot identifier" when opening a stream
for it in read-only mode.  The error handling is improved to be more
talkative in failure cases:
- If a snapshot identifier uses incorrect characters, complain with the
same error as before this commit.
- If the snapshot file cannot be found in pg_snapshots/, complain with a
"snapshot \"foo\" does not exist" instead.  This maps to the case where
AllocateFile() fails on ENOENT.  Based on a suggestion from Andres
Freund.
- If AllocateFile() throws something else than ENOENT as errno, report
it with more details in %m instead, as these failures are never
expected.

b29504eeb489 was the first improvement take.  The older error message
exists since bb446b689b66 that introduced snapshot imports.  Two test
cases are added to cover the cases of an identifier with an incorrect
format and of a missing snapshot.

Author: Bharath Rupireddy
Reviewed-by: Andres Freund, Daniel Gustafsson, Michael Paquier
Discussion: https://postgr.es/m/CALj2ACWmr=3KdxDkm8h7Zn1XxBoF6hdzq8WQyMn2y1OL5RYFrg@mail.gmail.com

3 files changed, 35 insertions, 3 deletions

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index b20440ee21f..4a3613d15fc 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1390,9 +1390,21 @@ ImportSnapshot(const char *idstr)
 
 	f = AllocateFile(path, PG_BINARY_R);
 	if (!f)
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("invalid snapshot identifier: \"%s\"", idstr)));
+	{
+		/*
+		 * If file is missing while identifier has a correct format, avoid
+		 * system errors.
+		 */
+		if (errno == ENOENT)
+			ereport(ERROR,
+					(errcode(ERRCODE_UNDEFINED_OBJECT),
+					 errmsg("snapshot \"%s\" does not exist", idstr)));
+		else
+			ereport(ERROR,
+					(errcode_for_file_access(),
+					 errmsg("could not open file \"%s\" for reading: %m",
+							path)));
+	}
 
 	/* get the size of the file so that we know how much memory we need */
 	if (fstat(fileno(f), &stat_buf))
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index 428c9edcc6f..7717967a9f3 100644
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -1148,6 +1148,17 @@ SELECT * FROM trans_abc ORDER BY 1;
 (3 rows)
 
 DROP TABLE trans_abc;
+-- TRANSACTION SNAPSHOT
+-- Incorrect identifier.
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SET TRANSACTION SNAPSHOT 'Incorrect Identifier';
+ERROR:  invalid snapshot identifier: "Incorrect Identifier"
+ROLLBACK;
+-- Correct identifier, missing file.
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SET TRANSACTION SNAPSHOT 'FFF-FFF-F';
+ERROR:  snapshot "FFF-FFF-F" does not exist
+ROLLBACK;
 -- Test for successful cleanup of an aborted transaction at session exit.
 -- THIS MUST BE THE LAST TEST IN THIS FILE.
 begin;
diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql
index 75ffe929d45..db2535c7878 100644
--- a/src/test/regress/sql/transactions.sql
+++ b/src/test/regress/sql/transactions.sql
@@ -613,6 +613,15 @@ SELECT * FROM trans_abc ORDER BY 1;
 
 DROP TABLE trans_abc;
 
+-- TRANSACTION SNAPSHOT
+-- Incorrect identifier.
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SET TRANSACTION SNAPSHOT 'Incorrect Identifier';
+ROLLBACK;
+-- Correct identifier, missing file.
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SET TRANSACTION SNAPSHOT 'FFF-FFF-F';
+ROLLBACK;
 
 -- Test for successful cleanup of an aborted transaction at session exit.
 -- THIS MUST BE THE LAST TEST IN THIS FILE.