Re: Load Distributed Checkpoints, revised patch

Heikki Linnakangas wrote:
> Alvaro Herrera wrote:

> >    if (BgWriterShmem->ckpt_time_warn && elapsed_secs <
> >    CheckPointWarning)
> >        ereport(LOG,
> >                (errmsg("checkpoints are occurring too
> >                frequently (%d seconds apart)",
> >                        elapsed_secs),
> >                 errhint("Consider increasing the
> >                 configuration parameter
> >                 \"checkpoint_segments\".")));
> >    BgWriterShmem->ckpt_time_warn = false;
>
> In the extremely unlikely event that RequestCheckpoint sets
> ckpt_time_warn right before it's cleared, after the test in the
> if-statement, the warning is missed.

I think this code should look like this:

    if (BgWriterShmem->ckpt_time_warn)
    {
        BgWriterShmem->chpt_time_warn = false;
        if (elapsed_secs < CheckPointWarning)
            ereport(LOG,
                    (errmsg("checkpoints are occurring too frequently (%d seconds apart)",
                            elapsed_secs),
                     errhint("Consider increasing the configuration parameter \"checkpoint_segments\".")));
    }

That way seems safer.  (I am assuming that a process other than the
bgwriter is able to set the ckpt_time_warn bit; otherwise there is no
point).  This is also used in pmsignal.c.  Of course, as you say, this
is probably very harmless, but in the other case it is important to get
it right.

--
Alvaro Herrera                               http://www.PlanetPostgreSQL.org/
"Hackers share the surgeon's secret pleasure in poking about in gross innards,
the teenager's secret pleasure in popping zits."                 (Paul Graham)