On Thu, Sep 14, 2023 at 01:33:39PM +0900, Michael Paquier wrote: > Ahem. This seems to be the only code path that tracks a failure on > AllocateFile() where we don't show %m at all, while the error is > misleading in basically all the cases as errno holds the extra > information telling somebody that something's going wrong, so I don't > quite see how it is useful to tell "invalid snapshot identifier" on > an EACCES or even ENOENT when opening this file, with zero information > about what's happening on top of that? Even on ENOENT, one can be > confused with the same error message generated a few lines above: if > AllocateFile() fails, the snapshot identifier is correctly shaped, but > its file is missing. If ENOENT is considered a particular case with > the old message, we'd still not know if this refers to the first > failure or the second failure.
I see your point after thinking about it, the new message would show up when running a SET TRANSACTION SNAPSHOT with a value id, which is not helpful either. Your idea of filtering out ENOENT may be the best move to get more information on %m. Still, it looks to me that using the same error message for both cases is incorrect. So, how about a "could not find the requested snapshot" if the snapshot ID is valid but its file cannot be found? We don't have any tests for the failure paths, either, so I've added some. This new suggestion is only for HEAD. I've reverted a0d87bc & co for now. -- Michael
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index b20440ee21..3040873672 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_INVALID_PARAMETER_VALUE),
+ errmsg("could not find the requested snapshot: \"%s\"", 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..7a674b2156 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: could not find the requested 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.
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
