Re: Recovery control functions

On Sat, 2011-01-15 at 20:00 +0900, Fujii Masao wrote:
> On Fri, Jan 14, 2011 at 8:15 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> > On Fri, 2011-01-14 at 11:09 +0000, Simon Riggs wrote:
> >> Functions to control recovery, to aid PITR and Hot Standby.
> >> pg_is_xlog_replay_paused()
> >> pg_xlog_replay_pause()
> >> pg_xlog_replay_resume()
> >>
> >> recovery.conf parameter: pause_at_recovery_target (bool)
> >
> > And now with the correct patch.
> 
> IIRC, in last year, you implemented the related function which
> advances recovery the specified number of records and pauses.
> Why did you drop that from the patch? That's very useful at least
> for me to do PITR and debug the recovery code.

SMoP. It complicated the code and the testing time would have exceeded
the amount of time I had available to spend on this, by a long way.

> +    If in hot standby, all queries will see the same consistent snapshot
> +    of the database, and no query conflicts will be generated.
> 
> Really? The access exclusive lock taken from the master before
> pause of recovery can conflict with a query?

No "recovery conflicts" will be generated. i.e. no new conflicts.

Yes, existing locks will interfere with queries, if they exist.

> +        <primary><varname>pause_at_recovery_target</> recovery
> parameter</primary>
> +      </indexterm>
> +      <listitem>
> +       <para>
> +        Specifies whether recovery should pause when the recovery target
> +        is reached. The default is true, if a recovery target is set.
> 
> The default is false, according to the code.

Thanks. Well spotted.

> If HS is disabled and pause_at_recovery_target is enabled,
> recovery might never end infinitely. This is not desirable.
> We should reject such a combination of settings or emit
> WARNING?

I was about to say "but it already does that". Checking the patch it
seems I must have removed that line, though I can't see any reason why I
would have removed it now. Will put it back.

> +    while (RecoveryIsPaused());
> +    {
> +        pg_usleep(100000L);        /* 100 ms */
> +        HandleStartupProcInterrupts();
> 
> 100ms seems too short. What about 1s or bigger?
> Or wait on the latch rather than using poll loop.

Yes, time is short.

-- Simon Riggs           http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services