Re: Have better wording for snapshot file reading failure

[Michael Paquier]

On Thu, Sep 14, 2023 at 05:33:35PM -0700, Andres Freund wrote:
> I'd probably just go for something like "snapshot \"%s\" does not exist",
> similar to what we report for unknown tables etc. Arguably changing the
> errcode to ERRCODE_UNDEFINED_OBJECT would make this more precise?

Good points.  Updated as suggested in v2 attached.
--
Michael

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index b20440ee21..4a3613d15f 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 428c9edcc6..7717967a9f 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 75ffe929d4..db2535c787 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.

signature.asc
Description: PGP signature