<div dir="ltr">Hi Ishii-San<div><br></div><div>Thanks for reviewing the patch. <br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 17, 2015 at 7:31 AM, Tatsuo Ishii <span dir="ltr"><<a href="mailto:ishii@postgresql.org" target="_blank">ishii@postgresql.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi Usama,<br>
<br>
I have taken a look at your patch.<br>
<br>
1) Below diff seems to be unnecessary because even without the "+ if<br>
(cache_connection)", the result would be same.<br></blockquote><div><br></div><div>Yes this is correct the result will be same after this new if statement. The reason I made this change was to bypass if statement with four strcmp()s when we already knew the cache_connection will remain false. Although might not make much difference but done this to improve performance of this condition statement.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
- /*<br>
- * For those special databases, and when frontend client exits abnormally,<br>
- * we don't cache connection to backend.<br>
- */<br>
- if ((sp &&<br>
- (!strcmp(sp->database, "template0") ||<br>
- !strcmp(sp->database, "template1") ||<br>
- !strcmp(sp->database, "postgres") ||<br>
- !strcmp(sp->database, "regression"))) ||<br>
- (*frontend != NULL &&<br>
- ((*frontend)->socket_state == POOL_SOCKET_EOF ||<br>
- (*frontend)->socket_state == POOL_SOCKET_ERROR)))<br>
- cache_connection = false;<br>
-<br>
+ if (cache_connection)<br>
+ {<br>
+ /*<br>
+ * For those special databases, and when frontend client exits abnormally,<br>
+ * we don't cache connection to backend.<br>
+ */<br>
+ if ((sp &&<br>
+ (!strcmp(sp->database, "template0") ||<br>
+ !strcmp(sp->database, "template1") ||<br>
+ !strcmp(sp->database, "postgres") ||<br>
+ !strcmp(sp->database, "regression"))) ||<br>
+ (*frontend != NULL &&<br>
+ ((*frontend)->socket_state == POOL_SOCKET_EOF ||<br>
+ (*frontend)->socket_state == POOL_SOCKET_ERROR)))<br>
+ cache_connection = false;<br>
+ }<br>
<br>
2) Shouldn't frontend_invalid variable be bool because it takes only 0<br>
or 1. If so, geterrposition()'s return type should be declared as<br>
bool.<br></blockquote><div><br></div><div>Yes, you are right. frontend_invalid should've been bool. I have updated this in attached version of the patch.</div><div><br></div><div><br></div><div>Thanks</div><div>Best regards</div><div>Muhammad Usama </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><div class="h5"><br>
Best regards,<br>
--<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_en.php</a><br>
Japanese:<a href="http://www.sraoss.co.jp" rel="noreferrer" target="_blank">http://www.sraoss.co.jp</a><br>
<br>
> Hi Ishii San<br>
><br>
> Can you please have a look at the attached patch for 3.4 branch.<br>
> Basically it introduces a new error level FRONTEND_ERROR for ereport(), to<br>
> inform the error handler not to cache the current backend connection. Other<br>
> than that, this FRONTEND_ERROR error level behaves and works similar to the<br>
> ereport(ERROR..)<br>
><br>
> Thanks<br>
> Best regards<br>
> Muhammad Usama<br>
><br>
><br>
> On Thu, Oct 15, 2015 at 4:45 PM, Muhammad Usama <<a href="mailto:m.usama@gmail.com">m.usama@gmail.com</a>> wrote:<br>
><br>
>><br>
>><br>
>> On Thu, Oct 15, 2015 at 2:23 AM, Tatsuo Ishii <<a href="mailto:ishii@postgresql.org">ishii@postgresql.org</a>><br>
>> wrote:<br>
>><br>
>>> Usama,<br>
>>><br>
>>> Can you please propose a patch to implement similar thing with 3.4? I<br>
>>> think it will make 3.4 more robust. I have no idea how to implement<br>
>>> it with 3.4's exception mechanism.<br>
>>><br>
>><br>
>> sure I will work on this<br>
>><br>
>> Thanks<br>
>> best regards<br>
>> Muhammad Usama<br>
>><br>
>>><br>
>>> Best regards,<br>
>>> --<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_en.php</a><br>
>>> Japanese:<a href="http://www.sraoss.co.jp" rel="noreferrer" target="_blank">http://www.sraoss.co.jp</a><br>
>>><br>
>>> > Hi Ishii San<br>
>>> ><br>
>>> > I think this difference of behavior between 3.3. And 3.4 is caused by<br>
>>> the<br>
>>> > commit "cb735f22441001b6afdbb5bac72808db66094ca9" that was not ported to<br>
>>> > the 3.4 branch.<br>
>>> ><br>
>>> ><br>
>>> <a href="http://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=cb735f22441001b6afdbb5bac72808db66094ca9" rel="noreferrer" target="_blank">http://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=cb735f22441001b6afdbb5bac72808db66094ca9</a><br>
>>> ><br>
>>> > Thanks<br>
>>> > Best regards<br>
>>> > Muhammad Usama<br>
>>> ><br>
>>> ><br>
>>> ><br>
>>> > On Tue, Oct 13, 2015 at 2:45 PM, Tatsuo Ishii <<a href="mailto:ishii@postgresql.org">ishii@postgresql.org</a>><br>
>>> wrote:<br>
>>> ><br>
>>> >> > Usama,<br>
>>> >> ><br>
>>> >> > While I have been working on:<br>
>>> >> > <a href="http://www.pgpool.net/mantisbt/view.php?id=147" rel="noreferrer" target="_blank">http://www.pgpool.net/mantisbt/view.php?id=147</a><br>
>>> >><br>
>>> >> Oops. This is wrong. Here is the correct one.<br>
>>> >><br>
>>> >> <a href="http://www.pgpool.net/mantisbt/view.php?id=145" rel="noreferrer" target="_blank">http://www.pgpool.net/mantisbt/view.php?id=145</a><br>
>>> >><br>
>>> >> > I found there was an interesting behavior change between pgpool-II<br>
>>> 3.3<br>
>>> >> > and 3.4.<br>
>>> >> ><br>
>>> >> > It turns out that the bug was related to the action when<br>
>>> >> > client_idle_limit is reached. In 3.3, when client_idle_limit is<br>
>>> >> > reached, following statement is executed (pool_process_query.c):<br>
>>> >> ><br>
>>> >> > if (idle_count > pool_config->client_idle_limit)<br>
>>> >> > {<br>
>>> >> > pool_log("pool_process_query: child connection forced to<br>
>>> terminate<br>
>>> >> due to client_idle_limit (%d) reached,<br>
>>> >> > pool_config->client_idle_limit);<br>
>>> >> > pool_send_error_message(frontend, MAJOR(backend),<br>
>>> >> > "57000", "connection terminated due to client idle<br>
>>> limit<br>
>>> >> reached",<br>
>>> >> > "","", __FILE__, __LINE__);<br>
>>> >> > return POOL_END_WITH_FRONTEND_ERROR;<br>
>>> >> > }<br>
>>> >> ><br>
>>> >> > then goes to this (child.c)<br>
>>> >> ><br>
>>> >> > if (status == POOL_END_WITH_FRONTEND_ERROR ||<br>
>>> >> > pool_config->connection_cache == 0 ||<br>
>>> >> > !strcmp(sp->database, "template0") ||<br>
>>> >> > !strcmp(sp->database, "template1") ||<br>
>>> >> > !strcmp(sp->database, "postgres") ||<br>
>>> >> > !strcmp(sp->database, "regression"))<br>
>>> >> > {<br>
>>> >> > reset_connection();<br>
>>> >> > pool_close(frontend);<br>
>>> >> > pool_send_frontend_exits(backend);<br>
>>> >> > pool_discard_cp(sp->user, sp->database, sp->major);<br>
>>> >> > }<br>
>>> >> ><br>
>>> >> > This results in closing connection to backend.<br>
>>> >> ><br>
>>> >> > In 3.4,<br>
>>> >> ><br>
>>> >> > if (idle_count > pool_config->client_idle_limit)<br>
>>> >> > {<br>
>>> >> > ereport(ERROR,<br>
>>> >> > (pool_error_code("57000"),<br>
>>> >> > errmsg("unable to read data"),<br>
>>> >> > errdetail("child connection forced to terminate due<br>
>>> to<br>
>>> >> client_idle_limit:%d is reached",<br>
>>> >> > pool_config->client_idle_limit)));<br>
>>> >> > }<br>
>>> >> ><br>
>>> >> > Then jump to backend_cleanup() and call pool_process_query() to<br>
>>> >> > execute queries defined in reset_query_list. This results in sending<br>
>>> >> > typically "DISCARD ALL" to backend. The problem reported in the bug<br>
>>> id<br>
>>> >> > 147 happens in this route (actually the reporter claims that<br>
>>> pgpool-II<br>
>>> >> > hangs in sending "DISCARD").<br>
>>> >> ><br>
>>> >> > In summary, when client idle limit reaches, pgpool-II 3.3 disconnects<br>
>>> >> > connections to backend, while 3.4 does not close the connections to<br>
>>> >> > backend and sends DISCARD to backend.<br>
>>> >> ><br>
>>> >> > We have a few reports that pgpool-II 3.4 hangs while sending DISCARD<br>
>>> >> > and I wonder those problems somewhat related to the behavior change<br>
>>> >> > described above because the code path in 3.4 is executed even if<br>
>>> >> > client_idle_limit is not reached (elog(ERROR...)).<br>
>>> >> ><br>
>>> >> > Usama,<br>
>>> >> ><br>
>>> >> > Was this behavior change intentional?<br>
>>> >> ><br>
>>> >> > Best regards,<br>
>>> >> > --<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_en.php</a><br>
>>> >> > Japanese:<a href="http://www.sraoss.co.jp" rel="noreferrer" target="_blank">http://www.sraoss.co.jp</a><br>
>>> >> > _______________________________________________<br>
>>> >> > pgpool-hackers mailing list<br>
>>> >> > <a href="mailto:pgpool-hackers@pgpool.net">pgpool-hackers@pgpool.net</a><br>
>>> >> > <a href="http://www.pgpool.net/mailman/listinfo/pgpool-hackers" rel="noreferrer" target="_blank">http://www.pgpool.net/mailman/listinfo/pgpool-hackers</a><br>
>>> >> _______________________________________________<br>
>>> >> pgpool-hackers mailing list<br>
>>> >> <a href="mailto:pgpool-hackers@pgpool.net">pgpool-hackers@pgpool.net</a><br>
>>> >> <a href="http://www.pgpool.net/mailman/listinfo/pgpool-hackers" rel="noreferrer" target="_blank">http://www.pgpool.net/mailman/listinfo/pgpool-hackers</a><br>
>>> >><br>
>>><br>
>><br>
>><br>
</div></div></blockquote></div><br></div></div></div>