Re: [HACKERS] [bugfix] commit timestamps ERROR on lookup of FrozenTransactionId
On 25 November 2016 at 02:44, Alvaro Herrera wrote: > Craig Ringer wrote: > >> Updated to correct the other expected file, since there's an alternate. > > FWIW I don't know what you did here, but you did not patch the > alternate expected file. Damn. Attached the first patch a second time is what I did. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [bugfix] commit timestamps ERROR on lookup of FrozenTransactionId
Craig Ringer wrote: > Updated to correct the other expected file, since there's an alternate. FWIW I don't know what you did here, but you did not patch the alternate expected file. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [bugfix] commit timestamps ERROR on lookup of FrozenTransactionId
Tom Lane wrote: > Alvaro Herrera writes: > > I considered the argument here for a bit and I think Craig is right -- > > FWIW, I agree. We shouldn't require every call site to special-case this, > and we definitely don't want it to require special cases in SQL code. > > (And I'm for back-patching, too.) Pushed. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [bugfix] commit timestamps ERROR on lookup of FrozenTransactionId
Alvaro Herrera writes: > I considered the argument here for a bit and I think Craig is right -- FWIW, I agree. We shouldn't require every call site to special-case this, and we definitely don't want it to require special cases in SQL code. (And I'm for back-patching, too.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [bugfix] commit timestamps ERROR on lookup of FrozenTransactionId
I considered the argument here for a bit and I think Craig is right -- FrozenXid eventually makes it to a tuple's xmin where it becomes a burden to the caller, making our interface bug-prone -- sure you can special-case it, but you don't until it first happens ... and it may not until you're deep into production. Even the code comment is confused: "error if the given Xid doesn't normally commit". But surely FrozenXid *does* commit in the sense that it appears in committed tuples' Xmin. We already have a good mechanism for replying to the query with "this value is too old for us to have its commit TS", which is a false return value. We should use that. I think not backpatching is worse, because then users have to be aware that they need to handle the FrozenXid case specially, but only on 9.5/9.6 ... I think the reason it took this long to pop up is because it has taken this long to get to replication systems on which this issue matters. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [bugfix] commit timestamps ERROR on lookup of FrozenTransactionId
On 24 November 2016 at 02:32, Andres Freund wrote: > Hi, > > On 2016-11-23 20:58:22 +0800, Craig Ringer wrote: >> Today I ran into an issue where commit timestamp lookups were failing with >> >> ERROR: cannot retrieve commit timestamp for transaction 2 >> >> which is of course FrozenTransactionId. >> >> TransactionIdGetCommitTsData(...) ERRORs on !TransactionIdIsNormal(), >> which I think is wrong. Attached is a patch to make it return 0 for >> FrozenTransactionId and BootstrapTransactionId, like it does for xids >> that are too old. > > Why? It seems quite correct to not allow lookups for special case > values, as it seems sensible to give them special treatmeant at the call > site? It's surprising behaviour that doesn't make sense. Look at it this way: - We do some work, generating rows that have commit timestamps - TransactionIdGetCommitTsData() on those rows returns their cts fine - The commit timestamp data ages out - TransactionIdGetCommitTsData() returns 0 on these rows - vacuum comes alone and freezes the rows, even though nothing's changed - TransactionIdGetCommitTsData() suddenly ERRORs Nothing has meaningfully changed on these rows. They have gone from "old, committed, past the commit timestamp threshold" to "old, commited, past the commit timestamp threshold, frozen". It makes no sense to ERROR when vacuum gets around to freezing the tuples, when we don't also ERROR when we pass the cts threshold. ERRORing on BootstrapTransactionId is slightly more reasonable since those rows can never have had a cts in the first place, but it's also unnecessary since they're effectively "oldest always-committed xids". Making it ERROR on FrozenTransactionId was a mistake and should be corrected. >> IMO this should be back-patched to 9.6 and, without the TAP test part, >> to 9.5. > > Why would we want to backpatch a behaviour change, where arguments for > the current and proposed behaviour exists? I don't think it's crucial since callers can just work around it, but IMO the current behaviour is a design oversight that should be corrected and can be safely and sensibly corrected. Nobody's going to rely on FrozenTransactionId ERRORing. I don't think a backpatch is crucial though; as you note, C-level callers can work around the problem pretty simply, and that's just what I've done in pglogical for existing versions. I just think it's ugly, should be fixed, and is safe to fix. It's slightly harder for SQL-level callers to work around since they must hardcode a CASE that tests for xmin = XID '1' OR xmin = XID '2', and it's much less reasonable to expect SQL level callers to deal with this sort of mess with low level state. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [bugfix] commit timestamps ERROR on lookup of FrozenTransactionId
Hi, On 2016-11-23 20:58:22 +0800, Craig Ringer wrote: > Today I ran into an issue where commit timestamp lookups were failing with > > ERROR: cannot retrieve commit timestamp for transaction 2 > > which is of course FrozenTransactionId. > > TransactionIdGetCommitTsData(...) ERRORs on !TransactionIdIsNormal(), > which I think is wrong. Attached is a patch to make it return 0 for > FrozenTransactionId and BootstrapTransactionId, like it does for xids > that are too old. Why? It seems quite correct to not allow lookups for special case values, as it seems sensible to give them special treatmeant at the call site? > IMO this should be back-patched to 9.6 and, without the TAP test part, > to 9.5. Why would we want to backpatch a behaviour change, where arguments for the current and proposed behaviour exists? Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [bugfix] commit timestamps ERROR on lookup of FrozenTransactionId
On 23 November 2016 at 20:58, Craig Ringer wrote:
> Hi all
>
> Today I ran into an issue where commit timestamp lookups were failing with
>
> ERROR: cannot retrieve commit timestamp for transaction 2
>
> which is of course FrozenTransactionId.
>
> TransactionIdGetCommitTsData(...) ERRORs on !TransactionIdIsNormal(),
> which I think is wrong. Attached is a patch to make it return 0 for
> FrozenTransactionId and BootstrapTransactionId, like it does for xids
> that are too old.
>
> Note that the prior behaviour was as designed and has tests to enforce
> it. I just think it's wrong, and it's also not documented.
>
> IMO this should be back-patched to 9.6 and, without the TAP test part, to 9.5.
Updated to correct the other expected file, since there's an alternate.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From 52081e9aa91102022d6e853d4e2217308bb230a9 Mon Sep 17 00:00:00 2001
From: Craig Ringer
Date: Wed, 23 Nov 2016 19:50:50 +0800
Subject: [PATCH] Treat frozen and bootstrap xids as old, not invalid, for
committs
TransactionIdGetCommitTsData() ERROR'd on FrozenTransactionId and
BootstrapTransactionId, which is a surprising outcome for callers
given that it usually returns 0 for xids too old for their commit
timestamp to still be known.
Callers hitting this problem would get an error like:
ERROR: cannot retrieve commit timestamp for transaction 2
Fix it so the frozen and bootstrap XIDs return 0 like other too-old XIDs.
---
src/backend/access/transam/commit_ts.c | 11 +--
src/test/modules/commit_ts/expected/commit_timestamp.out | 12 ++--
src/test/modules/commit_ts/t/004_restart.pl | 14 --
3 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 7746578..a5b270c 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -289,11 +289,18 @@ TransactionIdGetCommitTsData(TransactionId xid, TimestampTz *ts,
TransactionId oldestCommitTsXid;
TransactionId newestCommitTsXid;
- /* error if the given Xid doesn't normally commit */
- if (!TransactionIdIsNormal(xid))
+ if (!TransactionIdIsValid(xid))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("cannot retrieve commit timestamp for transaction %u", xid)));
+ else if (!TransactionIdIsNormal(xid))
+ {
+ /* frozen and bootstrap xids are always committed far in the past */
+ *ts = 0;
+ if (nodeid)
+ *nodeid = 0;
+ return false;
+ }
LWLockAcquire(CommitTsLock, LW_SHARED);
diff --git a/src/test/modules/commit_ts/expected/commit_timestamp.out b/src/test/modules/commit_ts/expected/commit_timestamp.out
index 99f3322..5b7783b 100644
--- a/src/test/modules/commit_ts/expected/commit_timestamp.out
+++ b/src/test/modules/commit_ts/expected/commit_timestamp.out
@@ -28,9 +28,17 @@ DROP TABLE committs_test;
SELECT pg_xact_commit_timestamp('0'::xid);
ERROR: cannot retrieve commit timestamp for transaction 0
SELECT pg_xact_commit_timestamp('1'::xid);
-ERROR: cannot retrieve commit timestamp for transaction 1
+ pg_xact_commit_timestamp
+--
+
+(1 row)
+
SELECT pg_xact_commit_timestamp('2'::xid);
-ERROR: cannot retrieve commit timestamp for transaction 2
+ pg_xact_commit_timestamp
+--
+
+(1 row)
+
SELECT x.xid::text::bigint > 0, x.timestamp > '-infinity'::timestamptz, x.timestamp <= now() FROM pg_last_committed_xact() x;
?column? | ?column? | ?column?
--+--+--
diff --git a/src/test/modules/commit_ts/t/004_restart.pl b/src/test/modules/commit_ts/t/004_restart.pl
index 900e9b7..32be07c 100644
--- a/src/test/modules/commit_ts/t/004_restart.pl
+++ b/src/test/modules/commit_ts/t/004_restart.pl
@@ -25,19 +25,13 @@ like(
($ret, $stdout, $stderr) =
$node_master->psql('postgres', qq[SELECT pg_xact_commit_timestamp('1');]);
-is($ret, 3, 'getting ts of BootstrapTransactionId reports error');
-like(
- $stderr,
- qr/cannot retrieve commit timestamp for transaction/,
- 'expected error from BootstrapTransactionId');
+is($ret, 0, 'getting ts of BootstrapTransactionId succeeds');
+is($stdout, '', 'timestamp of BootstrapTransactionId is null');
($ret, $stdout, $stderr) =
$node_master->psql('postgres', qq[SELECT pg_xact_commit_timestamp('2');]);
-is($ret, 3, 'getting ts of FrozenTransactionId reports error');
-like(
- $stderr,
- qr/cannot retrieve commit timestamp for transaction/,
- 'expected error from FrozenTransactionId');
+is($ret, 0, 'getting ts of FrozenTransactionId succeeds');
+is($stdout, '', 'timestamp of FrozenTransactionId is null');
# Since FirstNormalTransactionId will've occurred during initdb, long before we
# enabled commit timestamps, it'll be null since we have no cts data for it but
--
2.5.5
--
Sent via pgsql-hackers mailing list (pgsql-hackers@po
[HACKERS] [bugfix] commit timestamps ERROR on lookup of FrozenTransactionId
Hi all
Today I ran into an issue where commit timestamp lookups were failing with
ERROR: cannot retrieve commit timestamp for transaction 2
which is of course FrozenTransactionId.
TransactionIdGetCommitTsData(...) ERRORs on !TransactionIdIsNormal(),
which I think is wrong. Attached is a patch to make it return 0 for
FrozenTransactionId and BootstrapTransactionId, like it does for xids
that are too old.
Note that the prior behaviour was as designed and has tests to enforce
it. I just think it's wrong, and it's also not documented.
IMO this should be back-patched to 9.6 and, without the TAP test part, to 9.5.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From 52081e9aa91102022d6e853d4e2217308bb230a9 Mon Sep 17 00:00:00 2001
From: Craig Ringer
Date: Wed, 23 Nov 2016 19:50:50 +0800
Subject: [PATCH] Treat frozen and bootstrap xids as old, not invalid, for
committs
TransactionIdGetCommitTsData() ERROR'd on FrozenTransactionId and
BootstrapTransactionId, which is a surprising outcome for callers
given that it usually returns 0 for xids too old for their commit
timestamp to still be known.
Callers hitting this problem would get an error like:
ERROR: cannot retrieve commit timestamp for transaction 2
Fix it so the frozen and bootstrap XIDs return 0 like other too-old XIDs.
---
src/backend/access/transam/commit_ts.c | 11 +--
src/test/modules/commit_ts/expected/commit_timestamp.out | 12 ++--
src/test/modules/commit_ts/t/004_restart.pl | 14 --
3 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 7746578..a5b270c 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -289,11 +289,18 @@ TransactionIdGetCommitTsData(TransactionId xid, TimestampTz *ts,
TransactionId oldestCommitTsXid;
TransactionId newestCommitTsXid;
- /* error if the given Xid doesn't normally commit */
- if (!TransactionIdIsNormal(xid))
+ if (!TransactionIdIsValid(xid))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("cannot retrieve commit timestamp for transaction %u", xid)));
+ else if (!TransactionIdIsNormal(xid))
+ {
+ /* frozen and bootstrap xids are always committed far in the past */
+ *ts = 0;
+ if (nodeid)
+ *nodeid = 0;
+ return false;
+ }
LWLockAcquire(CommitTsLock, LW_SHARED);
diff --git a/src/test/modules/commit_ts/expected/commit_timestamp.out b/src/test/modules/commit_ts/expected/commit_timestamp.out
index 99f3322..5b7783b 100644
--- a/src/test/modules/commit_ts/expected/commit_timestamp.out
+++ b/src/test/modules/commit_ts/expected/commit_timestamp.out
@@ -28,9 +28,17 @@ DROP TABLE committs_test;
SELECT pg_xact_commit_timestamp('0'::xid);
ERROR: cannot retrieve commit timestamp for transaction 0
SELECT pg_xact_commit_timestamp('1'::xid);
-ERROR: cannot retrieve commit timestamp for transaction 1
+ pg_xact_commit_timestamp
+--
+
+(1 row)
+
SELECT pg_xact_commit_timestamp('2'::xid);
-ERROR: cannot retrieve commit timestamp for transaction 2
+ pg_xact_commit_timestamp
+--
+
+(1 row)
+
SELECT x.xid::text::bigint > 0, x.timestamp > '-infinity'::timestamptz, x.timestamp <= now() FROM pg_last_committed_xact() x;
?column? | ?column? | ?column?
--+--+--
diff --git a/src/test/modules/commit_ts/t/004_restart.pl b/src/test/modules/commit_ts/t/004_restart.pl
index 900e9b7..32be07c 100644
--- a/src/test/modules/commit_ts/t/004_restart.pl
+++ b/src/test/modules/commit_ts/t/004_restart.pl
@@ -25,19 +25,13 @@ like(
($ret, $stdout, $stderr) =
$node_master->psql('postgres', qq[SELECT pg_xact_commit_timestamp('1');]);
-is($ret, 3, 'getting ts of BootstrapTransactionId reports error');
-like(
- $stderr,
- qr/cannot retrieve commit timestamp for transaction/,
- 'expected error from BootstrapTransactionId');
+is($ret, 0, 'getting ts of BootstrapTransactionId succeeds');
+is($stdout, '', 'timestamp of BootstrapTransactionId is null');
($ret, $stdout, $stderr) =
$node_master->psql('postgres', qq[SELECT pg_xact_commit_timestamp('2');]);
-is($ret, 3, 'getting ts of FrozenTransactionId reports error');
-like(
- $stderr,
- qr/cannot retrieve commit timestamp for transaction/,
- 'expected error from FrozenTransactionId');
+is($ret, 0, 'getting ts of FrozenTransactionId succeeds');
+is($stdout, '', 'timestamp of FrozenTransactionId is null');
# Since FirstNormalTransactionId will've occurred during initdb, long before we
# enabled commit timestamps, it'll be null since we have no cts data for it but
--
2.5.5
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
