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

Hello,

At Tue, 8 Nov 2016 14:45:59 +0900, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote in <CAB7nPqT-VW5gRbUJwQusmgiu2MKpZSCV-XdrHv84w8FZa286KQ(at)mail(dot)gmail(dot)com>
> 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

If I understand you correctly, atomics prevents torn reads by its
definition on cache management and bus arbitration level. It
should be faster than LWlocks but as I said in the previous mail,
on some platforms, if any, it will fallbacks to individual
spinlocks. (atomics.c)

> 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.

Ok, the patch looks fine. So there's nothing for me but to accept
the current shape since the discussion about performance seems
not to be settled with out performance measurement with machines
with many cores.

> > 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?

Ok, that is not so siginificant point. Well, I'd like to wait for
a couple of days for anyone wants to comment, then mark this
'ready for committer'.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center