<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 9, 2017 at 5:53 PM, Tatsuo Ishii <span dir="ltr"><<a href="mailto:ishii@sraoss.co.jp" target="_blank">ishii@sraoss.co.jp</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">> On Tue, May 9, 2017 at 6:16 AM, Tatsuo Ishii <<a href="mailto:ishii@sraoss.co.jp">ishii@sraoss.co.jp</a>> wrote:<br>
><br>
>> Usama,<br>
>><br>
>> While adding a new test case to 003.failover regression test, I found<br>
>> a corner case bug in primary failover.<br>
>><br>
>> Suppose Pgpool-II starts but is yet finding primary node. If primary<br>
>> failover happens, it skips finding primary node and let the initial<br>
>> value of it (Req_info->primary_node_id == -1) to be used as the new<br>
>> primary node id. As a result, no primary node id exists until next<br>
>> failover happens.<br>
>><br>
>> Initialy I thought The problem is in the code of<br>
>> pgpool_main.c:failover() which tries to optimize finding primary node<br>
>> process.<br>
>><br>
>> /*<br>
>> * If the down node was a standby node in streaming<br>
>> replication<br>
>> * mode, we can avoid calling<br>
>> find_primary_node_repeatedly() and<br>
>> * recognize the former primary as the new primary node,<br>
>> which<br>
>> * will reduce the time to process standby down.<br>
>> */<br>
>> else if (MASTER_SLAVE && pool_config->master_slave_sub_<wbr>mode<br>
>> == STREAM_MODE &&<br>
>> reqkind == NODE_DOWN_REQUEST)<br>
>> {<br>
>> if (Req_info->primary_node_id != node_id)<br>
>> new_primary = Req_info->primary_node_id;<br>
>> else<br>
>> new_primary =<br>
>> find_primary_node_repeatedly()<wbr>;<br>
>><br>
>> I was attempting to fix it by checking Req_info->primary_node_id to<br>
>> see if it's initial value (-1) or not. If it's -1,<br>
>> find_primary_node_repeatedly() need to be called.<br>
>><br>
>> But looking into pgpool_main() closely, I suspect there's a<br>
>> fundamental problem:<br>
>><br>
>> 1) It processes failover in CHECK_REQUEST *before* setting<br>
>> Req_info->primary_node_id.<br>
>><br>
>> /*<br>
>> * check for child signals to ensure child startup before<br>
>> reporting successfull start<br>
>> */<br>
>> CHECK_REQUEST;<br>
>><br>
>> ereport(LOG,<br>
>> (errmsg("%s successfully started. version %s<br>
>> (%s)", PACKAGE, VERSION, PGPOOLVERSION)));<br>
>><br>
>> /*<br>
>> * if the primary node id is not loaded by watchdog, search for it<br>
>> */<br>
>> if (Req_info->primary_node_id < 0)<br>
>> {<br>
>> /* Save primary node id */<br>
>> Req_info->primary_node_id = find_primary_node();<br>
>> }<br>
>><br>
>> 2) It uses find_primary_node(), rather than<br>
>> find_primary_node_repeatedly()<wbr>. So if by some reasons (for example<br>
>> the backend does not come up yet), find_primary_node() will fail<br>
>> and Req_info->primary_node_id is set to -1.<br>
>><br>
>> I think proper fix will be moving the CHECK_REQUEST call above inside<br>
>> main loop, and change the find_primary_node() call to<br>
>> find_primary_node_repeatedly()<wbr>.<br>
>><br>
>> Attached is the patch to do that (plus change the<br>
>> search_primary_node_timeout to smaller value in 055.backend_all_down<br>
>> test. Otherwise, regression timeout is triggered) against master<br>
>> branch.<br>
>><br>
>> What do you think?<br>
>><br>
><br>
><br>
> Waoo thanks for catching this, it is a really annoying issue, I think your<br>
> patch does solve the problem and is the right approach,<br>
> But I was thinking what if we move search for the primary node before<br>
> starting the child processes. So that we spawn the child processes after<br>
> finishing all the<br>
> startup rituals?<br>
<br>
</div></div>Oh I think that will make even things safer.<br>
<span class=""><br>
> Do you think it will cause some issues?<br>
<br>
</span>So far there's nothing I can think of. Let me try it. I will report<br>
back tomorrow.<br></blockquote><div><br></div><div>Many thanks</div><div><br></div><div>Best regards</div><div>Muhammad Usama </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Best regards,<br>
<div class="HOEnZb"><div class="h5">--<br>
Tatsuo Ishii<br>
SRA OSS, Inc. Japan<br>
English: <a href="http://www.sraoss.co.jp/index_en.php" rel="noreferrer" target="_blank">http://www.sraoss.co.jp/index_<wbr>en.php</a><br>
Japanese:<a href="http://www.sraoss.co.jp" rel="noreferrer" target="_blank">http://www.sraoss.co.<wbr>jp</a><br>
</div></div></blockquote></div><br></div></div>