Re: pg_stop_backup wait bug fix

Hi,

I have looked at the patch and it looks OK to me. BTW I am not too
much familiar with this area of code, so I am not at the position to
argue that patch -:) . I haven't found an easy way to test the patch.

On Wed, Dec 3, 2008 at 1:24 PM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> Fujii Masao wrote:
>>
>> On Wed, Dec 3, 2008 at 5:13 AM, Heikki Linnakangas
>> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>>
>>> Agreed, should use XLByteToPrevSeg. But I wonder if we can just replace
>>> the
>>> current XLByteToSeg call with XLByteToPrevSeg? That would offset the
>>> return
>>> value of the function by one byte as well, as well as the value printed
>>> to
>>> the backup history file. In fact, I think the original patch got that
>>> wrong;
>>> it would return the location of the *beginning* of the last xlog file.
>>
>> You're right. As you say, the value (stopxlogfilename) printed to the
>> backup
>> history file is wrong. But, since the value is not used fortunately,
>> any troubles
>> have not come up. So, I think that we can just replace them.
>
> Changing the return value doesn't seem like a good idea. If nothing else, it
> would be complicated to explain what it returns. I committed a patch that
> changes the waiting behavior, but not the return value or what's written
> into the backup label file,
>
>>> I also noticed that the 2nd BackupHistoryFileName call in that function
>>> is
>>> useless; histfilepath variable is already filled in earlier.
>>
>> Somewhat confusingly, BackupHistoryFileName is called only once. Isn't 1st
>> (which probably you thought) BackupHistoryFilePath?
>
> Ouch, you're right. That's subtle.
>
>> In order to prevent
>> confusion, we should add new local variable (histfilename) for the backup
>> history file name?
>
> Agreed. I included that in the patch.
>
> --
> Heikki Linnakangas
> EnterpriseDB http://www.enterprisedb.com
>
> --
> 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
>

--
Ibrar Ahmed
EnterpriseDB http://www.enterprisedb.com