On Mon, Mar 20, 2017 at 1:38 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 14 March 2017 at 19:57, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Mon, Mar 13, 2017 at 10:22 PM, Craig Ringer <craig@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