Date: Wed, 24 Mar 2021 06:05:19 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: zoulx1982@163.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16931: source code problem about commit_ts
Message-ID: <20210324090519.GA14510@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <d5b98b4b-a3e3-8095-cfb9-aafb5dbe4f7b@oss.nttdata.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: 
 <https://www.postgresql.org/message-id/20210324090519.GA14510%40alvherre.pgsql>
Precedence: bulk

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)