Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Remove leftover dead code from commit_ts.h.

  2. Remove COMMIT_TS_SETTS record.

  3. Fix bug in WAL replay of COMMIT_TS_SETTS record.

  1. BUG #16931: source code problem about commit_ts

    PG Bug reporting form <noreply@postgresql.org> — 2021-03-18T04:36:08Z

    The following bug has been logged on the website:

Bug reference:      16931
Logged by:          lx zou
Email address:      zoulx1982@163.com
PostgreSQL version: 13.2
Operating system:   Linux
Description:        

Hi,
recently i am reading commit ts code, and i have a problem about it.
i found commit_ts_redo call TransactionTreeSetCommitTsData with last param
`write_xlog`  true,
but RecordTransactionCommitPrepared and RecordTransactionCommit call
TransactionTreeSetCommitTsData 
with `write_xlog` false, which cause there are never xlog with commit_ts -
COMMIT_TS_SETTS type.
thanks for your time.

best wishes.

  2. Re: BUG #16931: source code problem about commit_ts

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-18T10:19:10Z

    
On 2021/03/18 13:36, PG Bug reporting form wrote:
> The following bug has been logged on the website:
> 
> Bug reference:      16931
> Logged by:          lx zou
> Email address:      zoulx1982@163.com
> PostgreSQL version: 13.2
> Operating system:   Linux
> Description:
> 
> Hi,
> recently i am reading commit ts code, and i have a problem about it.
> i found commit_ts_redo call TransactionTreeSetCommitTsData with last param
> `write_xlog`  true,
> but RecordTransactionCommitPrepared and RecordTransactionCommit call
> TransactionTreeSetCommitTsData
> with `write_xlog` false, which cause there are never xlog with commit_ts -
> COMMIT_TS_SETTS type.
> thanks for your time.

I guess that TransactionTreeSetCommitTsData(write_xlog=true) is basically
used by some extensions using commit_ts.

IIUC commit_ts_redo() is called during recovery. So it's strange that
commit_ts_redo() calls TransactionTreeSetCommitTsData() with write_xlog=true
because no new WAL can be generated during recovery. Probably this is a bug,
and it should be called with write_xlog=false, instead. Patch attached.

Also one problem is that there is no test for WAL replay of COMMIT_TS_SETTS
for now. Maybe this is why we could not find that bug.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  3. Re: BUG #16931: source code problem about commit_ts

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-24T09:05:19Z

    On 2021-Mar-18, Fujii Masao wrote:

> On 2021/03/18 13:36, PG Bug reporting form wrote:

> > Hi,
> > recently i am reading commit ts code, and i have a problem about it.
> > i found commit_ts_redo call TransactionTreeSetCommitTsData with last param
> > `write_xlog`  true,
> > but RecordTransactionCommitPrepared and RecordTransactionCommit call
> > TransactionTreeSetCommitTsData
> > with `write_xlog` false, which cause there are never xlog with commit_ts -
> > COMMIT_TS_SETTS type.

Hmm, sharp eyes.

> I guess that TransactionTreeSetCommitTsData(write_xlog=true) is basically
> used by some extensions using commit_ts.

I am not aware of any extensions that do that.

> IIUC commit_ts_redo() is called during recovery. So it's strange that
> commit_ts_redo() calls TransactionTreeSetCommitTsData() with write_xlog=true
> because no new WAL can be generated during recovery. Probably this is a bug,
> and it should be called with write_xlog=false, instead. Patch attached.

Yeah, it seems like a bug.  With your patch, the write_xlog=true path
becomes unused, and thus the whole COMMIT_TS_SETTS record, so we could
remove those things in branch master.  The timestamp is acquired from
the COMMIT record.

> Also one problem is that there is no test for WAL replay of COMMIT_TS_SETTS
> for now. Maybe this is why we could not find that bug.

Yeah, coverage for xlog in commit_ts.c is lacking.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo lógico y coherente. Pero el universo real se halla siempre
un paso más allá de la lógica" (Irulan)

  4. Re: BUG #16931: source code problem about commit_ts

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-24T10:25:54Z

    
On 2021/03/24 18:05, Alvaro Herrera wrote:
> Yeah, it seems like a bug.  With your patch, the write_xlog=true path
> becomes unused, and thus the whole COMMIT_TS_SETTS record, so we could
> remove those things in branch master.  The timestamp is acquired from
> the COMMIT record.

I agree to remove COMMIT_TS_SETTS record from the master branch
if there are no users or extensions of it. Patch attached.

Anyway, at first, what about applying the bugfix patch I posted upthread
to all supported branches?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  5. Re: BUG #16931: source code problem about commit_ts

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-24T16:39:31Z

    On 2021-Mar-24, Fujii Masao wrote:

> On 2021/03/24 18:05, Alvaro Herrera wrote:
> > Yeah, it seems like a bug.  With your patch, the write_xlog=true path
> > becomes unused, and thus the whole COMMIT_TS_SETTS record, so we could
> > remove those things in branch master.  The timestamp is acquired from
> > the COMMIT record.
> 
> I agree to remove COMMIT_TS_SETTS record from the master branch
> if there are no users or extensions of it. Patch attached.

Looks good in a quick skim.  Let's get this pushed quickly; if anything
exists out there that is calling that code, it'd be good to know sooner
rather than later.  However, my feeling is that no such caller exists,
because they would have told us about this problem already.

Are you bumping WAL page magic?  It'd be logical to do so, but on the
other hand if no code exists that is capable of emitting that record,
then it'd be pointless.

> Anyway, at first, what about applying the bugfix patch I posted upthread
> to all supported branches?

Yeah, let's do that.


-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"XML!" Exclaimed C++.  "What are you doing here? You're not a programming
language."
"Tell that to the people who use me," said XML.
https://burningbird.net/the-parable-of-the-languages/

  6. Re: BUG #16931: source code problem about commit_ts

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-25T02:59:49Z

    
On 2021/03/25 1:39, Alvaro Herrera wrote:
> On 2021-Mar-24, Fujii Masao wrote:
> 
>> On 2021/03/24 18:05, Alvaro Herrera wrote:
>>> Yeah, it seems like a bug.  With your patch, the write_xlog=true path
>>> becomes unused, and thus the whole COMMIT_TS_SETTS record, so we could
>>> remove those things in branch master.  The timestamp is acquired from
>>> the COMMIT record.
>>
>> I agree to remove COMMIT_TS_SETTS record from the master branch
>> if there are no users or extensions of it. Patch attached.
> 
> Looks good in a quick skim.  Let's get this pushed quickly; if anything
> exists out there that is calling that code, it'd be good to know sooner
> rather than later.  However, my feeling is that no such caller exists,
> because they would have told us about this problem already.

Agreed. I will commit the patch in the master.

> Are you bumping WAL page magic?  It'd be logical to do so, but on the
> other hand if no code exists that is capable of emitting that record,
> then it'd be pointless.

Yes, I'm thinking to bump WAL page magic because it seems a bit safer.

>> Anyway, at first, what about applying the bugfix patch I posted upthread
>> to all supported branches?
> 
> Yeah, let's do that.

Pushed. Thanks!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  7. Re: BUG #16931: source code problem about commit_ts

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-09T22:42:41Z

    On 2021-Mar-24, Fujii Masao wrote:

> diff --git a/src/backend/access/rmgrdesc/committsdesc.c b/src/backend/access/rmgrdesc/committsdesc.c
> index 7ebd3d35ef..26bad44b96 100644
> --- a/src/backend/access/rmgrdesc/committsdesc.c
> +++ b/src/backend/access/rmgrdesc/committsdesc.c
> @@ -38,31 +38,6 @@ commit_ts_desc(StringInfo buf, XLogReaderState *record)
>  		appendStringInfo(buf, "pageno %d, oldestXid %u",
>  						 trunc->pageno, trunc->oldestXid);
>  	}
> -	else if (info == COMMIT_TS_SETTS)

You have not pushed this one, right?  I think we should do it now.

Thanks

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

  8. Re: BUG #16931: source code problem about commit_ts

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-04-11T15:09:45Z

    
On 2021/04/10 7:42, Alvaro Herrera wrote:
> On 2021-Mar-24, Fujii Masao wrote:
> 
>> diff --git a/src/backend/access/rmgrdesc/committsdesc.c b/src/backend/access/rmgrdesc/committsdesc.c
>> index 7ebd3d35ef..26bad44b96 100644
>> --- a/src/backend/access/rmgrdesc/committsdesc.c
>> +++ b/src/backend/access/rmgrdesc/committsdesc.c
>> @@ -38,31 +38,6 @@ commit_ts_desc(StringInfo buf, XLogReaderState *record)
>>   		appendStringInfo(buf, "pageno %d, oldestXid %u",
>>   						 trunc->pageno, trunc->oldestXid);
>>   	}
>> -	else if (info == COMMIT_TS_SETTS)
> 
> You have not pushed this one, right?  I think we should do it now.

Thanks for the ping! Pushed!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  9. Re: BUG #16931: source code problem about commit_ts

    Andy Fan <zhihuifan1213@163.com> — 2025-07-02T01:42:32Z

    Fujii Masao <masao.fujii@oss.nttdata.com> writes:

Hi,

> On 2021/04/10 7:42, Alvaro Herrera wrote:
>> On 2021-Mar-24, Fujii Masao wrote:
>> 
>>> diff --git a/src/backend/access/rmgrdesc/committsdesc.c b/src/backend/access/rmgrdesc/committsdesc.c
>>> index 7ebd3d35ef..26bad44b96 100644
>>> --- a/src/backend/access/rmgrdesc/committsdesc.c
>>> +++ b/src/backend/access/rmgrdesc/committsdesc.c
>>> @@ -38,31 +38,6 @@ commit_ts_desc(StringInfo buf, XLogReaderState *record)
>>>   		appendStringInfo(buf, "pageno %d, oldestXid %u",
>>>   						 trunc->pageno, trunc->oldestXid);
>>>   	}
>>> -	else if (info == COMMIT_TS_SETTS)
>> You have not pushed this one, right?  I think we should do it now.
>
> Thanks for the ping! Pushed!

Did this commit(08aa89b326261b669648df97d4f2a6edba22d26a) forget to
remove struct xl_commit_ts_set? After it, there is no reference to
xl_commit_ts_set. So Is it better clean them as well? 

typedef struct xl_commit_ts_set
{
	TimestampTz timestamp;
	RepOriginId nodeid;
	TransactionId mainxid;
	/* subxact Xids follow */
}			xl_commit_ts_set;

#define SizeOfCommitTsSet	(offsetof(xl_commit_ts_set, mainxid) + \

(When going through commit_ts.h, these code makes me think there is a
dedicated xlog for commit_ts, but the fact isn't, the commit_ts is
recorded in xl_xact_commit, removing dead code could be useful for such
case).

-- 
Best Regards
Andy Fan

  10. Re: BUG #16931: source code problem about commit_ts

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-02T08:44:30Z

    
On 2025/07/02 10:42, Andy Fan wrote:
> Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> 
> Hi,
> 
>> On 2021/04/10 7:42, Alvaro Herrera wrote:
>>> On 2021-Mar-24, Fujii Masao wrote:
>>>
>>>> diff --git a/src/backend/access/rmgrdesc/committsdesc.c b/src/backend/access/rmgrdesc/committsdesc.c
>>>> index 7ebd3d35ef..26bad44b96 100644
>>>> --- a/src/backend/access/rmgrdesc/committsdesc.c
>>>> +++ b/src/backend/access/rmgrdesc/committsdesc.c
>>>> @@ -38,31 +38,6 @@ commit_ts_desc(StringInfo buf, XLogReaderState *record)
>>>>    		appendStringInfo(buf, "pageno %d, oldestXid %u",
>>>>    						 trunc->pageno, trunc->oldestXid);
>>>>    	}
>>>> -	else if (info == COMMIT_TS_SETTS)
>>> You have not pushed this one, right?  I think we should do it now.
>>
>> Thanks for the ping! Pushed!
> 
> Did this commit(08aa89b326261b669648df97d4f2a6edba22d26a) forget to
> remove struct xl_commit_ts_set?

You're right. I seem to have overlooked that.


> After it, there is no reference to
> xl_commit_ts_set. So Is it better clean them as well?

Agreed, the patch looks good to me. Unless there are objections,
I'll go ahead and commit it. While this could be considered an oversight
in the original commit, it's not a bug fix, so I plan to apply it
only to the master branch.

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

  11. Re: BUG #16931: source code problem about commit_ts

    Andy Fan <zhihuifan1213@163.com> — 2025-07-03T13:33:27Z

    Fujii Masao <masao.fujii@oss.nttdata.com> writes:

>>>
>>> Thanks for the ping! Pushed!
>> Did this commit(08aa89b326261b669648df97d4f2a6edba22d26a) forget to
>> remove struct xl_commit_ts_set?
>
> You're right. I seem to have overlooked that.
>
>> After it, there is no reference to
>> xl_commit_ts_set. So Is it better clean them as well?
>
> Agreed, the patch looks good to me. Unless there are objections,
> I'll go ahead and commit it.

Thank you Fujii!

> While this could be considered an oversight in the original commit,
> it's not a bug fix, so I plan to apply it only to the master branch. 

That makes sense to me.

-- 
Best Regards
Andy Fan

  12. Re: BUG #16931: source code problem about commit_ts

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-03T14:42:16Z

    
On 2025/07/03 22:33, Andy Fan wrote:
> Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> 
>>>>
>>>> Thanks for the ping! Pushed!
>>> Did this commit(08aa89b326261b669648df97d4f2a6edba22d26a) forget to
>>> remove struct xl_commit_ts_set?
>>
>> You're right. I seem to have overlooked that.
>>
>>> After it, there is no reference to
>>> xl_commit_ts_set. So Is it better clean them as well?
>>
>> Agreed, the patch looks good to me. Unless there are objections,
>> I'll go ahead and commit it.
> 
> Thank you Fujii!
> 
>> While this could be considered an oversight in the original commit,
>> it's not a bug fix, so I plan to apply it only to the master branch.
> 
> That makes sense to me.

I've pushed the patch. Thanks!

Regards,


-- 
Fujii Masao
NTT DATA Japan Corporation