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

Thanks for merging. It still applies on the current master with
some displacements.

At Wed, 5 Oct 2016 15:18:53 +0900, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote in <CAB7nPqT4U=OSOLXuFuxMonmfdQFmd5F_0DmKoddvjG-HHWQaBA(at)mail(dot)gmail(dot)com>
> (Squashing replies)
>
> On Fri, Sep 30, 2016 at 6:13 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Fri, Sep 30, 2016 at 2:51 PM, Michael Paquier
> > <michael(dot)paquier(at)gmail(dot)com> wrote:
> >> On Fri, Sep 30, 2016 at 2:05 PM, Kyotaro HORIGUCHI
> >> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> >>> At Fri, 30 Sep 2016 14:00:15 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20160930(dot)140015(dot)150178454(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> >>>> I don't see no problem in setting progressAt in XLogInsertRecord.
> >>>> But I doubt GetProgressRecPtr is harmful, especially when
> >>>
> >>> But I suspect that GetProgressRecPtr could be harmful.
> >>
> >> Well, you can maximize its effects by doing NUM_XLOGINSERT_LOCKS ==
> >> nproc and reducing checkpoint_timeout. That's what I did but..
> >
> > Note: I am moving this patch to next CF.
>
> And I am back on it more seriously... And I am taking back what I said upthread.
>
> I looked at the v12 that Horiguchi-san has written, and that seems
> correct to me. So I have squashed everything into a single patch,

Could you let me struggle a bit more to avoid LWLocks in
GetProgressRecPtr?

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

I found that the former reuiqres 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...)

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.

> including the log entry that gets logged with log_checkpoints. Testing
> with archive_timeout to 10s, checkpoint_timeout to 30s, sometimes
> triggering manual activity with CREATE TABLE/whatever and manual
> pg_switch_xlog(), I am able to see that checkpoints can be correctly
> skipped or generated.
>
> There was as well a compilation error with ereport(). Not sure how I
> missed that... Likely too many patches handled these days.
>
> I have also updated the description of archive_timeout that increasing
> checkpoint_timeout would reduce unnecessary checkpoints on a idle
> system. With this patch, on an idle system checkpoints are just
> skipped as they should.
>
> How does that look?

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

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center