Re: Fix checkpoint skip logic on idle systems by tracking LSN progress

On Tue, Nov 1, 2016 at 8:31 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Could you let me struggle a bit more to avoid LWLocks in
> GetProgressRecPtr?

Be my guest :)

> I considered two alternatives for updating logic of progressAt
> more seriously. One is, as Amit suggested, replacing progressAt
> within the SpinLock section in
> ReserverXLogInsertLocation. Another is using pg_atomic_u64 for
> progressAt. The attached two patches rouhgly implement the aboves
> respectively. (But I've not tested them. The patches are to show
> the two alternatives concretely.)

Okay.

> I found that the former requires to take insertpos_lck also on
> reading. I have to admit that this is too bad. (Even I saw no
> degradation by pgbench on my poor environment. It marks 118tr/s
> by 10 threads and that doesn't seem make any stress on xlog
> logics...)

Interesting...

> For the latter, it is free from locks and doesn't reduce parallel
> degree but I'm not sure it is proper to use it there and I'm not
> sure about actual overheads. In the worst case, it makes another
> SpinLock section for every call to pg_atmoic_* functions.

The WAL insert slots have been introduced in 9.4, and the PG atomics
in 9.5, so perhaps the first implementation of the WAL insert slots
would have used it. Still that looks quite promising. At the same time
we may be able to do something for insertingAt to make the locks held
more consistent, and just remove WALInsertLocks, even if this makes me
wonder about torn reads and about how we may break things if we rely
on something else than LW_EXCLUSIVE compared to now. To keep things
more simple I' would still favor using WALInsertLocks for this patch,
that looks more consistent, and also because there is no noticeable
difference.

> All except the above point looks good for me. Maybe it is better
> that XLOG_INCLUDE_ORIGIN stuff is in a separate patch.

I have kept that grouped bas XLOG_INCLUDE_ORIGIN is the only
XLogInsert flag present on HEAD. Could the patch be marked as "ready
for committer" then?
--
Michael