On Mon, Jun 6, 2011 at 11:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> As to the question of whether it's safe, I think I'd agree that the
> chances of this backfiring are pretty remote. I think that with the
> zeroing they are exactly zero, because (now that we start XLOG
> positions at 0/1) there is now way that xl_prev = {0, 0} can ever be
> valid. Without the zeroing, well, it's remotely possible that xl_prev
> could happen to appear valid and that xl_crc could happen to match...
> but the chances are presumably quite remote. Just the chances of the
> CRC matching should be around 2^-32. The chances of an xl_prev match
> are harder to predict, because the matching values for CRCs should be
> pretty much uniformly distributed, while xl_prev isn't random. But
> even given that the chance of a match is should be very small, so in
> practice there is likely no harm.
And if such a thing did actually happen we would also need to have an
accidentally correct value of all of the rest of the header values.
And even if we did we would apply at most one junk WAL record. Then we
are onto the next WAL record where we would need have to the same luck
all over again.
The probability of these occurrences is well below the acceptable
threshold for other problems occurring.
> It strikes me, though, that we
> could probably get nearly all of the benefit of this patch by being
> willing to zero the first sizeof(XLogRecord) bytes following a record,
> but not the rest of the buffer. That would pretty much wipe out any
> chance of an xl_prev match, I think, and would likely still get nearly
> all of the performance benefit.
Which adds something onto the path of every XlogInsert(), rather than
once per page, so I'm a little hesitant to agree.
If we did that, we would only need to write out an additional 12 bytes
per WAL record, not the full sizeof(XLogRecord).
But even so, I think its wasted effort.
Measuring the benefit of a performance patch is normal, but I'm not
proposing this as a risk trade-off. It's just a straight removal of
multiple cycles from a hot code path. The exact benefit will depend
upon whether the WALInsertLock is the hot lock, which it likely will
be when other patches are applied.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
