On Mon, Mar 20, 2017 at 1:38 AM, Craig Ringer <cr...@2ndquadrant.com> wrote: > On 14 March 2017 at 19:57, Robert Haas <robertmh...@gmail.com> wrote: >> On Mon, Mar 13, 2017 at 10:22 PM, Craig Ringer <cr...@2ndquadrant.com> wrote: >>> I'll introduce a new LWLock, ClogTruncationLock, which will be held >>> from when we advance the new clogOldestXid field through to when clog >>> truncation completes. >> >> +1. > > OK, here's the revised patch. Relevant comments added where needed. > > It still changes the xlog record for clog truncation to carry the new > xid, but I've left advancing oldestXid until the checkpoint as is > currently the case, and only advanced oldestClogXid when we replay > clog truncation.
/me smacks forehead. Actually, it should be CLogTruncationLock, with a capital L, for consistency with CLogControlLock. The new lock needs to be added to the table in monitoring.sgml. I don't think the new header comments in TransactionIdDidCommit and TransactionIdDidAbort are super-clear. I'm not sure you're going to be able to explain it there in a reasonable number of words, but I think that speaking of "testing against oldestClogXid" will leave people wondering what exactly that means. Maybe just write "caller is responsible for ensuring that the clog records covering XID being looked up can't be truncated away while the lookup is in progress", and then leave the bit about CLogTruncationLock to be explained by the callers that do that. Or you could drop these comments entirely. Overall, though, I think that 0001 looks far better than any previous iteration. It's simple. It looks safe. It seems unlikely to break anything that works now. Woo hoo! -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers