On 24 January 2017 at 23:49, Robert Haas <robertmhaas@gmail.com> wrote:
> I think it's fairly surprising that TruncateCLOG() has responsibility
> for writing the xlog record that protects advancing
> ShmemVariableCache->oldestXid, but not the responsibility for actually
> advancing that value. In other words, I think the AdvanceOldestXid()
> call in vac_truncate_clog() should be moved into TruncateClog().
> (Similarly, one wonders why AdvanceOldestCommitTsXid() isn't the
> responsibility of TruncateCommitTs().)
Agreed, and done in attached.
I haven't done the same for commit_ts but will do so on the thread for
the commit_ts race fix if we go ahead with this change here.
> I think it is not correct to advance oldestXid but not oldestXidDB.
> Otherwise, GetNewTransactionId() might complain about the wrong
> database.
That's a clear oversight. Fixed in attached.
> The way that SetTransactionIdLimit() now works looks a bit dangerous.
> xidWrapLimit, xidStopLimit, and xidWarnLimit are computed based on the
> passed-in oldestXid value and written straight into shared memory.
> But the shared memory copy of oldestXid could have a different value.
> I'm not sure if that breaks anything, but it certainly weakens any
> confidence callers might have had that all those values are consistent
> with each other.
This was my main hesitation with the whole thing too.
It's necessary to advance oldestXmin before we xlog the advance and
truncate clog, and necessary to advance the vacuum limits only
afterwards.
I thought it sensible to be conservative about the xid limit to
minimise the change from current behaviour, i.e. advance only up to
xid limits calculated from the oldestXid determined by the currently
vac_truncate_clog. But the current one is actually wrong; in the
(unlikely if it's possible at all) case where two vac_truncate_clog()s
A and B run with ordering A(advanceOldestXmin) B(advanceOldestXmin)
A(truncate) B(truncate) B(advanceLimits) A(advanceLimits), A's advance
of the limits would clobber b's. Not too bad, but it'd delay
advancing the limits until the next vacuum, and some xid could get
allocated before the limits go backwards.
It's safer to take XidGenLock for slightly longer in order to capture
the oldestXid from shmem and calculate using it. That ensures we never
have any risk of going backwards. The attached updated patch does so.
BTW, I find it quite amusing that this was intended to be a small,
unintrusive patch, and now it's messing with xid limits and clog
truncation. I'm almost tempted to say we should commit it with the
(tiny) race with clog truncation in place. We certainly haven't had
any complaints about the same race from people using commit_ts. But
the race window on standby is quite large and I'd rather not knowingly
introduce new bugs.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers