Have better wording for snapshot file reading failure
Hi,
When a snapshot file reading fails in ImportSnapshot(), it errors out
with "invalid snapshot identifier". This message better suits for
snapshot identifier parsing errors which is being done just before the
file reading. The attached patch adds a generic file reading error
message with path to help distinguish if the issue is with snapshot
identifier parsing or file reading.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Have-better-wording-for-snapshot-file-reading-fai.patchapplication/octet-stream; name=v1-0001-Have-better-wording-for-snapshot-file-reading-fai.patchDownload
From 5cf457e61367ca8420406b3d14c40708bd922707 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Wed, 13 Sep 2023 06:04:48 +0000
Subject: [PATCH v1] Have better wording for snapshot file reading failure
When a snapshot file reading fails in ImportSnapshot(), it errors
out with "invalid snapshot identifier". This message better suits
for snapshot identifier parsing errors which is being done just
before the file reading. This commit adds a generic file reading
error message with path to help distinguish if the issue is with
snapshot identifier parsing or file reading.
---
src/backend/utils/time/snapmgr.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index b20440ee21..65230811c0 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1391,8 +1391,9 @@ 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)));
+ (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))
--
2.34.1
On Wed, Sep 13, 2023 at 11:40:25AM +0530, Bharath Rupireddy wrote:
When a snapshot file reading fails in ImportSnapshot(), it errors out
with "invalid snapshot identifier". This message better suits for
snapshot identifier parsing errors which is being done just before the
file reading. The attached patch adds a generic file reading error
message with path to help distinguish if the issue is with snapshot
identifier parsing or file reading.
f = AllocateFile(path, PG_BINARY_R);
if (!f)
ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("invalid snapshot identifier: \"%s\"", idstr)));
+ (errcode_for_file_access(),
+ errmsg("could not open file \"%s\" for reading: %m",
+ path)));Agreed that this just looks like a copy-pasto. The path provides
enough context about what's being read, so using this generic error
message is fine. Will apply if there are no objections.
--
Michael
On 9/13/23 02:10, Bharath Rupireddy wrote:
Hi,
When a snapshot file reading fails in ImportSnapshot(), it errors out
with "invalid snapshot identifier". This message better suits for
snapshot identifier parsing errors which is being done just before the
file reading. The attached patch adds a generic file reading error
message with path to help distinguish if the issue is with snapshot
identifier parsing or file reading.
I suggest error message to include "snapshot" keyword in message, like this:
errmsg("could not open snapshot file \"%s\" for reading: %m",
and also tweak other messages accordingly.
--
Kind Regards,
Yogesh Sharma
PostgreSQL, Linux, and Networking Expert
Open Source Enthusiast and Advocate
On Wed, Sep 13, 2023 at 3:32 PM Yogesh Sharma
<yogesh.sharma@catprosystems.com> wrote:
On 9/13/23 02:10, Bharath Rupireddy wrote:
Hi,
When a snapshot file reading fails in ImportSnapshot(), it errors out
with "invalid snapshot identifier". This message better suits for
snapshot identifier parsing errors which is being done just before the
file reading. The attached patch adds a generic file reading error
message with path to help distinguish if the issue is with snapshot
identifier parsing or file reading.I suggest error message to include "snapshot" keyword in message, like this:
errmsg("could not open snapshot file \"%s\" for reading: %m",
and also tweak other messages accordingly.
-1. The path includes the pg_snapshots there which is enough to give
the clue, so no need to say "could not open snapshot file". AFAICS,
this is the typical messaging followed across postgres code for
AllocateFile failures.
[1]: /* Define pathname of exported-snapshot files */ #define SNAPSHOT_EXPORT_DIR "pg_snapshots"
/* Define pathname of exported-snapshot files */
#define SNAPSHOT_EXPORT_DIR "pg_snapshots"
/* OK, read the file */
snprintf(path, MAXPGPATH, SNAPSHOT_EXPORT_DIR "/%s", idstr);
f = AllocateFile(path, PG_BINARY_R);
if (!f)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open file \"%s\" for reading: %m",
path)));
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On 13 Sep 2023, at 08:18, Michael Paquier <michael@paquier.xyz> wrote:
f = AllocateFile(path, PG_BINARY_R); if (!f) ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("invalid snapshot identifier: \"%s\"", idstr))); + (errcode_for_file_access(), + errmsg("could not open file \"%s\" for reading: %m", + path)));Agreed that this just looks like a copy-pasto. The path provides
enough context about what's being read, so using this generic error
message is fine. Will apply if there are no objections.
+1. This errmsg is already present so it eases the translation burden as well.
--
Daniel Gustafsson
On Wed, Sep 13, 2023 at 01:19:38PM +0200, Daniel Gustafsson wrote:
+1. This errmsg is already present so it eases the translation burden as well.
I was thinking about doing only that on HEAD, but there is an argument
that one could get confusing errors when dealing with snapshot imports
on back-branches as well, and it applies down to 11 without conflicts.
So, applied and backpatched.
--
Michael
Hi,
On 2023-09-14 10:33:33 +0900, Michael Paquier wrote:
On Wed, Sep 13, 2023 at 01:19:38PM +0200, Daniel Gustafsson wrote:
+1. This errmsg is already present so it eases the translation burden as well.
I was thinking about doing only that on HEAD, but there is an argument
that one could get confusing errors when dealing with snapshot imports
on back-branches as well, and it applies down to 11 without conflicts.
So, applied and backpatched.
Huh. I don't think this is a good idea - and certainly not in the back
branches. The prior message made more sense, imo. The fact that the snapshot
identifier is a file is an implementation detail, no snapshot with the
identifier being exported is a user level detail. Hence that being mentioned
in the error message.
I can see an argument for treating ENOENT different than other errors though,
and using the standard file opening error message for anything other than
ENOENT.
Greetings,
Andres
Hi,
On 2023-09-13 19:07:24 -0700, Andres Freund wrote:
On 2023-09-14 10:33:33 +0900, Michael Paquier wrote:
On Wed, Sep 13, 2023 at 01:19:38PM +0200, Daniel Gustafsson wrote:
+1. This errmsg is already present so it eases the translation burden as well.
I was thinking about doing only that on HEAD, but there is an argument
that one could get confusing errors when dealing with snapshot imports
on back-branches as well, and it applies down to 11 without conflicts.
So, applied and backpatched.Huh. I don't think this is a good idea - and certainly not in the back
branches. The prior message made more sense, imo. The fact that the snapshot
identifier is a file is an implementation detail, no snapshot with the
identifier being exported is a user level detail. Hence that being mentioned
in the error message.I can see an argument for treating ENOENT different than other errors though,
and using the standard file opening error message for anything other than
ENOENT.
Oh, and given that this actually changes the error code for an invalid
snapshot, I think this needs to be reverted. It's not that unlikely that
there's code out there that depends on getting ERRCODE_INVALID_PARAMETER_VALUE
when the snapshot doesn't exist.
- Andres
On Wed, Sep 13, 2023 at 07:09:32PM -0700, Andres Freund wrote:
On 2023-09-13 19:07:24 -0700, Andres Freund wrote:
Huh. I don't think this is a good idea - and certainly not in the back
branches. The prior message made more sense, imo. The fact that the snapshot
identifier is a file is an implementation detail, no snapshot with the
identifier being exported is a user level detail. Hence that being mentioned
in the error message.I can see an argument for treating ENOENT different than other errors though,
and using the standard file opening error message for anything other than
ENOENT.Oh, and given that this actually changes the error code for an invalid
snapshot, I think this needs to be reverted. It's not that unlikely that
there's code out there that depends on getting ERRCODE_INVALID_PARAMETER_VALUE
when the snapshot doesn't exist.
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.
Saying that, I'm OK with reverting to the previous behavior on
back-branches if you feel strongly about that.
--
Michael
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
Attachments:
snapmgr-import-error.patchtext/x-diff; charset=us-asciiDownload
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.
Hi,
On 2023-09-14 16:29:22 +0900, Michael Paquier wrote:
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.
I wouldn't call it quite incorrect, but it's certainly a good idea to provide
relevant details for the rare case of errors other than ENOENT.
So, how about a "could not find the requested snapshot" if the snapshot ID
is valid but its file cannot be found?
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?
This new suggestion is only for HEAD. I've reverted a0d87bc & co for
now.
I think there's really no reason to backpatch this, so that makes sense to me.
Greetings,
Andres Freund
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
Attachments:
snapmgr-import-error-v2.patchtext/x-diff; charset=us-asciiDownload
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.
On 9/14/23 20:33, 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?
+1 better informative message compare to the original patch.
--
Kind Regards,
Yogesh Sharma
PostgreSQL, Linux, and Networking Expert
Open Source Enthusiast and Advocate