On Tue, Mar 6, 2012 at 8:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> On 06.03.2012 17:12, Tom Lane wrote:
>>> How long is the current locked code exactly --- does it contain a loop?
>
>> Perhaps best if you take a look for yourself, the function is called
>> ReserveXLogInsertLocation() in patch. It calls a helper function called
>> AdvanceXLogRecPtrToNextPage(ptr), which is small and could be inlined.
>> It does contain one loop, which iterates once for every WAL page the
>> record crosses.
>
> Hm. The loop makes me a tad uncomfortable, because it is possible for
> WAL records to be very long (many pages). I see the point that
> replacing the spinlock with an LWLock would likely negate any
> performance win from this patch, but having other processes arrive and
> spin while somebody is busy calculating the size of a multi-megabyte
> commit record would be bad too.
I would have thought the existence of a multi-megabyte commit record
would already imply a huge performance effect elsewhere and we
wouldn't care too much about a few spinlock cycles. But I think
they're as rare as Higgs bosons.
If/when such records do exist it isn't likely to be on a high
transaction rate server. Even allocating ~1 million xids takes long
enough that we wouldn't be expecting a very high commit rate even with
200 concurrent sessions doing this. If such records are rare, then the
minor blip they cause is just a drop in the ocean.
So I think Tom's concern is valid, but the frequency of problems
resulting from it will be so low as to not even be measurable. And
before we fix a perceived performance issue, we really should prove
its existence first, then confirm that this area is the bottleneck
that is slowing such workloads.
So +1 to Heikki keeping the spinlock, as is, and not redesign anything.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
