Re: [HACKERS] track_commit_timestamp and COMMIT PREPARED

[Petr Jelinek]

On 2015-09-29 13:44, Fujii Masao wrote:

On Tue, Sep 29, 2015 at 12:05 PM, 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  wrote:

On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao  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

Re: [HACKERS] track_commit_timestamp and COMMIT PREPARED

[Fujii Masao]

On Tue, Sep 29, 2015 at 12:05 PM, 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  wrote:
>> >>On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao  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

Re: [HACKERS] track_commit_timestamp and COMMIT PREPARED

[Petr Jelinek]

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  wrote:

On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao  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

Re: [HACKERS] track_commit_timestamp and COMMIT PREPARED

[Alvaro Herrera]

Petr Jelinek wrote:
> On 2015-09-29 13:44, Fujii Masao wrote:
> >On Tue, Sep 29, 2015 at 12:05 PM, Alvaro Herrera
> > 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 Herrerahttp://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] track_commit_timestamp and COMMIT PREPARED

[Alvaro Herrera]

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 Herrerahttp://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] track_commit_timestamp and COMMIT PREPARED

[Petr Jelinek]

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
>From e8c68da28dd3f6b785aa23d4cf3c2973d6bca6c5 Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
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 
 #include 
 
+#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 

Re: [HACKERS] track_commit_timestamp and COMMIT PREPARED

[Robert Haas]

On Mon, Sep 28, 2015 at 2:07 PM, Petr Jelinek  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

Re: [HACKERS] track_commit_timestamp and COMMIT PREPARED

[Robert Haas]

On Fri, Sep 18, 2015 at 12:53 AM, Fujii Masao  wrote:
> On Sat, Sep 5, 2015 at 7:48 PM, Petr Jelinek  wrote:
>> On 2015-09-02 16:14, Fujii Masao wrote:
>>>
>>> On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas  wrote:

 On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao 
 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

Re: [HACKERS] track_commit_timestamp and COMMIT PREPARED

[Alvaro Herrera]

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 Herrerahttp://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] track_commit_timestamp and COMMIT PREPARED

[Alvaro Herrera]

Petr Jelinek wrote:
> On 2015-09-02 16:14, Fujii Masao wrote:
> >On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas  wrote:
> >>On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao  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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
*** 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 
  #include 
  
+ #include "access/commit_ts.h"
  #include "access/htup_details.h"
  #include "access/subtrans.h"
  #include "access/transam.h"
***
*** 56,63 
  #include 

Re: [HACKERS] track_commit_timestamp and COMMIT PREPARED

[Fujii Masao]

On Sat, Sep 5, 2015 at 7:48 PM, Petr Jelinek  wrote:
> On 2015-09-02 16:14, Fujii Masao wrote:
>>
>> On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas  wrote:
>>>
>>> On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao 
>>> 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

Re: [HACKERS] track_commit_timestamp and COMMIT PREPARED

[Petr Jelinek]

On 2015-09-02 16:14, Fujii Masao wrote:

On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas  wrote:

On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao  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
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 
 #include 
 
+#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)

-- 
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] track_commit_timestamp and COMMIT PREPARED

[Fujii Masao]

On Wed, Aug 5, 2015 at 2:16 AM, Robert Haas  wrote:
> On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao  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

Re: [HACKERS] track_commit_timestamp and COMMIT PREPARED

[Andres Freund]

On 2015-08-04 13:16:52 -0400, Robert Haas wrote:
 On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao masao.fu...@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

Re: [HACKERS] track_commit_timestamp and COMMIT PREPARED

[Robert Haas]

On Tue, Aug 4, 2015 at 1:18 PM, Andres Freund and...@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.fu...@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

Re: [HACKERS] track_commit_timestamp and COMMIT PREPARED

[Robert Haas]

On Mon, Aug 3, 2015 at 10:31 AM, Fujii Masao masao.fu...@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