Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Make pg_recievexlog by default loop on connection failures

  1. several problems in pg_receivexlog

    Fujii Masao <masao.fujii@gmail.com> — 2012-07-09T18:23:58Z

    Hi,

I found several problems in pg_receivexlog, e.g., memory leaks,
file-descripter leaks, ..etc. The attached patch fixes these problems.

ISTM there are still some other problems in pg_receivexlog, so I'll
read it deeply later.

Regards,

-- 
Fujii Masao

  2. Re: several problems in pg_receivexlog

    Magnus Hagander <magnus@hagander.net> — 2012-07-09T21:27:06Z

    On Mon, Jul 9, 2012 at 8:23 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> Hi,
>
> I found several problems in pg_receivexlog, e.g., memory leaks,
> file-descripter leaks, ..etc. The attached patch fixes these problems.

While I don't doubt that what you've found are real problems, would
you mind explaining exactly what they are, so we don't have to
reverse-engineer the patch to figure that out?


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

  3. Re: several problems in pg_receivexlog

    Fujii Masao <masao.fujii@gmail.com> — 2012-07-10T16:45:39Z

    On Tue, Jul 10, 2012 at 6:27 AM, Magnus Hagander <magnus@hagander.net> wrote:
> On Mon, Jul 9, 2012 at 8:23 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> Hi,
>>
>> I found several problems in pg_receivexlog, e.g., memory leaks,
>> file-descripter leaks, ..etc. The attached patch fixes these problems.
>
> While I don't doubt that what you've found are real problems, would
> you mind explaining exactly what they are, so we don't have to
> reverse-engineer the patch to figure that out?

Yep.

When an error happens after replication connection has been established,
pg_receivexlog doesn't close an open file descriptor and release an allocated
memory area. This was harmless before 16282ae688de2b320cf176e9be8a89e4dfc60698
because pg_receivexlog exits immediately when an error happens. But
currently in an error case, pg_receivexlog tries reconnecting to the server
infinitely, so file descriptors and memory would leak. I think this is problem
and should be fixed. The patch which I submitted yesterday changes
pg_receivexlog so that it closes the open file and frees the memory area
before reconnecting to the server.

Regards,

-- 
Fujii Masao

  4. Re: several problems in pg_receivexlog

    Fujii Masao <masao.fujii@gmail.com> — 2012-07-10T17:03:06Z

    On Tue, Jul 10, 2012 at 3:23 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> Hi,
>
> I found several problems in pg_receivexlog, e.g., memory leaks,
> file-descripter leaks, ..etc. The attached patch fixes these problems.
>
> ISTM there are still some other problems in pg_receivexlog, so I'll
> read it deeply later.

While pg_basebackup background process is streaming WAL records,
if its replication connection is terminated (e.g., walsender in the server
is accidentally terminated by SIGTERM signal), pg_basebackup ends
up failing to include all required WAL files in the backup. The problem
is that, in this case, pg_basebackup doesn't emit any error message at all.
So an user might misunderstand that a base backup has been successfully
taken even though it doesn't include all required WAL files.

To fix this problem, I think that, when the replication connection is
terminated, ReceiveXlogStream() should check whether we've already
reached the stop point by calling stream_stop() before returning TRUE.
If we've not yet (this means that we've not received all required WAL
files yet), ReceiveXlogStream() should return FALSE and
pg_basebackup should emit an error message.  Comments?

Regards,

-- 
Fujii Masao

  5. Re: several problems in pg_receivexlog

    Magnus Hagander <magnus@hagander.net> — 2012-07-12T11:35:11Z

    On Tue, Jul 10, 2012 at 6:45 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Tue, Jul 10, 2012 at 6:27 AM, Magnus Hagander <magnus@hagander.net> wrote:
>> On Mon, Jul 9, 2012 at 8:23 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> Hi,
>>>
>>> I found several problems in pg_receivexlog, e.g., memory leaks,
>>> file-descripter leaks, ..etc. The attached patch fixes these problems.
>>
>> While I don't doubt that what you've found are real problems, would
>> you mind explaining exactly what they are, so we don't have to
>> reverse-engineer the patch to figure that out?
>
> Yep.
>
> When an error happens after replication connection has been established,
> pg_receivexlog doesn't close an open file descriptor and release an allocated
> memory area. This was harmless before 16282ae688de2b320cf176e9be8a89e4dfc60698
> because pg_receivexlog exits immediately when an error happens. But
> currently in an error case, pg_receivexlog tries reconnecting to the server
> infinitely, so file descriptors and memory would leak. I think this is problem
> and should be fixed. The patch which I submitted yesterday changes
> pg_receivexlog so that it closes the open file and frees the memory area
> before reconnecting to the server.

Thanks. I get it now, and this explains why I didn't see it before - I
didn't check properly after we added the loop mode. Patch applied with
minor changes (e.g. there's no point in doing PQfinish(tmpconn) right
after you've verified tmpconn is NULL)

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

  6. Re: several problems in pg_receivexlog

    Magnus Hagander <magnus@hagander.net> — 2012-07-12T11:39:13Z

    On Tue, Jul 10, 2012 at 7:03 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Tue, Jul 10, 2012 at 3:23 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> Hi,
>>
>> I found several problems in pg_receivexlog, e.g., memory leaks,
>> file-descripter leaks, ..etc. The attached patch fixes these problems.
>>
>> ISTM there are still some other problems in pg_receivexlog, so I'll
>> read it deeply later.
>
> While pg_basebackup background process is streaming WAL records,
> if its replication connection is terminated (e.g., walsender in the server
> is accidentally terminated by SIGTERM signal), pg_basebackup ends
> up failing to include all required WAL files in the backup. The problem
> is that, in this case, pg_basebackup doesn't emit any error message at all.
> So an user might misunderstand that a base backup has been successfully
> taken even though it doesn't include all required WAL files.

Ouch. That is definitely a bug if it behaves that way.


> To fix this problem, I think that, when the replication connection is
> terminated, ReceiveXlogStream() should check whether we've already
> reached the stop point by calling stream_stop() before returning TRUE.
> If we've not yet (this means that we've not received all required WAL
> files yet), ReceiveXlogStream() should return FALSE and
> pg_basebackup should emit an error message.  Comments?

Doesn't it already return false because it detects the error of the
connection? What's the codepath where we end up returning true even
though we had a connection failure? Shouldn't that end up under the
"could not read copy data" branch, which already returns false?


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

  7. Re: several problems in pg_receivexlog

    Fujii Masao <masao.fujii@gmail.com> — 2012-07-12T16:07:13Z

    On Thu, Jul 12, 2012 at 8:39 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Tue, Jul 10, 2012 at 7:03 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Tue, Jul 10, 2012 at 3:23 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> Hi,
>>>
>>> I found several problems in pg_receivexlog, e.g., memory leaks,
>>> file-descripter leaks, ..etc. The attached patch fixes these problems.
>>>
>>> ISTM there are still some other problems in pg_receivexlog, so I'll
>>> read it deeply later.
>>
>> While pg_basebackup background process is streaming WAL records,
>> if its replication connection is terminated (e.g., walsender in the server
>> is accidentally terminated by SIGTERM signal), pg_basebackup ends
>> up failing to include all required WAL files in the backup. The problem
>> is that, in this case, pg_basebackup doesn't emit any error message at all.
>> So an user might misunderstand that a base backup has been successfully
>> taken even though it doesn't include all required WAL files.
>
> Ouch. That is definitely a bug if it behaves that way.
>
>
>> To fix this problem, I think that, when the replication connection is
>> terminated, ReceiveXlogStream() should check whether we've already
>> reached the stop point by calling stream_stop() before returning TRUE.
>> If we've not yet (this means that we've not received all required WAL
>> files yet), ReceiveXlogStream() should return FALSE and
>> pg_basebackup should emit an error message.  Comments?
>
> Doesn't it already return false because it detects the error of the
> connection? What's the codepath where we end up returning true even
> though we had a connection failure? Shouldn't that end up under the
> "could not read copy data" branch, which already returns false?

You're right. If the error is detected, that function always returns false
and the error message is emitted (but I think that current error message
"pg_basebackup: child process exited with error 1" is confusing....),
so it's OK. But if walsender in the server is terminated by SIGTERM,
no error is detected and pg_basebackup background process gets out
of the loop in ReceiveXlogStream() and returns true.

Regards,

-- 
Fujii Masao

  8. Re: several problems in pg_receivexlog

    Magnus Hagander <magnus@hagander.net> — 2012-07-12T16:15:07Z

    On Thu, Jul 12, 2012 at 6:07 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Thu, Jul 12, 2012 at 8:39 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> On Tue, Jul 10, 2012 at 7:03 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> On Tue, Jul 10, 2012 at 3:23 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>> Hi,
>>>>
>>>> I found several problems in pg_receivexlog, e.g., memory leaks,
>>>> file-descripter leaks, ..etc. The attached patch fixes these problems.
>>>>
>>>> ISTM there are still some other problems in pg_receivexlog, so I'll
>>>> read it deeply later.
>>>
>>> While pg_basebackup background process is streaming WAL records,
>>> if its replication connection is terminated (e.g., walsender in the server
>>> is accidentally terminated by SIGTERM signal), pg_basebackup ends
>>> up failing to include all required WAL files in the backup. The problem
>>> is that, in this case, pg_basebackup doesn't emit any error message at all.
>>> So an user might misunderstand that a base backup has been successfully
>>> taken even though it doesn't include all required WAL files.
>>
>> Ouch. That is definitely a bug if it behaves that way.
>>
>>
>>> To fix this problem, I think that, when the replication connection is
>>> terminated, ReceiveXlogStream() should check whether we've already
>>> reached the stop point by calling stream_stop() before returning TRUE.
>>> If we've not yet (this means that we've not received all required WAL
>>> files yet), ReceiveXlogStream() should return FALSE and
>>> pg_basebackup should emit an error message.  Comments?
>>
>> Doesn't it already return false because it detects the error of the
>> connection? What's the codepath where we end up returning true even
>> though we had a connection failure? Shouldn't that end up under the
>> "could not read copy data" branch, which already returns false?
>
> You're right. If the error is detected, that function always returns false
> and the error message is emitted (but I think that current error message
> "pg_basebackup: child process exited with error 1" is confusing....),
> so it's OK. But if walsender in the server is terminated by SIGTERM,
> no error is detected and pg_basebackup background process gets out
> of the loop in ReceiveXlogStream() and returns true.

Oh. Because the server does a graceful shutdown. D'uh, of course.

Then yes, your suggested fix seems like a good one.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

  9. Re: several problems in pg_receivexlog

    Fujii Masao <masao.fujii@gmail.com> — 2012-07-17T17:58:38Z

    On Fri, Jul 13, 2012 at 1:15 AM, Magnus Hagander <magnus@hagander.net> wrote:
> On Thu, Jul 12, 2012 at 6:07 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Thu, Jul 12, 2012 at 8:39 PM, Magnus Hagander <magnus@hagander.net> wrote:
>>> On Tue, Jul 10, 2012 at 7:03 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>> On Tue, Jul 10, 2012 at 3:23 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>>> Hi,
>>>>>
>>>>> I found several problems in pg_receivexlog, e.g., memory leaks,
>>>>> file-descripter leaks, ..etc. The attached patch fixes these problems.
>>>>>
>>>>> ISTM there are still some other problems in pg_receivexlog, so I'll
>>>>> read it deeply later.
>>>>
>>>> While pg_basebackup background process is streaming WAL records,
>>>> if its replication connection is terminated (e.g., walsender in the server
>>>> is accidentally terminated by SIGTERM signal), pg_basebackup ends
>>>> up failing to include all required WAL files in the backup. The problem
>>>> is that, in this case, pg_basebackup doesn't emit any error message at all.
>>>> So an user might misunderstand that a base backup has been successfully
>>>> taken even though it doesn't include all required WAL files.
>>>
>>> Ouch. That is definitely a bug if it behaves that way.
>>>
>>>
>>>> To fix this problem, I think that, when the replication connection is
>>>> terminated, ReceiveXlogStream() should check whether we've already
>>>> reached the stop point by calling stream_stop() before returning TRUE.
>>>> If we've not yet (this means that we've not received all required WAL
>>>> files yet), ReceiveXlogStream() should return FALSE and
>>>> pg_basebackup should emit an error message.  Comments?
>>>
>>> Doesn't it already return false because it detects the error of the
>>> connection? What's the codepath where we end up returning true even
>>> though we had a connection failure? Shouldn't that end up under the
>>> "could not read copy data" branch, which already returns false?
>>
>> You're right. If the error is detected, that function always returns false
>> and the error message is emitted (but I think that current error message
>> "pg_basebackup: child process exited with error 1" is confusing....),
>> so it's OK. But if walsender in the server is terminated by SIGTERM,
>> no error is detected and pg_basebackup background process gets out
>> of the loop in ReceiveXlogStream() and returns true.
>
> Oh. Because the server does a graceful shutdown. D'uh, of course.
>
> Then yes, your suggested fix seems like a good one.

Attached patch adds the fix.

Also I found I had forgotten to set the file descriptor to -1 at the end of
ReceiveXlogStream(), in previously-committed my patch. Attached patch
fixes this problem.

Regards,

-- 
Fujii Masao

  10. Re: several problems in pg_receivexlog

    Alvaro Herrera <alvherre@commandprompt.com> — 2012-07-31T15:06:32Z

    Excerpts from Magnus Hagander's message of jue jul 12 07:35:11 -0400 2012:
> On Tue, Jul 10, 2012 at 6:45 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

> > When an error happens after replication connection has been established,
> > pg_receivexlog doesn't close an open file descriptor and release an allocated
> > memory area. This was harmless before 16282ae688de2b320cf176e9be8a89e4dfc60698
> > because pg_receivexlog exits immediately when an error happens. But
> > currently in an error case, pg_receivexlog tries reconnecting to the server
> > infinitely, so file descriptors and memory would leak. I think this is problem
> > and should be fixed. The patch which I submitted yesterday changes
> > pg_receivexlog so that it closes the open file and frees the memory area
> > before reconnecting to the server.
> 
> Thanks. I get it now, and this explains why I didn't see it before - I
> didn't check properly after we added the loop mode. Patch applied with
> minor changes (e.g. there's no point in doing PQfinish(tmpconn) right
> after you've verified tmpconn is NULL)

For some reason, Magnus neglected to backpatch this to 9.2, so I just
did.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  11. Re: several problems in pg_receivexlog

    Alvaro Herrera <alvherre@commandprompt.com> — 2012-07-31T15:09:02Z

    Excerpts from Fujii Masao's message of mar jul 17 13:58:38 -0400 2012:

> >> You're right. If the error is detected, that function always returns false
> >> and the error message is emitted (but I think that current error message
> >> "pg_basebackup: child process exited with error 1" is confusing....),
> >> so it's OK. But if walsender in the server is terminated by SIGTERM,
> >> no error is detected and pg_basebackup background process gets out
> >> of the loop in ReceiveXlogStream() and returns true.
> >
> > Oh. Because the server does a graceful shutdown. D'uh, of course.
> >
> > Then yes, your suggested fix seems like a good one.
> 
> Attached patch adds the fix.
> 
> Also I found I had forgotten to set the file descriptor to -1 at the end of
> ReceiveXlogStream(), in previously-committed my patch. Attached patch
> fixes this problem.

This hasn't been committed yet AFAICT, and it probably needs a refresh
now after my changes to pg_basebackup.  Please update the patch.  Also,
if this is not in the Open Items list, please put it there so that we
don't forget it before the 9.2 release.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  12. Re: several problems in pg_receivexlog

    Fujii Masao <masao.fujii@gmail.com> — 2012-07-31T16:50:00Z

    On Wed, Aug 1, 2012 at 12:09 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
>
> Excerpts from Fujii Masao's message of mar jul 17 13:58:38 -0400 2012:
>
>> >> You're right. If the error is detected, that function always returns false
>> >> and the error message is emitted (but I think that current error message
>> >> "pg_basebackup: child process exited with error 1" is confusing....),
>> >> so it's OK. But if walsender in the server is terminated by SIGTERM,
>> >> no error is detected and pg_basebackup background process gets out
>> >> of the loop in ReceiveXlogStream() and returns true.
>> >
>> > Oh. Because the server does a graceful shutdown. D'uh, of course.
>> >
>> > Then yes, your suggested fix seems like a good one.
>>
>> Attached patch adds the fix.
>>
>> Also I found I had forgotten to set the file descriptor to -1 at the end of
>> ReceiveXlogStream(), in previously-committed my patch. Attached patch
>> fixes this problem.
>
> This hasn't been committed yet AFAICT, and it probably needs a refresh
> now after my changes to pg_basebackup.  Please update the patch.

I attached the updated version.

>  Also,
> if this is not in the Open Items list, please put it there so that we
> don't forget it before the 9.2 release.

Yep, done.

Regards,

-- 
Fujii Masao

  13. Re: several problems in pg_receivexlog

    Magnus Hagander <magnus@hagander.net> — 2012-08-06T11:48:32Z

    On Tue, Jul 31, 2012 at 5:06 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
>
> Excerpts from Magnus Hagander's message of jue jul 12 07:35:11 -0400 2012:
>> On Tue, Jul 10, 2012 at 6:45 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>
>> > When an error happens after replication connection has been established,
>> > pg_receivexlog doesn't close an open file descriptor and release an allocated
>> > memory area. This was harmless before 16282ae688de2b320cf176e9be8a89e4dfc60698
>> > because pg_receivexlog exits immediately when an error happens. But
>> > currently in an error case, pg_receivexlog tries reconnecting to the server
>> > infinitely, so file descriptors and memory would leak. I think this is problem
>> > and should be fixed. The patch which I submitted yesterday changes
>> > pg_receivexlog so that it closes the open file and frees the memory area
>> > before reconnecting to the server.
>>
>> Thanks. I get it now, and this explains why I didn't see it before - I
>> didn't check properly after we added the loop mode. Patch applied with
>> minor changes (e.g. there's no point in doing PQfinish(tmpconn) right
>> after you've verified tmpconn is NULL)
>
> For some reason, Magnus neglected to backpatch this to 9.2, so I just
> did.

Thanks. I believe that was just an oversight.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

  14. Re: several problems in pg_receivexlog

    Magnus Hagander <magnus@hagander.net> — 2012-08-06T11:56:35Z

    On Tue, Jul 31, 2012 at 6:50 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Wed, Aug 1, 2012 at 12:09 AM, Alvaro Herrera
> <alvherre@commandprompt.com> wrote:
>>
>> Excerpts from Fujii Masao's message of mar jul 17 13:58:38 -0400 2012:
>>
>>> >> You're right. If the error is detected, that function always returns false
>>> >> and the error message is emitted (but I think that current error message
>>> >> "pg_basebackup: child process exited with error 1" is confusing....),
>>> >> so it's OK. But if walsender in the server is terminated by SIGTERM,
>>> >> no error is detected and pg_basebackup background process gets out
>>> >> of the loop in ReceiveXlogStream() and returns true.
>>> >
>>> > Oh. Because the server does a graceful shutdown. D'uh, of course.
>>> >
>>> > Then yes, your suggested fix seems like a good one.
>>>
>>> Attached patch adds the fix.
>>>
>>> Also I found I had forgotten to set the file descriptor to -1 at the end of
>>> ReceiveXlogStream(), in previously-committed my patch. Attached patch
>>> fixes this problem.
>>
>> This hasn't been committed yet AFAICT, and it probably needs a refresh
>> now after my changes to pg_basebackup.  Please update the patch.
>
> I attached the updated version.

Thanks, applied.


>>  Also,
>> if this is not in the Open Items list, please put it there so that we
>> don't forget it before the 9.2 release.
>
> Yep, done.

And I'll go take it off :-)

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/