From: | Fabrízio de Royes Mello <fabriziomello(at)gmail(dot)com> |
---|---|
To: | Simon Riggs <simon(at)2ndquadrant(dot)com> |
Cc: | Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>, KONDO Mitsumasa <kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> |
Subject: | Re: Time-Delayed Standbys |
Date: | 2013-12-06 04:07:15 |
Message-ID: | CAFcNs+rAJO6wvzN8zQCTUpii0mC9Ax51JxVhWTsos7xsFdwXjw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Dec 3, 2013 at 5:33 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
> > - compute recoveryUntilDelayTime in XLOG_XACT_COMMIT and
> > XLOG_XACT_COMMIT_COMPACT checks
>
> Why just those? Why not aborts and restore points also?
>
I think make no sense execute the delay after aborts and/or restore points,
because it not change data in a standby server.
> > - don't care about clockdrift because it's an admin problem.
>
> Few minor points on things
>
> * The code with comment "Clear any previous recovery delay time" is in
> wrong place, move down or remove completely. Setting the delay to zero
> doesn't prevent calling recoveryDelay(), so that logic looks wrong
> anyway.
>
Fixed.
> * The loop exit in recoveryDelay() is inelegant, should break if <= 0
>
Fixed.
> * There's a spelling mistake in sample
>
Fixed.
> * The patch has whitespace in one place
>
Fixed.
> and one important point...
>
> * The delay loop happens AFTER we check for a pause. Which means if
> the user notices a problem on a commit, then hits pause button on the
> standby, the pause will have no effect and the next commit will be
> applied anyway. Maybe just one commit, but its an off by one error
> that removes the benefit of the patch. So I think we need to test this
> again after we finish delaying
>
> if (xlogctl->recoveryPause)
> recoveryPausesHere();
>
Fixed.
>
> We need to explain in the docs that this is intended only for use in a
> live streaming deployment. It will have little or no meaning in a
> PITR.
>
Fixed.
> I think recovery_time_delay should be called
> <something>_apply_delay
> to highlight the point that it is the apply of records that is
> delayed, not the receipt. And hence the need to document that sync rep
> is NOT slowed down by setting this value.
>
Fixed.
> And to make the name consistent with other parameters, I suggest
> min_standby_apply_delay
>
I agree. Fixed!
> We also need to document caveats about the patch, in that it only
> delays on timestamped WAL records and other records may be applied
> sooner than the delay in some circumstances, so it is not a way to
> avoid all cancellations.
>
> We also need to document the behaviour of the patch is to apply all
> data received as quickly as possible once triggered, so the specified
> delay does not slow down promoting the server to a master. That might
> also be seen as a negative behaviour, since promoting the master
> effectively sets recovery_time_delay to zero.
>
> I will handle the additional documentation, if you can update the
> patch with the main review comments. Thanks.
>
Thanks, your help is welcome.
Att,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Attachment | Content-Type | Size |
---|---|---|
time-delayed-standby-v3.patch | text/x-diff | 7.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2013-12-06 04:16:05 | Re: Regression tests failing if not launched on db "regression" |
Previous Message | Peter Eisentraut | 2013-12-06 03:47:29 | Re: RFC: programmable file format for postgresql.conf |