[Pgpool-hackers] Patch to avoid crashes after adding new node in Master/Slave mode

[Gurjeet Singh]

On Sat, Aug 20, 2011 at 12:19 AM, Tatsuo Ishii <ishii at sraoss.co.jp> wrote:

> > Thanks for analyzing the problem and patches.
> >
> >>     The ability to add new slave nodes, without requiring a restart was
> >> added in commit [1].
> >>
> >>     One problem that was overlooked was that in main(), when
> initializing
> >> my_backend_status[], only NUM_BACKENDS number of elements of that array
> are
> >> initialized and used for the rest of the life of the pool_worker_child
> >> process. Since the NUM_BACKEDS increases everytime a new slave backend
> is
> >> added, but the corresponding element in the private array is still NULL,
> the
> >> pool_worker_child crashes when trying to check VALID_BACKEND(i) in
> >> establish_persistent_connection().
> >>
> >>     The attached micro patch avoids the crashes by initializing the
> private
> >> array's all MAX_NUM_BACKENDS instead of just NUM_BACKENDS.
> >>
> >>     This will also avoid crashes of pcp_attach_node when it tries to
> check
> >> VALID_BACKEND() in send_failback_request(). It crashes because, same as
> >> above, its private array is initialized only up to NUM_BACKENDS and the
> ID
> >> of the new slave node being attached is greater than current
> NUM_BACKENDS.
> >>
> >>     The bigger concern, which this patch does not address, is that of
> >> keeping my_backend_status[] up-to-date in pool_worker_child process and
> any
> >> other auxiliary processes.
> >
> > Ok. For this I will add a flag on shared memory area which is set by
> > main process. pool_worker_child process and other auxiliary processes
> > (curretly it's only pcp process) will check the flag and restart
> > itself if the flag is set.
>
> Included patches let pgpool main send SIGUSR1 signal to worker process
> and pcp process to restart them whenever they are convenient.
>
>
Hi Tatsuo,

    Thanks for the quick turnaround times.

    After posting my original concern

>>     The bigger concern, which this patch does not address, is that of
>> keeping my_backend_status[] up-to-date in pool_worker_child process and
any
>> other auxiliary processes.

        I thought a bit more, and noticed that my_backend_status[] is an
array of pointers. So as long as the shared memory that they point to does
not get freed/reused we can rely on the my_backend_status[] elements to be
perfectly up-to-date whenever the status of a backend is changed by anyone.

    I maybe wrong, but I think there's no need send signals to the PCP child
and the worker backend since they can see the updated status values in
my_backend_status[].

Regards,
-- 
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://pgfoundry.org/pipermail/pgpool-hackers/attachments/20110822/54511ca8/attachment.html>