Re: Add shutdown_at_recovery_target option to recovery.conf

Hi Petr,

I have spent sometime to review the patch, overall patch looks good, it
applies fine and make check run without issue. If recovery target is
specified and shutdown_at_recovery_target is set to true, it shutdown the
server at specified recovery point. I do have few points to share i.e.

1. It seems that following log message need to be more descriptive about
reason for shutdown i.e.

+ if (recoveryShutdownAtTarget && reachedStopPoint)
> + {
> + ereport(LOG, (errmsg("shutting down")));

2. As Simon suggesting following recovery settings are not clear i.e.

shutdown_at_recovery_target = true
> pause_at_recovery_target = true

It is going to make true both but patch describe as following i.e.

+ Setting this to true will set <link
> linkend="pause-at-recovery-target">
> + <varname>pause_at_recovery_target</></link> to false.
>

3. As it don’t rename reconvery.conf, subsequent attempt (without any
changes in reconvery.conf) to start of server keep showing the following
i.e.

> ...
> LOG: redo starts at 0/1803620
> DEBUG: checkpointer updated shared memory configuration values
> LOG: consistent recovery state reached at 0/1803658
> LOG: restored log file "000000010000000000000002" from archive
> DEBUG: got WAL segment from archive
> LOG: restored log file "000000010000000000000003" from archive
> DEBUG: got WAL segment from archive
> LOG: restored log file "000000010000000000000004" from archive
> DEBUG: got WAL segment from archive
> LOG: restored log file "000000010000000000000005" from archive
> DEBUG: got WAL segment from archive
> LOG: restored log file "000000010000000000000006" from archive
> DEBUG: got WAL segment from archive
> …
>

Is that right ?. Thanks.

Regards,
Muhammad Asif Naeem

On Thu, Oct 16, 2014 at 7:24 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 11 September 2014 16:02, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>
> >> What about adding something like
> action_at_recovery_target=pause|shutdown
> >> instead of increasing the number of parameters?
> >>
> >
> > That will also increase number of parameters as we can't remove the
> current
> > pause one if we want to be backwards compatible. Also there would have
> to be
> > something like action_at_recovery_target=none or off or something since
> the
> > default is that pause is on and we need to be able to turn off pause
> without
> > having to have shutdown on. What more, I am not sure I see any other
> actions
> > that could be added in the future as promote action already works and
> listen
> > (for RO queries) also already works independently of this.
>
> I accept your argument, though I have other thoughts.
>
> If someone specifies
>
> shutdown_at_recovery_target = true
> pause_at_recovery_target = true
>
> it gets a little hard to work out what to do; we shouldn't allow such
> lack of clarity.
>
> In recovery its easy to do this
>
> if (recoveryShutdownAtTarget)
> recoveryPauseAtTarget = false;
>
> but it won't be when these become GUCs, so I think Fuji's suggestion
> is a good one.
>
> No other comments on patch, other than good idea.
>
> --
> Simon Riggs http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>