From 78a33bba4c634afc3c67dddeb359b5ce872a0b04 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 19 Sep 2023 10:19:50 +0900 Subject: [PATCH] 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 --- src/backend/utils/time/snapmgr.c | 18 +++++++++++++++--- src/test/regress/expected/transactions.out | 11 +++++++++++ src/test/regress/sql/transactions.sql | 9 +++++++++ 3 files changed, 35 insertions(+), 3 deletions(-) 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.