Re: [proposal] recovery_target "latest"

On 11/8/19 7:00 AM, Grigory Smolkin wrote:
>
> On 11/7/19 4:36 PM, Grigory Smolkin wrote:
>>
>> On 11/7/19 12:56 PM, Kyotaro Horiguchi wrote:
>>> At Thu, 7 Nov 2019 12:22:28 +0300, Grigory Smolkin
>>> <g(dot)smolkin(at)postgrespro(dot)ru> wrote in
>>>> On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote:
>>>>> At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin
>>>>> <g(dot)smolkin(at)postgrespro(dot)ru> wrote in
>>>>>> On 11/6/19 1:55 PM, Grigory Smolkin wrote:
>>>>>>> On 11/6/19 12:56 PM, Fujii Masao wrote:
>>>>>>>> What happens if this parameter is set to latest in the standby
>>>>>>>> mode?
>>>>>>>> Or the combination of those settings should be prohibited?
>>>>>>> Currently it will behave just like regular standby, so I think, to
>>>>>>> avoid confusion and keep things simple, the combination of them
>>>>>>> should
>>>>>>> be prohibited.
>>>>>>> Thank you for pointing this out, I will work on it.
>>>>>> Attached new patch revision, now it is impossible to use
>>>>>> recovery_target 'latest' in standby mode.
>>>>>> TAP test is updated to reflect this behavior.
>>>>> In the first place, latest (or anything it could be named as) is
>>>>> defined as the explit label for the default behavior. Thus the latest
>>>>> should work as if nothing is set to recovery_target* following the
>>>>> definition.  That might seems somewhat strange but I think at
>>>>> least it
>>>>> is harmless.
>>>>
>>>> Well, it was more about getting default behavior by using some
>>>> explicit recovery_target, not the other way around. Because it will
>>>> break some 3rd party backup and replication applications, that may
>>>> rely on old behavior of ignoring recovery_target_action when no
>>>> recovery_target is provided.
>>>> But you think that it is worth pursuing, I can do that.
>>> Ah. Sorry for the misleading statement. What I had in my mind was
>>> somewhat the mixture of them.  I thought that recovery_target =''
>>> behaves the same way as now, r_t_action is ignored. And 'latest' just
>>> makes recovery_target_action work as the current non-empty
>>> recovery_target's does. But I'm not confident that it is a good
>>> design.
>>>
>>>>> recovery_target=immediate + r_t_action=shutdown for a standby
>>>>> works as
>>>>> commanded. Do we need to inhibit that, too?
>>>> Why something, that work as expected, should be inhibited?
>>> To make sure, I don't think we should do that. I meant by the above
>>> that standby mode is already accepting recovery_target_action so
>>> inhibiting that only for 'latest' is not orthogonal and could be more
>>> confusing for users, and complicatig the code. So my opinion is we
>>> shouldn't inhibit 'latest' unless r_t_action harms.
>>
>> I gave it some thought and now think that prohibiting recovery_target
>> 'latest' and standby was a bad idea.
>> All recovery_targets follow the same pattern of usage, so
>> recovery_target "latest" also must be capable of working in standby
>> mode.
>> All recovery_targets have a clear deterministic 'target' where
>> recovery should end.
>> In case of recovery_target "latest" this target is the end of
>> available WAL, therefore the end of available WAL must be more
>> clearly defined.
>> I will work on it.
>>
>> Thank you for a feedback.
>
>
> Attached new patch revision, now end of available WAL is defined as
> the fact of missing required WAL.
> In case of standby, the end of WAL is defined as 2 consecutive
> switches of WAL source, that didn`t provided requested record.
> In case of streaming standby, each switch of WAL source is forced
> after 3 failed attempts to get new data from walreceiver.
>
> All constants are taken off the top of my head and serves as proof of
> concept.
> TAP test is updated.
>
Attached new revision, it contains some minor refactoring.

While working on it, I stumbled upon something strange:

why DisownLatch(&XLogCtl->recoveryWakeupLatch) is called before
ReadRecord(xlogreader, LastRec, PANIC, false) ?

Isn`t this latch may be accessed in WaitForWALToBecomeAvailable() if
streaming standby gets promoted?

>
>
>>
>>
>>>
>>> regards.
>>>
--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company