Thread: BUG #16931: source code problem about commit_ts
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.
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
Attachment
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
Attachment
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