track_commit_timestamp and COMMIT PREPARED
Hi,
track_commit_timestamp tracks COMMIT PREPARED as expected in standby server,
but not in master server. Is this intentional? It should track COMMIT PREPARED
even in master? Otherwise, we cannot use commit_timestamp feature to check
the replication lag properly while we use 2PC.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
track_commit_timestamp tracks COMMIT PREPARED as expected in standby server,
but not in master server. Is this intentional? It should track COMMIT PREPARED
even in master? Otherwise, we cannot use commit_timestamp feature to check
the replication lag properly while we use 2PC.
That sounds like it must be a bug. I think you should add it to the
open items list.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-08-04 13:16:52 -0400, Robert Haas wrote:
On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
track_commit_timestamp tracks COMMIT PREPARED as expected in standby server,
but not in master server. Is this intentional? It should track COMMIT PREPARED
even in master? Otherwise, we cannot use commit_timestamp feature to check
the replication lag properly while we use 2PC.That sounds like it must be a bug. I think you should add it to the
open items list.
Yea, completely agreed. It's probably because twophase.c duplicates a
good bit of xact.c. How about we also add a warning to the respective
places that one should always check the others?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 4, 2015 at 1:18 PM, Andres Freund <andres@anarazel.de> wrote:
On 2015-08-04 13:16:52 -0400, Robert Haas wrote:
On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
track_commit_timestamp tracks COMMIT PREPARED as expected in standby server,
but not in master server. Is this intentional? It should track COMMIT PREPARED
even in master? Otherwise, we cannot use commit_timestamp feature to check
the replication lag properly while we use 2PC.That sounds like it must be a bug. I think you should add it to the
open items list.Yea, completely agreed. It's probably because twophase.c duplicates a
good bit of xact.c. How about we also add a warning to the respective
places that one should always check the others?
Sounds like a good idea. Or we could try to, heh heh, refactor away
the duplication.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
track_commit_timestamp tracks COMMIT PREPARED as expected in standby server,
but not in master server. Is this intentional? It should track COMMIT PREPARED
even in master? Otherwise, we cannot use commit_timestamp feature to check
the replication lag properly while we use 2PC.That sounds like it must be a bug. I think you should add it to the
open items list.
Yep, added.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-09-02 16:14, Fujii Masao wrote:
On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
track_commit_timestamp tracks COMMIT PREPARED as expected in standby server,
but not in master server. Is this intentional? It should track COMMIT PREPARED
even in master? Otherwise, we cannot use commit_timestamp feature to check
the replication lag properly while we use 2PC.That sounds like it must be a bug. I think you should add it to the
open items list.
Attached fixes this. It includes advancement of replication origin as
well. I didn't feel like doing refactor of commit code this late in 9.5
cycle though, so I went with the code duplication + note in xact.c.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
committs-2pcfix.difftext/x-diff; name=committs-2pcfix.diffDownload
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index d48d101..95da328 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -41,6 +41,7 @@
#include <time.h>
#include <unistd.h>
+#include "access/commit_ts.h"
#include "access/htup_details.h"
#include "access/subtrans.h"
#include "access/transam.h"
@@ -56,6 +57,7 @@
#include "miscadmin.h"
#include "pg_trace.h"
#include "pgstat.h"
+#include "replication/origin.h"
#include "replication/walsender.h"
#include "replication/syncrep.h"
#include "storage/fd.h"
@@ -2087,6 +2089,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
bool initfileinval)
{
XLogRecPtr recptr;
+ TimestampTz committs = GetCurrentTimestamp();
START_CRIT_SECTION();
@@ -2094,13 +2097,34 @@ RecordTransactionCommitPrepared(TransactionId xid,
MyPgXact->delayChkpt = true;
/* Emit the XLOG commit record */
- recptr = XactLogCommitRecord(GetCurrentTimestamp(),
+ recptr = XactLogCommitRecord(committs,
nchildren, children, nrels, rels,
ninvalmsgs, invalmsgs,
initfileinval, false,
xid);
/*
+ * Record plain commit ts if not replaying remote actions, or if no
+ * timestamp is configured.
+ */
+ if (replorigin_sesssion_origin == InvalidRepOriginId ||
+ replorigin_sesssion_origin == DoNotReplicateId ||
+ replorigin_sesssion_origin_timestamp == 0)
+ replorigin_sesssion_origin_timestamp = committs;
+ else
+ replorigin_session_advance(replorigin_sesssion_origin_lsn,
+ XactLastRecEnd);
+
+ /*
+ * We don't need to WAL log origin or timestamp here, the commit
+ * record contains all the necessary information and will redo the SET
+ * action during replay.
+ */
+ TransactionTreeSetCommitTsData(xid, nchildren, children,
+ replorigin_sesssion_origin_timestamp,
+ replorigin_sesssion_origin, false);
+
+ /*
* We don't currently try to sleep before flush here ... nor is there any
* support for async commit of a prepared xact (the very idea is probably
* a contradiction)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index b53d95f..6ef876d 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1119,6 +1119,9 @@ AtSubStart_ResourceOwner(void)
*
* Returns latest XID among xact and its children, or InvalidTransactionId
* if the xact has no XID. (We compute that here just because it's easier.)
+ *
+ * Note: if you change this functions you should also look at
+ * RecordTransactionCommitPrepared in twophase.c.
*/
static TransactionId
RecordTransactionCommit(void)
On Sat, Sep 5, 2015 at 7:48 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:
On 2015-09-02 16:14, Fujii Masao wrote:
On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao <masao.fujii@gmail.com>
wrote:track_commit_timestamp tracks COMMIT PREPARED as expected in standby
server,
but not in master server. Is this intentional? It should track COMMIT
PREPARED
even in master? Otherwise, we cannot use commit_timestamp feature to
check
the replication lag properly while we use 2PC.That sounds like it must be a bug. I think you should add it to the
open items list.Attached fixes this. It includes advancement of replication origin as well.
I didn't feel like doing refactor of commit code this late in 9.5 cycle
though, so I went with the code duplication + note in xact.c.
Agreed. We can refactor the code later if needed.
The patch looks good to me except the following minor points.
* or not. Normal path through RecordTransactionCommit() will be related
* to a transaction commit XLog record, and so should pass "false" here.
The above source comment of TransactionTreeSetCommitTsData() seems to
need to be updated.
+ * Note: if you change this functions you should also look at
+ * RecordTransactionCommitPrepared in twophase.c.Typo: "this functions" should be "this function"
+ if (replorigin_sesssion_origin == InvalidRepOriginId ||
This is not the problem of the patch, but I started wondering what
"sesssion" in the variable name "replorigin_sesssion_origin" means.
Is it just a typo and it should be "session"? Or it's the abbreviation
of something?
Regards,
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Sep 18, 2015 at 12:53 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Sat, Sep 5, 2015 at 7:48 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:
On 2015-09-02 16:14, Fujii Masao wrote:
On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao <masao.fujii@gmail.com>
wrote:track_commit_timestamp tracks COMMIT PREPARED as expected in standby
server,
but not in master server. Is this intentional? It should track COMMIT
PREPARED
even in master? Otherwise, we cannot use commit_timestamp feature to
check
the replication lag properly while we use 2PC.That sounds like it must be a bug. I think you should add it to the
open items list.Attached fixes this. It includes advancement of replication origin as well.
I didn't feel like doing refactor of commit code this late in 9.5 cycle
though, so I went with the code duplication + note in xact.c.Agreed. We can refactor the code later if needed.
The patch looks good to me except the following minor points.
* or not. Normal path through RecordTransactionCommit() will be related
* to a transaction commit XLog record, and so should pass "false" here.The above source comment of TransactionTreeSetCommitTsData() seems to
need to be updated.+ * Note: if you change this functions you should also look at + * RecordTransactionCommitPrepared in twophase.c.Typo: "this functions" should be "this function"
+ if (replorigin_sesssion_origin == InvalidRepOriginId ||
This is not the problem of the patch, but I started wondering what
"sesssion" in the variable name "replorigin_sesssion_origin" means.
Is it just a typo and it should be "session"? Or it's the abbreviation
of something?
This should go in before beta. Is someone updating the patch?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-09-28 18:59, Robert Haas wrote:
The patch looks good to me except the following minor points.
* or not. Normal path through RecordTransactionCommit() will be related
* to a transaction commit XLog record, and so should pass "false" here.The above source comment of TransactionTreeSetCommitTsData() seems to
need to be updated.+ * Note: if you change this functions you should also look at + * RecordTransactionCommitPrepared in twophase.c.Typo: "this functions" should be "this function"
+ if (replorigin_sesssion_origin == InvalidRepOriginId ||
This is not the problem of the patch, but I started wondering what
"sesssion" in the variable name "replorigin_sesssion_origin" means.
Is it just a typo and it should be "session"? Or it's the abbreviation
of something?This should go in before beta. Is someone updating the patch?
Sorry, missed your reply.
The "sesssion" is typo and it actually affects several variables around
the replication origin, so I attached separate patch (which should be
applied first) which fixes the typo everywhere.
I reworded the comment, hopefully it's better this way.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
0002-commit_ts-2pcfix.patchtext/x-diff; name=0002-commit_ts-2pcfix.patchDownload
>From e8c68da28dd3f6b785aa23d4cf3c2973d6bca6c5 Mon Sep 17 00:00:00 2001
From: Petr Jelinek <pjmodos@pjmodos.net>
Date: Mon, 28 Sep 2015 19:43:19 +0200
Subject: [PATCH 2/2] commit_ts-2pcfix
---
src/backend/access/transam/commit_ts.c | 9 +++++----
src/backend/access/transam/twophase.c | 26 +++++++++++++++++++++++++-
src/backend/access/transam/xact.c | 3 +++
3 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 33136e3..cddde30 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -123,10 +123,11 @@ static void WriteSetTimestampXlogRec(TransactionId mainxid, int nsubxids,
* decision of storing timestamp info for each subxid.
*
* The do_xlog parameter tells us whether to include an XLog record of this
- * or not. Normal path through RecordTransactionCommit() will be related
- * to a transaction commit XLog record, and so should pass "false" here.
- * Other callers probably want to pass true, so that the given values persist
- * in case of crashes.
+ * or not. Normally, this is called from transaction (both normal and
+ * prepared) commit code and the information will be stored in the transaction
+ * commit XLog record, and so it should pass "false" here. The XLog redo code
+ * should use "false" here as well. Other callers probably want to pass true,
+ * so that the given values persist in case of crashes.
*/
void
TransactionTreeSetCommitTsData(TransactionId xid, int nsubxids,
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index d48d101..6e7ede9 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -41,6 +41,7 @@
#include <time.h>
#include <unistd.h>
+#include "access/commit_ts.h"
#include "access/htup_details.h"
#include "access/subtrans.h"
#include "access/transam.h"
@@ -56,6 +57,7 @@
#include "miscadmin.h"
#include "pg_trace.h"
#include "pgstat.h"
+#include "replication/origin.h"
#include "replication/walsender.h"
#include "replication/syncrep.h"
#include "storage/fd.h"
@@ -2087,6 +2089,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
bool initfileinval)
{
XLogRecPtr recptr;
+ TimestampTz committs = GetCurrentTimestamp();
START_CRIT_SECTION();
@@ -2094,13 +2097,34 @@ RecordTransactionCommitPrepared(TransactionId xid,
MyPgXact->delayChkpt = true;
/* Emit the XLOG commit record */
- recptr = XactLogCommitRecord(GetCurrentTimestamp(),
+ recptr = XactLogCommitRecord(committs,
nchildren, children, nrels, rels,
ninvalmsgs, invalmsgs,
initfileinval, false,
xid);
/*
+ * Record plain commit ts if not replaying remote actions, or if no
+ * timestamp is configured.
+ */
+ if (replorigin_session_origin == InvalidRepOriginId ||
+ replorigin_session_origin == DoNotReplicateId ||
+ replorigin_session_origin_timestamp == 0)
+ replorigin_session_origin_timestamp = committs;
+ else
+ replorigin_session_advance(replorigin_session_origin_lsn,
+ XactLastRecEnd);
+
+ /*
+ * We don't need to WAL log origin or timestamp here, the commit
+ * record contains all the necessary information and will redo the SET
+ * action during replay.
+ */
+ TransactionTreeSetCommitTsData(xid, nchildren, children,
+ replorigin_session_origin_timestamp,
+ replorigin_session_origin, false);
+
+ /*
* We don't currently try to sleep before flush here ... nor is there any
* support for async commit of a prepared xact (the very idea is probably
* a contradiction)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 068214d..0238771 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1119,6 +1119,9 @@ AtSubStart_ResourceOwner(void)
*
* Returns latest XID among xact and its children, or InvalidTransactionId
* if the xact has no XID. (We compute that here just because it's easier.)
+ *
+ * Note: if you change this function you should also look at
+ * RecordTransactionCommitPrepared in twophase.c.
*/
static TransactionId
RecordTransactionCommit(void)
--
1.9.1
0001-replication-origin-typo-fixes.patchtext/x-diff; name=0001-replication-origin-typo-fixes.patchDownload
>From 682a6040fd49027e51c2477adb98d3132073ddef Mon Sep 17 00:00:00 2001
From: Petr Jelinek <pjmodos@pjmodos.net>
Date: Mon, 28 Sep 2015 19:46:23 +0200
Subject: [PATCH 1/2] Replication origin typo fixes (sesssion -> session)
---
src/backend/access/transam/xact.c | 20 ++++++++++----------
src/backend/access/transam/xloginsert.c | 6 +++---
src/backend/replication/logical/origin.c | 26 +++++++++++++-------------
src/include/replication/origin.h | 6 +++---
4 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 56a1cb4..068214d 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1210,12 +1210,12 @@ RecordTransactionCommit(void)
* Record plain commit ts if not replaying remote actions, or if no
* timestamp is configured.
*/
- if (replorigin_sesssion_origin == InvalidRepOriginId ||
- replorigin_sesssion_origin == DoNotReplicateId ||
- replorigin_sesssion_origin_timestamp == 0)
- replorigin_sesssion_origin_timestamp = xactStopTimestamp;
+ if (replorigin_session_origin == InvalidRepOriginId ||
+ replorigin_session_origin == DoNotReplicateId ||
+ replorigin_session_origin_timestamp == 0)
+ replorigin_session_origin_timestamp = xactStopTimestamp;
else
- replorigin_session_advance(replorigin_sesssion_origin_lsn,
+ replorigin_session_advance(replorigin_session_origin_lsn,
XactLastRecEnd);
/*
@@ -1224,8 +1224,8 @@ RecordTransactionCommit(void)
* action during replay.
*/
TransactionTreeSetCommitTsData(xid, nchildren, children,
- replorigin_sesssion_origin_timestamp,
- replorigin_sesssion_origin, false);
+ replorigin_session_origin_timestamp,
+ replorigin_session_origin, false);
}
/*
@@ -5134,12 +5134,12 @@ XactLogCommitRecord(TimestampTz commit_time,
}
/* dump transaction origin information */
- if (replorigin_sesssion_origin != InvalidRepOriginId)
+ if (replorigin_session_origin != InvalidRepOriginId)
{
xl_xinfo.xinfo |= XACT_XINFO_HAS_ORIGIN;
- xl_origin.origin_lsn = replorigin_sesssion_origin_lsn;
- xl_origin.origin_timestamp = replorigin_sesssion_origin_timestamp;
+ xl_origin.origin_lsn = replorigin_session_origin_lsn;
+ xl_origin.origin_timestamp = replorigin_session_origin_timestamp;
}
if (xl_xinfo.xinfo != 0)
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index abd8420..925255f 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -701,11 +701,11 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
}
/* followed by the record's origin, if any */
- if (include_origin && replorigin_sesssion_origin != InvalidRepOriginId)
+ if (include_origin && replorigin_session_origin != InvalidRepOriginId)
{
*(scratch++) = XLR_BLOCK_ID_ORIGIN;
- memcpy(scratch, &replorigin_sesssion_origin, sizeof(replorigin_sesssion_origin));
- scratch += sizeof(replorigin_sesssion_origin);
+ memcpy(scratch, &replorigin_session_origin, sizeof(replorigin_session_origin));
+ scratch += sizeof(replorigin_session_origin);
}
/* followed by main data, if any */
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index b5fa3d8..f55641b 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -148,9 +148,9 @@ typedef struct ReplicationStateCtl
} ReplicationStateCtl;
/* external variables */
-RepOriginId replorigin_sesssion_origin = InvalidRepOriginId; /* assumed identity */
-XLogRecPtr replorigin_sesssion_origin_lsn = InvalidXLogRecPtr;
-TimestampTz replorigin_sesssion_origin_timestamp = 0;
+RepOriginId replorigin_session_origin = InvalidRepOriginId; /* assumed identity */
+XLogRecPtr replorigin_session_origin_lsn = InvalidXLogRecPtr;
+TimestampTz replorigin_session_origin_timestamp = 0;
/*
* Base address into a shared memory array of replication states of size
@@ -803,7 +803,7 @@ replorigin_redo(XLogReaderState *record)
* Tell the replication origin progress machinery that a commit from 'node'
* that originated at the LSN remote_commit on the remote node was replayed
* successfully and that we don't need to do so again. In combination with
- * setting up replorigin_sesssion_origin_lsn and replorigin_sesssion_origin
+ * setting up replorigin_session_origin_lsn and replorigin_session_origin
* that ensures we won't loose knowledge about that after a crash if the
* transaction had a persistent effect (think of asynchronous commits).
*
@@ -1236,7 +1236,7 @@ pg_replication_origin_session_setup(PG_FUNCTION_ARGS)
origin = replorigin_by_name(name, false);
replorigin_session_setup(origin);
- replorigin_sesssion_origin = origin;
+ replorigin_session_origin = origin;
pfree(name);
@@ -1253,9 +1253,9 @@ pg_replication_origin_session_reset(PG_FUNCTION_ARGS)
replorigin_session_reset();
- replorigin_sesssion_origin = InvalidRepOriginId;
- replorigin_sesssion_origin_lsn = InvalidXLogRecPtr;
- replorigin_sesssion_origin_timestamp = 0;
+ replorigin_session_origin = InvalidRepOriginId;
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;
PG_RETURN_VOID();
}
@@ -1268,7 +1268,7 @@ pg_replication_origin_session_is_setup(PG_FUNCTION_ARGS)
{
replorigin_check_prerequisites(false, false);
- PG_RETURN_BOOL(replorigin_sesssion_origin != InvalidRepOriginId);
+ PG_RETURN_BOOL(replorigin_session_origin != InvalidRepOriginId);
}
@@ -1312,8 +1312,8 @@ pg_replication_origin_xact_setup(PG_FUNCTION_ARGS)
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("no replication origin is configured")));
- replorigin_sesssion_origin_lsn = location;
- replorigin_sesssion_origin_timestamp = PG_GETARG_TIMESTAMPTZ(1);
+ replorigin_session_origin_lsn = location;
+ replorigin_session_origin_timestamp = PG_GETARG_TIMESTAMPTZ(1);
PG_RETURN_VOID();
}
@@ -1323,8 +1323,8 @@ pg_replication_origin_xact_reset(PG_FUNCTION_ARGS)
{
replorigin_check_prerequisites(true, false);
- replorigin_sesssion_origin_lsn = InvalidXLogRecPtr;
- replorigin_sesssion_origin_timestamp = 0;
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;
PG_RETURN_VOID();
}
diff --git a/src/include/replication/origin.h b/src/include/replication/origin.h
index 8cec434..daa3b93 100644
--- a/src/include/replication/origin.h
+++ b/src/include/replication/origin.h
@@ -34,9 +34,9 @@ typedef struct xl_replorigin_drop
#define InvalidRepOriginId 0
#define DoNotReplicateId PG_UINT16_MAX
-extern PGDLLIMPORT RepOriginId replorigin_sesssion_origin;
-extern PGDLLIMPORT XLogRecPtr replorigin_sesssion_origin_lsn;
-extern PGDLLIMPORT TimestampTz replorigin_sesssion_origin_timestamp;
+extern PGDLLIMPORT RepOriginId replorigin_session_origin;
+extern PGDLLIMPORT XLogRecPtr replorigin_session_origin_lsn;
+extern PGDLLIMPORT TimestampTz replorigin_session_origin_timestamp;
/* API for querying & manipulating replication origins */
extern RepOriginId replorigin_by_name(char *name, bool missing_ok);
--
1.9.1
On Mon, Sep 28, 2015 at 2:07 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:
Sorry, missed your reply.
To be clear, that was actually Fujii Masao's reply, not mine. I hope
he can have a look at this version.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Fujii Masao wrote:
+ if (replorigin_sesssion_origin == InvalidRepOriginId ||
This is not the problem of the patch, but I started wondering what
"sesssion" in the variable name "replorigin_sesssion_origin" means.
Is it just a typo and it should be "session"? Or it's the abbreviation
of something?
Just a typo; I just pushed a fix.
--
�lvaro Herrera http://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
Petr Jelinek wrote:
On 2015-09-02 16:14, Fujii Masao wrote:
On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
track_commit_timestamp tracks COMMIT PREPARED as expected in standby server,
but not in master server. Is this intentional? It should track COMMIT PREPARED
even in master? Otherwise, we cannot use commit_timestamp feature to check
the replication lag properly while we use 2PC.That sounds like it must be a bug. I think you should add it to the
open items list.Attached fixes this. It includes advancement of replication origin as well.
I didn't feel like doing refactor of commit code this late in 9.5 cycle
though, so I went with the code duplication + note in xact.c.
Thanks, your proposed behavior looks reasonable. I didn't like the
existing coding nor the fact that with your patch we'd have two copies
of it, so I changed a bit instead to be more understandable. Hopefully I
didn't break too many things. This patch includes the patch for the
other commitTS open item too.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
committs.patchtext/x-diff; charset=us-asciiDownload
*** a/src/backend/access/transam/commit_ts.c
--- b/src/backend/access/transam/commit_ts.c
***************
*** 122,150 **** static void WriteSetTimestampXlogRec(TransactionId mainxid, int nsubxids,
* subtrans implementation changes in the future, we might want to revisit the
* decision of storing timestamp info for each subxid.
*
! * The do_xlog parameter tells us whether to include an XLog record of this
! * or not. Normal path through RecordTransactionCommit() will be related
! * to a transaction commit XLog record, and so should pass "false" here.
! * Other callers probably want to pass true, so that the given values persist
! * in case of crashes.
*/
void
TransactionTreeSetCommitTsData(TransactionId xid, int nsubxids,
TransactionId *subxids, TimestampTz timestamp,
! RepOriginId nodeid, bool do_xlog)
{
int i;
TransactionId headxid;
TransactionId newestXact;
! if (!track_commit_timestamp)
return;
/*
* Comply with the WAL-before-data rule: if caller specified it wants this
* value to be recorded in WAL, do so before touching the data.
*/
! if (do_xlog)
WriteSetTimestampXlogRec(xid, nsubxids, subxids, timestamp, nodeid);
/*
--- 122,160 ----
* subtrans implementation changes in the future, we might want to revisit the
* decision of storing timestamp info for each subxid.
*
! * The replaying_xlog parameter indicates whether the module should execute
! * its write even if the feature is nominally disabled, because we're replaying
! * a record generated from a master where the feature is enabled.
! *
! * The write_xlog parameter tells us whether to include an XLog record of this
! * or not. Normally, this is called from transaction commit routines (both
! * normal and prepared) and the information will be stored in the transaction
! * commit XLog record, and so they should pass "false" for this. The XLog redo
! * code should use "false" here as well. Other callers probably want to pass
! * true, so that the given values persist in case of crashes.
*/
void
TransactionTreeSetCommitTsData(TransactionId xid, int nsubxids,
TransactionId *subxids, TimestampTz timestamp,
! RepOriginId nodeid,
! bool replaying_xlog, bool write_xlog)
{
int i;
TransactionId headxid;
TransactionId newestXact;
! /* We'd better not try to write xlog during replay */
! Assert(!(write_xlog && replaying_xlog));
!
! /* No-op if feature not enabled, unless replaying WAL */
! if (!track_commit_timestamp && !replaying_xlog)
return;
/*
* Comply with the WAL-before-data rule: if caller specified it wants this
* value to be recorded in WAL, do so before touching the data.
*/
! if (write_xlog)
WriteSetTimestampXlogRec(xid, nsubxids, subxids, timestamp, nodeid);
/*
***************
*** 906,912 **** commit_ts_redo(XLogReaderState *record)
subxids = NULL;
TransactionTreeSetCommitTsData(setts->mainxid, nsubxids, subxids,
! setts->timestamp, setts->nodeid, false);
if (subxids)
pfree(subxids);
}
--- 916,923 ----
subxids = NULL;
TransactionTreeSetCommitTsData(setts->mainxid, nsubxids, subxids,
! setts->timestamp, setts->nodeid, false,
! true);
if (subxids)
pfree(subxids);
}
*** a/src/backend/access/transam/twophase.c
--- b/src/backend/access/transam/twophase.c
***************
*** 41,46 ****
--- 41,47 ----
#include <time.h>
#include <unistd.h>
+ #include "access/commit_ts.h"
#include "access/htup_details.h"
#include "access/subtrans.h"
#include "access/transam.h"
***************
*** 56,63 ****
#include "miscadmin.h"
#include "pg_trace.h"
#include "pgstat.h"
! #include "replication/walsender.h"
#include "replication/syncrep.h"
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/predicate.h"
--- 57,65 ----
#include "miscadmin.h"
#include "pg_trace.h"
#include "pgstat.h"
! #include "replication/origin.h"
#include "replication/syncrep.h"
+ #include "replication/walsender.h"
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/predicate.h"
***************
*** 2070,2077 **** RecoverPreparedTransactions(void)
/*
* RecordTransactionCommitPrepared
*
! * This is basically the same as RecordTransactionCommit: in particular,
! * we must set the delayChkpt flag to avoid a race condition.
*
* We know the transaction made at least one XLOG entry (its PREPARE),
* so it is never possible to optimize out the commit record.
--- 2072,2080 ----
/*
* RecordTransactionCommitPrepared
*
! * This is basically the same as RecordTransactionCommit (q.v. if you change
! * this function): in particular, we must set the delayChkpt flag to avoid a
! * race condition.
*
* We know the transaction made at least one XLOG entry (its PREPARE),
* so it is never possible to optimize out the commit record.
***************
*** 2087,2092 **** RecordTransactionCommitPrepared(TransactionId xid,
--- 2090,2101 ----
bool initfileinval)
{
XLogRecPtr recptr;
+ TimestampTz committs = GetCurrentTimestamp();
+ bool replorigin;
+
+ /* are we using the replication origins feature? */
+ replorigin = (replorigin_session_origin != InvalidRepOriginId &&
+ replorigin_session_origin != DoNotReplicateId);
START_CRIT_SECTION();
***************
*** 2094,2105 **** RecordTransactionCommitPrepared(TransactionId xid,
MyPgXact->delayChkpt = true;
/* Emit the XLOG commit record */
! recptr = XactLogCommitRecord(GetCurrentTimestamp(),
nchildren, children, nrels, rels,
ninvalmsgs, invalmsgs,
initfileinval, false,
xid);
/*
* We don't currently try to sleep before flush here ... nor is there any
* support for async commit of a prepared xact (the very idea is probably
--- 2103,2135 ----
MyPgXact->delayChkpt = true;
/* Emit the XLOG commit record */
! recptr = XactLogCommitRecord(committs,
nchildren, children, nrels, rels,
ninvalmsgs, invalmsgs,
initfileinval, false,
xid);
+
+ if (replorigin)
+ /* Tell replorigin that this session is moving forward */
+ replorigin_session_advance(replorigin_session_origin_lsn,
+ XactLastRecEnd);
+
+ /*
+ * Record commit timestamp. The value comes from plain commit timestamp if
+ * replorigin is not enabled, or replorigin already set a value for us in
+ * replorigin_session_origin_timestamp otherwise.
+ *
+ * We don't need to WAL-log anything here, as the commit record written
+ * above already contains the data.
+ */
+ if (!replorigin || replorigin_session_origin_timestamp == 0)
+ replorigin_session_origin_timestamp = committs;
+
+ TransactionTreeSetCommitTsData(xid, nchildren, children,
+ replorigin_session_origin_timestamp,
+ replorigin_session_origin, false, false);
+
/*
* We don't currently try to sleep before flush here ... nor is there any
* support for async commit of a prepared xact (the very idea is probably
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
***************
*** 42,50 ****
#include "miscadmin.h"
#include "pgstat.h"
#include "replication/logical.h"
- #include "replication/walsender.h"
- #include "replication/syncrep.h"
#include "replication/origin.h"
#include "storage/fd.h"
#include "storage/lmgr.h"
#include "storage/predicate.h"
--- 42,50 ----
#include "miscadmin.h"
#include "pgstat.h"
#include "replication/logical.h"
#include "replication/origin.h"
+ #include "replication/syncrep.h"
+ #include "replication/walsender.h"
#include "storage/fd.h"
#include "storage/lmgr.h"
#include "storage/predicate.h"
***************
*** 1119,1124 **** AtSubStart_ResourceOwner(void)
--- 1119,1126 ----
*
* Returns latest XID among xact and its children, or InvalidTransactionId
* if the xact has no XID. (We compute that here just because it's easier.)
+ *
+ * If you change this function, see RecordTransactionCommitPrepared also.
*/
static TransactionId
RecordTransactionCommit(void)
***************
*** 1172,1177 **** RecordTransactionCommit(void)
--- 1174,1188 ----
}
else
{
+ bool replorigin;
+
+ /*
+ * are we using the replication origins feature? Or, in other words,
+ * are we replaying remote actions?
+ */
+ replorigin = (replorigin_session_origin != InvalidRepOriginId &&
+ replorigin_session_origin != DoNotReplicateId);
+
/*
* Begin commit critical section and insert the commit XLOG record.
*/
***************
*** 1206,1231 **** RecordTransactionCommit(void)
RelcacheInitFileInval, forceSyncCommit,
InvalidTransactionId /* plain commit */ );
! /*
! * Record plain commit ts if not replaying remote actions, or if no
! * timestamp is configured.
! */
! if (replorigin_session_origin == InvalidRepOriginId ||
! replorigin_session_origin == DoNotReplicateId ||
! replorigin_session_origin_timestamp == 0)
! replorigin_session_origin_timestamp = xactStopTimestamp;
! else
replorigin_session_advance(replorigin_session_origin_lsn,
XactLastRecEnd);
/*
! * We don't need to WAL log origin or timestamp here, the commit
! * record contains all the necessary information and will redo the SET
! * action during replay.
*/
TransactionTreeSetCommitTsData(xid, nchildren, children,
replorigin_session_origin_timestamp,
! replorigin_session_origin, false);
}
/*
--- 1217,1243 ----
RelcacheInitFileInval, forceSyncCommit,
InvalidTransactionId /* plain commit */ );
! if (replorigin)
! /* Tell replorigin that this session is moving forward */
replorigin_session_advance(replorigin_session_origin_lsn,
XactLastRecEnd);
/*
! * Record commit timestamp. The value comes from plain commit
! * timestamp if replorigin is not enabled, or replorigin already set a
! * value for us in replorigin_session_origin_timestamp otherwise.
! *
! * We don't need to WAL-log anything here, as the commit record written
! * above already contains the data.
*/
+
+ if (!replorigin || replorigin_session_origin_timestamp == 0)
+ replorigin_session_origin_timestamp = xactStopTimestamp;
+
TransactionTreeSetCommitTsData(xid, nchildren, children,
replorigin_session_origin_timestamp,
! replorigin_session_origin,
! false, false);
}
/*
***************
*** 5321,5327 **** xact_redo_commit(xl_xact_parsed_commit *parsed,
/* Set the transaction commit timestamp and metadata */
TransactionTreeSetCommitTsData(xid, parsed->nsubxacts, parsed->subxacts,
commit_time, origin_id,
! false);
if (standbyState == STANDBY_DISABLED)
{
--- 5333,5339 ----
/* Set the transaction commit timestamp and metadata */
TransactionTreeSetCommitTsData(xid, parsed->nsubxacts, parsed->subxacts,
commit_time, origin_id,
! false, true);
if (standbyState == STANDBY_DISABLED)
{
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***************
*** 5826,5844 **** do { \
minValue))); \
} while(0)
- #define RecoveryRequiresBoolParameter(param_name, currValue, masterValue) \
- do { \
- bool _currValue = (currValue); \
- bool _masterValue = (masterValue); \
- if (_currValue != _masterValue) \
- ereport(ERROR, \
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE), \
- errmsg("hot standby is not possible because it requires \"%s\" to be same on master and standby (master has \"%s\", standby has \"%s\")", \
- param_name, \
- _masterValue ? "true" : "false", \
- _currValue ? "true" : "false"))); \
- } while(0)
-
/*
* Check to see if required parameters are set high enough on this server
* for various aspects of recovery operation.
--- 5826,5831 ----
*** a/src/include/access/commit_ts.h
--- b/src/include/access/commit_ts.h
***************
*** 24,30 **** extern bool check_track_commit_timestamp(bool *newval, void **extra,
extern void TransactionTreeSetCommitTsData(TransactionId xid, int nsubxids,
TransactionId *subxids, TimestampTz timestamp,
! RepOriginId nodeid, bool do_xlog);
extern bool TransactionIdGetCommitTsData(TransactionId xid,
TimestampTz *ts, RepOriginId *nodeid);
extern TransactionId GetLatestCommitTsData(TimestampTz *ts,
--- 24,31 ----
extern void TransactionTreeSetCommitTsData(TransactionId xid, int nsubxids,
TransactionId *subxids, TimestampTz timestamp,
! RepOriginId nodeid,
! bool replaying_xlog, bool write_xlog);
extern bool TransactionIdGetCommitTsData(TransactionId xid,
TimestampTz *ts, RepOriginId *nodeid);
extern TransactionId GetLatestCommitTsData(TimestampTz *ts,
On 2015-09-29 05:05, Alvaro Herrera wrote:
Petr Jelinek wrote:
On 2015-09-02 16:14, Fujii Masao wrote:
On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
track_commit_timestamp tracks COMMIT PREPARED as expected in standby server,
but not in master server. Is this intentional? It should track COMMIT PREPARED
even in master? Otherwise, we cannot use commit_timestamp feature to check
the replication lag properly while we use 2PC.That sounds like it must be a bug. I think you should add it to the
open items list.Attached fixes this. It includes advancement of replication origin as well.
I didn't feel like doing refactor of commit code this late in 9.5 cycle
though, so I went with the code duplication + note in xact.c.Thanks, your proposed behavior looks reasonable. I didn't like the
existing coding nor the fact that with your patch we'd have two copies
of it, so I changed a bit instead to be more understandable. Hopefully I
didn't break too many things. This patch includes the patch for the
other commitTS open item too.
Looks good. It does change the logic slightly - previous code didn't
advance session origin lsn if origin timestamp wasn't set, your code
does, but I think the new behavior is better.
--
Petr Jelinek 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
On Tue, Sep 29, 2015 at 12:05 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Petr Jelinek wrote:
On 2015-09-02 16:14, Fujii Masao wrote:
On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
track_commit_timestamp tracks COMMIT PREPARED as expected in standby server,
but not in master server. Is this intentional? It should track COMMIT PREPARED
even in master? Otherwise, we cannot use commit_timestamp feature to check
the replication lag properly while we use 2PC.That sounds like it must be a bug. I think you should add it to the
open items list.Attached fixes this. It includes advancement of replication origin as well.
I didn't feel like doing refactor of commit code this late in 9.5 cycle
though, so I went with the code duplication + note in xact.c.Thanks, your proposed behavior looks reasonable. I didn't like the
existing coding nor the fact that with your patch we'd have two copies
of it, so I changed a bit instead to be more understandable. Hopefully I
didn't break too many things. This patch includes the patch for the
other commitTS open item too.
-#define RecoveryRequiresBoolParameter(param_name, currValue, masterValue) \
-do { \
- bool _currValue = (currValue); \
- bool _masterValue = (masterValue); \
- if (_currValue != _masterValue) \
- ereport(ERROR, \
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE), \
- errmsg("hot standby is not possible because it
requires \"%s\" to be same on master and standby (master has \"%s\",
standby has \"%s\")", \
- param_name, \
- _masterValue ? "true" : "false", \
- _currValue ? "true" : "false"))); \
-} while(0)
This code should not be deleted because there is still the caller of
the macro function.
@@ -5321,7 +5333,7 @@ xact_redo_commit(xl_xact_parsed_commit *parsed,
/* Set the transaction commit timestamp and metadata */
TransactionTreeSetCommitTsData(xid, parsed->nsubxacts, parsed->subxacts,
commit_time, origin_id,
- false);
+ false, true);Why does xact_redo_commit always set replaying_xlog and write_xlog to
false and true, respectively? ISTM that they should be opposite...
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-09-29 13:44, Fujii Masao wrote:
On Tue, Sep 29, 2015 at 12:05 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Petr Jelinek wrote:
On 2015-09-02 16:14, Fujii Masao wrote:
On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
track_commit_timestamp tracks COMMIT PREPARED as expected in standby server,
but not in master server. Is this intentional? It should track COMMIT PREPARED
even in master? Otherwise, we cannot use commit_timestamp feature to check
the replication lag properly while we use 2PC.That sounds like it must be a bug. I think you should add it to the
open items list.Attached fixes this. It includes advancement of replication origin as well.
I didn't feel like doing refactor of commit code this late in 9.5 cycle
though, so I went with the code duplication + note in xact.c.Thanks, your proposed behavior looks reasonable. I didn't like the
existing coding nor the fact that with your patch we'd have two copies
of it, so I changed a bit instead to be more understandable. Hopefully I
didn't break too many things. This patch includes the patch for the
other commitTS open item too.-#define RecoveryRequiresBoolParameter(param_name, currValue, masterValue) \
-do { \
- bool _currValue = (currValue); \
- bool _masterValue = (masterValue); \
- if (_currValue != _masterValue) \
- ereport(ERROR, \
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE), \
- errmsg("hot standby is not possible because it
requires \"%s\" to be same on master and standby (master has \"%s\",
standby has \"%s\")", \
- param_name, \
- _masterValue ? "true" : "false", \
- _currValue ? "true" : "false"))); \
-} while(0)This code should not be deleted because there is still the caller of
the macro function.
Looks like Alvaro didn't merge the second patch correctly, the only
caller should have been removed as well.
--
Petr Jelinek 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
Petr Jelinek wrote:
On 2015-09-29 13:44, Fujii Masao wrote:
On Tue, Sep 29, 2015 at 12:05 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
-#define RecoveryRequiresBoolParameter(param_name, currValue, masterValue) \
-do { \
- bool _currValue = (currValue); \
- bool _masterValue = (masterValue); \
- if (_currValue != _masterValue) \
- ereport(ERROR, \
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE), \
- errmsg("hot standby is not possible because it
requires \"%s\" to be same on master and standby (master has \"%s\",
standby has \"%s\")", \
- param_name, \
- _masterValue ? "true" : "false", \
- _currValue ? "true" : "false"))); \
-} while(0)This code should not be deleted because there is still the caller of
the macro function.Looks like Alvaro didn't merge the second patch correctly, the only caller
should have been removed as well.
filterdiff loses hunks once in a while, which is why I stopped using it
quite some time ago :-( I eyeballed its output to ensure it hadn't
dropped any hunk, but apparently I missed that one.
I guess I should file a bug report.
--
�lvaro Herrera http://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
Hi Fujii, thanks for the review. I have pushed the patch to 9.5 and
master.
Fujii Masao wrote:
@@ -5321,7 +5333,7 @@ xact_redo_commit(xl_xact_parsed_commit *parsed, /* Set the transaction commit timestamp and metadata */ TransactionTreeSetCommitTsData(xid, parsed->nsubxacts, parsed->subxacts, commit_time, origin_id, - false); + false, true);Why does xact_redo_commit always set replaying_xlog and write_xlog to
false and true, respectively? ISTM that they should be opposite...
A stupid oversight on my part. Thanks for pointing it out.
Thanks, Petr, for the patches.
--
�lvaro Herrera http://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