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

michaelpq · michaelpq · commit 78a33bba4c63 · 2023-09-19T10:19:50.000+09:00
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 bb446b6 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
diff --git 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
@@ -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
@@ -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.
