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

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index b20440ee21ff8ad836aa8ef72aa0d18fd3dd85f3..4a3613d15fcd9450de3bcb07f75debf40e70b385 100644 (file)
--- 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 428c9edcc6f0508fb488b40d705fce6e7d0f2dd7..7717967a9f3a8d3cbeeee72549fdd3c7c84c2fc7 100644 (file)
--- 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 75ffe929d455d68cae3620ca12492931cbf3b332..db2535c787801fee03212f4c37be8b35cca40e2d 100644 (file)
--- 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.