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 <[email protected]> wrote:
> On 11 September 2014 16:02, Petr Jelinek <[email protected]> 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 ([email protected])
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
