<div dir="ltr">Hi Ishii-San<div><br></div><div>Many thanks for the review. I have pushed the fix in 3_4 and master branches</div><div><br></div><div>Best regards</div><div>Muhammad Usama</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 18, 2015 at 3:50 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Usama,<br>
<span class=""><br>
> Hi Ishii-San<br>
><br>
> Thanks for reviewing the patch.<br>
><br>
> On Tue, Nov 17, 2015 at 7:31 AM, Tatsuo Ishii <<a href="mailto:ishii@postgresql.org">ishii@postgresql.org</a>> wrote:<br>
><br>
>> 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>
>><br>
><br>
> Yes this is correct the result will be same after this new if statement.<br>
> The reason I made this change was to bypass if statement with four<br>
> strcmp()s when we already knew the cache_connection will remain false.<br>
> Although might not make much difference but done this to improve<br>
> performance of this condition statement.<br>
<br>
</span>Ok, that makes sense.<br>
<span class=""><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>
>><br>
><br>
> Yes, you are right. frontend_invalid should've been bool. I have updated<br>
> this in attached version of the patch.<br>
<br>
</span>Thanks. Looks good to me.<br>
<div class="HOEnZb"><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(),<br>
>> to<br>
>> > inform the error handler not to cache the current backend connection.<br>
>> Other<br>
>> > than that, this FRONTEND_ERROR error level behaves and works similar to<br>
>> 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>><br>
>> 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<br>
>> ported to<br>
>> >>> > the 3.4 branch.<br>
>> >>> ><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<br>
>> 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<br>
>> sending<br>
>> >>> >> > typically "DISCARD ALL" to backend. The problem reported in the<br>
>> 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<br>
>> disconnects<br>
>> >>> >> > connections to backend, while 3.4 does not close the connections<br>
>> to<br>
>> >>> >> > backend and sends DISCARD to backend.<br>
>> >>> >> ><br>
>> >>> >> > We have a few reports that pgpool-II 3.4 hangs while sending<br>
>> DISCARD<br>
>> >>> >> > and I wonder those problems somewhat related to the behavior<br>
>> 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>
>><br>
</div></div></blockquote></div><br></div>