On Tue, Jun 7, 2011 at 3:21 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> 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.
Urk. Well, we don't want that, for sure. The previous discussion
was talking about moving the zeroing around somehow, rather than
getting rid of it, so maybe there's some way to make it work...
One other thought is that I think that this patch might cause a
user-visible behavior change. Right now, when you hit the end of
recovery, you most typically get a message saying - record with zero
length. Not always, but often. If we adopt this approach, you'll get
a wider variety of error messages there, depending on exactly how the
new record fails validation. I dunno if that's important to be worth
caring about, or not.
> 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.
I don't think it's too hard to construct a test case where it is, even
now. pgbench on a medium-sized machine ought to do it, with
synchronous_commit turned off.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
