The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            not tested

Hello

I think it is convenient to know the last commit LSN of a database for 
troubleshooting and monitoring purposes similar to the "pd_lsn" field in a page 
header that records the last LSN that modifies this page. I am not sure if it 
can help determine the WAL location to resume / catch up in logical replication 
as the "confirmed_flush_lsn" and "restart_lsn" in a logical replication slot 
are already there to figure out the resume / catchup point as I understand. 
Anyhow, I had a look at the patch, in general it looks good to me. Also ran it 
against the CI bot, which also turned out fine. Just one small comment though. 
The patch supports the recording of last commit lsn from 2 phase commit as 
well, but the test does not seem to have a test on 2 phase commit. In my 
opinion, it should test whether the last commit lsn increments when a prepared 
transaction is committed in addition to a regular transaction.

thank you
-----------------
Cary Huang
www.highgo.ca

Reply via email to