BUG #16931: source code problem about commit_ts

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

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)

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

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/

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

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

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

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

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

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

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