On 21.02.2012 13:19, Fujii Masao wrote:
> On Sat, Feb 18, 2012 at 12:36 AM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>> Attached is a new version, fixing that, and off-by-one bug you pointed out
>> in the slot wraparound handling. I also moved code around a bit, I think
>> this new division of labor between the XLogInsert subroutines is more
>> readable.
>
> This patch includes not only xlog scaling improvement but also other ones.
> I think it's better to extract them as separate patches and commit them first.
> If we do so, the main patch would become more readable.
Good point.
> For example, I think that the followings can be extracted as a separate patch.
>
> (1) Make walwriter try to initialize as many of the no-longer-needed
> WAL buffers
> for future use as we can.
This is pretty hard to extract from the larger patch. The current code
in master assumes that there's only one page that is currently inserted
to, and relies on WALInsertLock being held in AdvanceXLInsertBuffer().
The logic with the scaling patch is quite different.
> (2) Refactor the "update full_page_writes code".
> (3) Get rid of XLogCtl->Write.LogwrtResult and XLogCtl->Insert.LogwrtResult.
Attached are patches for these two items. Barring objections, I'll
commit these.
> (4) Call TRACE_POSTGRESQL_XLOG_SWITCH() even if the xlog switch has no
> work to do.
Actually, I think I'll just move it in the patch to keep the existing
behavior.
> I'm not sure if (3) makes sense. In current master, those two shared variables
> are used to reduce the contention of XLogCtl->info_lck and WALWriteLock.
> You think they have no effect on reducing the lock contention?
XLogCtl->Write.LogwrtResult certainly seems redundant with
XLogCtl->LogwrtResult. There might be some value in
XLogCtl->Insert.LogwrtResult, it's used in AdvanceXLInsertBuffer() to
before acquiring info_lck. But I doubt that makes any difference in
practice either. At best it's saving one spinlock acquisition per WAL
buffer, which isn't all much compared to all the other work involved.
(once the scaling patch is committed, this point is moot anyway)
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
