Thread: 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
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
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?
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
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
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
Attachment
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 shouldpass "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
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
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
Attachment
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
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
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
Attachment
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
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
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
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
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