Re: [HACKERS] issue with track_commit_timestamp and server restart

[Tom Lane]

Alvaro Herrera  writes:
> Now let's discuss a release note entry for 9.5 and 9.6, shall we?

The text in your commit message seems sufficient from here.
On it now (although if somebody doesn't fix anon git PDQ, we're
not releasing today anyway).

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] issue with track_commit_timestamp and server restart

[Alvaro Herrera]

Now let's discuss a release note entry for 9.5 and 9.6, shall we?

-- 
Á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] issue with track_commit_timestamp and server restart

[Alvaro Herrera]

Craig Ringer wrote:

> I initially could'n't see this when tested on 8f1fb7d with a
> src/test/recovery/t test script. But it turns out it's OK on immediate
> shutdown and crash recovery, but not on clean shutdown and restart.

Oh, oops.

> The attached patch adds a TAP test to src/test/recovery to show this.

I moved it to src/test/modules/commit_ts and pushed.  src/test/recovery
is not currently run by buildfarm, except hamster, unless the bf client
has been patched since I last looked.  But that's not the reason I moved
it, but rather because the other location seemed better.  I also fixed
the wrongly pasted comment at the top of the file while at it.

If this disrupts the buildfarm in any way I will just revert it
immediately.

-- 
Á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] issue with track_commit_timestamp and server restart

[Julien Rouhaud]

On 24/10/2016 06:58, Craig Ringer wrote:
> On 22 October 2016 at 19:51, Julien Rouhaud  wrote:
>> I just noticed that if track_commit_timestamp is enabled, the
>> oldestCommitTsXid and newestCommitTsXid don't persist after a server
>> restart, so you can't ask for the commit ts of a transaction that
>> committed before the last server start, although the information is
>> still available (unless of course if a freeze occured).  AFAICT it
>> always behave this way.
> 
> I initially could'n't see this when tested on 8f1fb7d with a
> src/test/recovery/t test script. But it turns out it's OK on immediate
> shutdown and crash recovery, but not on clean shutdown and restart.
> 
> The attached patch adds a TAP test to src/test/recovery to show this.
> If you run the TAP test before recompiling with the fix it'll fail.
> "make" to apply the fix, then re-run and it'll succeed. Or just
> temporarily roll back the fix with:
> 
> git checkout HEAD^1 -- src/backend/access/transam/commit_ts.c
> git reset src/backend/access/transam/commit_ts.c
> 
> and rebuild to see it fail.
> 
> To save time running the recovery suite, just
> 
>rm src/test/recovery/00[0-8]*.pl
> 
> (It'd be nice to have a prove_check target to run just one test file).
> 
> This would explain a few issues I've seen reported with BDR from the
> community which I've so far been unable to reproduce, so thanks very
> much for the report.
> 
> Álvaro, would you mind checking this and committing to HEAD and 9.6?
> The commits.c change only should also be committed to 9.5, but not the
> TAP test.
> 

Thanks a lot for the review, and adding the tests!


-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org


-- 
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] issue with track_commit_timestamp and server restart

[Craig Ringer]

On 24 October 2016 at 12:58, Craig Ringer  wrote:

> The attached patch adds a TAP test to src/test/recovery to show this.

Added to CF.

https://commitfest.postgresql.org/11/834/

... but IMO this should go in the next bugfix release.

I've also applied nearly the same fix for the same bug in the original
commit timestamp implementation in the BDR Postgres tree.

-- 
 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] issue with track_commit_timestamp and server restart

[Michael Paquier]

(thread hijacking)

On Mon, Oct 24, 2016 at 1:58 PM, Craig Ringer  wrote:
> To save time running the recovery suite, just
>
>rm src/test/recovery/00[0-8]*.pl
>
> (It'd be nice to have a prove_check target to run just one test file).

Agreed! Or multiple chosen files. I usually remove those files
manually from time to time when working on a fix just to run one
dedicated test repeatedly.
-- 
Michael


-- 
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] issue with track_commit_timestamp and server restart

[Craig Ringer]

On 22 October 2016 at 19:51, Julien Rouhaud  wrote:
> I just noticed that if track_commit_timestamp is enabled, the
> oldestCommitTsXid and newestCommitTsXid don't persist after a server
> restart, so you can't ask for the commit ts of a transaction that
> committed before the last server start, although the information is
> still available (unless of course if a freeze occured).  AFAICT it
> always behave this way.

I initially could'n't see this when tested on 8f1fb7d with a
src/test/recovery/t test script. But it turns out it's OK on immediate
shutdown and crash recovery, but not on clean shutdown and restart.

The attached patch adds a TAP test to src/test/recovery to show this.
If you run the TAP test before recompiling with the fix it'll fail.
"make" to apply the fix, then re-run and it'll succeed. Or just
temporarily roll back the fix with:

git checkout HEAD^1 -- src/backend/access/transam/commit_ts.c
git reset src/backend/access/transam/commit_ts.c

and rebuild to see it fail.

To save time running the recovery suite, just

   rm src/test/recovery/00[0-8]*.pl

(It'd be nice to have a prove_check target to run just one test file).

This would explain a few issues I've seen reported with BDR from the
community which I've so far been unable to reproduce, so thanks very
much for the report.

Álvaro, would you mind checking this and committing to HEAD and 9.6?
The commits.c change only should also be committed to 9.5, but not the
TAP test.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From 485ecc6c579073cf71ac88746c53668733bc5cac Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Mon, 24 Oct 2016 12:16:41 +0800
Subject: [PATCH] Preserve access to commit timestamps across restart

An oversight in setting the boundaries of known commit timestamps during
startup caused old commit timestamps to become inaccessible after a server
restart.

Report and bugfix by Julien Rouhaud
Review and commit timestamp tests by Craig Ringer
---
 src/backend/access/transam/commit_ts.c |   2 +
 src/test/recovery/t/009_committs.pl| 103 +
 2 files changed, 105 insertions(+)
 create mode 100644 src/test/recovery/t/009_committs.pl

diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index a8d275f..7746578 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -844,6 +844,8 @@ SetCommitTsLimit(TransactionId oldestXact, TransactionId newestXact)
 	else
 	{
 		Assert(ShmemVariableCache->newestCommitTsXid == InvalidTransactionId);
+		ShmemVariableCache->oldestCommitTsXid = oldestXact;
+		ShmemVariableCache->newestCommitTsXid = newestXact;
 	}
 	LWLockRelease(CommitTsLock);
 }
diff --git a/src/test/recovery/t/009_committs.pl b/src/test/recovery/t/009_committs.pl
new file mode 100644
index 000..48f8899
--- /dev/null
+++ b/src/test/recovery/t/009_committs.pl
@@ -0,0 +1,103 @@
+# Testing of logical decoding using SQL interface and/or pg_recvlogical
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 16;
+
+my $node_master = get_new_node('master');
+$node_master->init(allows_streaming => 1);
+$node_master->append_conf(
+		'postgresql.conf', qq(
+track_commit_timestamp = on
+));
+$node_master->start;
+
+my ($ret, $stdout, $stderr);
+
+($ret, $stdout, $stderr) = $node_master->psql('postgres', qq[SELECT pg_xact_commit_timestamp('0');]);
+is($ret, 3, 'getting ts of InvalidTransactionId reports error');
+like($stderr, qr/cannot retrieve commit timestamp for transaction/, 'expected error from InvalidTransactionId');
+
+($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');
+
+($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');
+
+# 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
+# cts are enabled.
+is($node_master->safe_psql('postgres', qq[SELECT pg_xact_commit_timestamp('3');]), '',
+	'committs for FirstNormalTransactionId is null');
+
+$node_master->safe_psql('postgres', qq[CREATE TABLE committs_test(x integer, y timestamp with time zone);]);
+
+my $xid = $node_master->safe_psql('postgres', qq[
+	BEGIN;
+	INSERT INTO committs_test(x, y) VALUES (1, current_timestamp);
+	SELECT txid_current();
+	COMMIT;
+]);
+
+my $before_restart_ts = $node_master->safe_psql('postgres', qq[SELECT