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+28-1
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
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+112-81
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