Re: LSN as a recovery target

On 08/23/2016 10:39 AM, Petr Jelinek wrote:
> On 23/08/16 09:33, Michael Paquier wrote:
>> On Tue, Aug 23, 2016 at 12:49 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
>> wrote:
>>> On Mon, Aug 22, 2016 at 8:28 AM, Michael Paquier
>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> On Mon, Aug 22, 2016 at 7:12 PM, Adrien Nayrat
>>>> <adrien(dot)nayrat(at)dalibo(dot)com> wrote:
>>>>> As Julien said, there is nothing to notice that error comes from
>>>>> recovery.conf.
>>>>> My fear would be that an user encounters an error like this. Il
>>>>> will be
>>>>> difficult to link to the recovery.conf.
>>>>
>>>> Thinking a bit wider than that, we may want to know such context for
>>>> normal GUC parameters as well, and that's not the case now. Perhaps
>>>> there is actually a reason why that's not done for GUCs, but it seems
>>>> that it would be useful there as well. That would give another reason
>>>> to move all that under the GUC umbrella.
>>>
>>> Maybe so, but that's been tried multiple times without success. If
>>> you think an error context is useful here, and I bet it is, I'd say
>>> just add it and be done with it.
>>
>> This has finished by being less ugly than I thought, so I implemented
>> it as attached. Patch 0001 introduces recovery_target_lsn, and patch
>> 0002 sets up an error context callback generating things like that on
>> failure:
>> FATAL: invalid input syntax for type pg_lsn: "popo"
>> CONTEXT: line 11 of configuration file "recovery.conf", parameter
>> "recovery_target_lsn"

Good! Message is clear now for recovery_target_lsn and recovery_target_time.

Thanks for your work.

>>
>
> Looks very reasonable to me (both patches). Thanks for doing that.
>
> I am inclined to mark this as ready for committer.
>

+1 everything if fine for me.

--
Adrien NAYRAT

http://dalibo.com - http://dalibo.org