Re: BUG #16931: source code problem about commit_ts

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)